From mboxrd@z Thu Jan 1 00:00:00 1970 From: vaibhav.hiremath@linaro.org (Vaibhav Hiremath) Date: Fri, 03 Jul 2015 23:53:56 +0530 Subject: [PATCH-V2 08/12] i2c: pxa: enable/disable i2c module across msg xfer In-Reply-To: <87oajtb6r0.fsf@belgarion.home> References: <1434383399-2370-1-git-send-email-vaibhav.hiremath@linaro.org> <1434383399-2370-9-git-send-email-vaibhav.hiremath@linaro.org> <87oajtb6r0.fsf@belgarion.home> Message-ID: <5596D33C.6010100@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday 03 July 2015 08:58 PM, Robert Jarzmik wrote: > Vaibhav Hiremath writes: > >> #define _IBMR(i2c) ((i2c)->reg_ibmr) >> @@ -286,6 +287,22 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why) >> static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret); >> static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id); >> >> +/* enable/disable i2c unit */ >> +static inline int i2c_pxa_is_enabled(struct pxa_i2c *i2c) >> +{ >> + return (readl(_ICR(i2c)) & ICR_IUE); >> +} >> + >> +static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable) >> +{ >> + if (enable && !i2c_pxa_is_enabled(i2c)) { >> + writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c)); >> + udelay(100); >> + } else { >> + writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c)); >> + } >> +} >> + >> static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c) >> { >> return !(readl(_ICR(i2c)) & ICR_SCLE); >> @@ -482,8 +499,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) >> i2c_pxa_set_slave(i2c, 0); >> >> /* enable unit */ >> - writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c)); >> - udelay(100); >> + i2c_pxa_enable(i2c, true); >> } >> >> >> @@ -840,6 +856,9 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, >> struct pxa_i2c *i2c = adap->algo_data; >> int ret, i; >> >> + /* Enable i2c unit */ >> + i2c_pxa_enable(i2c, true); >> + >> /* If the I2C controller is disabled we need to reset it >> (probably due to a suspend/resume destroying state). We do >> this here as we can then avoid worrying about resuming the >> @@ -860,6 +879,11 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, >> ret = -EREMOTEIO; >> out: >> i2c_pxa_set_slave(i2c, ret); >> + >> + /* disable i2c unit */ >> + if (i2c->disable_after_xfer) >> + i2c_pxa_enable(i2c, false); >> + >> return ret; >> } >> >> @@ -1075,6 +1099,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num >> struct pxa_i2c *i2c = adap->algo_data; >> int ret, i; >> >> + /* Enable i2c unit */ >> + i2c_pxa_enable(i2c, true); > Okay, what happens in master mode when we get there on the 2nd xfer : > - i2c is enabled AFAIU. > - as a consequence i2c_pxa_is_enabled() returns true > - as a consequence, i2c_pxa_enable() _disables_ the i2c, right ? > - as a consequence i2c is broken > > Is this correct, because if it is the patch needs a rework ? > Good catch :) I will fix this in next version. Thanks, Vaibhav From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vaibhav Hiremath Subject: Re: [PATCH-V2 08/12] i2c: pxa: enable/disable i2c module across msg xfer Date: Fri, 03 Jul 2015 23:53:56 +0530 Message-ID: <5596D33C.6010100@linaro.org> References: <1434383399-2370-1-git-send-email-vaibhav.hiremath@linaro.org> <1434383399-2370-9-git-send-email-vaibhav.hiremath@linaro.org> <87oajtb6r0.fsf@belgarion.home> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <87oajtb6r0.fsf-4ty26DBLk+jEm7gnYqmdkQ@public.gmane.org> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Robert Jarzmik Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org, Yi Zhang List-Id: linux-i2c@vger.kernel.org On Friday 03 July 2015 08:58 PM, Robert Jarzmik wrote: > Vaibhav Hiremath writes: > >> #define _IBMR(i2c) ((i2c)->reg_ibmr) >> @@ -286,6 +287,22 @@ static void i2c_pxa_scream_blue_murder(struct pxa_i2c *i2c, const char *why) >> static void i2c_pxa_master_complete(struct pxa_i2c *i2c, int ret); >> static irqreturn_t i2c_pxa_handler(int this_irq, void *dev_id); >> >> +/* enable/disable i2c unit */ >> +static inline int i2c_pxa_is_enabled(struct pxa_i2c *i2c) >> +{ >> + return (readl(_ICR(i2c)) & ICR_IUE); >> +} >> + >> +static inline void i2c_pxa_enable(struct pxa_i2c *i2c, bool enable) >> +{ >> + if (enable && !i2c_pxa_is_enabled(i2c)) { >> + writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c)); >> + udelay(100); >> + } else { >> + writel(readl(_ICR(i2c)) & ~ICR_IUE, _ICR(i2c)); >> + } >> +} >> + >> static inline int i2c_pxa_is_slavemode(struct pxa_i2c *i2c) >> { >> return !(readl(_ICR(i2c)) & ICR_SCLE); >> @@ -482,8 +499,7 @@ static void i2c_pxa_reset(struct pxa_i2c *i2c) >> i2c_pxa_set_slave(i2c, 0); >> >> /* enable unit */ >> - writel(readl(_ICR(i2c)) | ICR_IUE, _ICR(i2c)); >> - udelay(100); >> + i2c_pxa_enable(i2c, true); >> } >> >> >> @@ -840,6 +856,9 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, >> struct pxa_i2c *i2c = adap->algo_data; >> int ret, i; >> >> + /* Enable i2c unit */ >> + i2c_pxa_enable(i2c, true); >> + >> /* If the I2C controller is disabled we need to reset it >> (probably due to a suspend/resume destroying state). We do >> this here as we can then avoid worrying about resuming the >> @@ -860,6 +879,11 @@ static int i2c_pxa_pio_xfer(struct i2c_adapter *adap, >> ret = -EREMOTEIO; >> out: >> i2c_pxa_set_slave(i2c, ret); >> + >> + /* disable i2c unit */ >> + if (i2c->disable_after_xfer) >> + i2c_pxa_enable(i2c, false); >> + >> return ret; >> } >> >> @@ -1075,6 +1099,9 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num >> struct pxa_i2c *i2c = adap->algo_data; >> int ret, i; >> >> + /* Enable i2c unit */ >> + i2c_pxa_enable(i2c, true); > Okay, what happens in master mode when we get there on the 2nd xfer : > - i2c is enabled AFAIU. > - as a consequence i2c_pxa_is_enabled() returns true > - as a consequence, i2c_pxa_enable() _disables_ the i2c, right ? > - as a consequence i2c is broken > > Is this correct, because if it is the patch needs a rework ? > Good catch :) I will fix this in next version. Thanks, Vaibhav