Re: [PATCH-V2 08/12] i2c: pxa: enable/disable i2c module across msg xfer

[Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Friday 03 July 2015 11:53 PM, Vaibhav Hiremath wrote:
>
>
> On Friday 03 July 2015 08:58 PM, Robert Jarzmik wrote:
>> Vaibhav Hiremath <vaibhav.hiremath-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 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.
>

I have taken care of all comments on the 3 patches.

Just in case if you have any comments on other patches in series,
I will wait for a day before pushing next version.

Thanks,
Vaibhav