Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag

[Liam Breck <liam@networkimprov.net>]

On Sat, Apr 8, 2017 at 2:58 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 07-04-17 23:46, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Thu, Apr 6, 2017 at 12:20 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 06-04-17 05:10, Liam Breck wrote:
>>>>
>>>>
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> On chip reset, polling loop was reading reset register immediately.
>>>> Instead, call udelay() before reading chip register.
>>>
>>>
>>>
>>> Can you extend the commit message to explain why udelay is being
>>> called ?  IOW what is the problem with reading the register
>>> immediately which this patch addresses ?
>>
>>
>> I thought I'd read that reset isn't instantaneous, but in testing it
>> seems to be. So I will revert to the original post-reset delay, and
>> change udelay() to usleep_range(10, 20). Any thoughts on those
>> numbers?
>
>
> Using usleep makes no sense for such short delays, so I would stick
> with udelay.
>
> More importantly you still have not explained why we need to delay
> *at all* esp. such a short delay, 10us is a single bit at 100kHz i2c
> so the set bq24190 register-pointer part of the i2c transfer which
> happens before reading back the reset-bit alone takes 20 times as
> long as the delay.
>
> I still don't understand why a delay is needed at all, what are
> you trying to achieve with this delay ? Are you seeing a specific
> problem you are trying to fix ?

A loop with udelay(10) was in the code already, because at manual
9.5.1.2 the note for bit 7 says "Back to 0 after register reset".

But I think you're right, we don't need to loop checking this; I'll
just return -EIO (?) if bit 7 isn't 0 -- which presumably means
charger is kaput.



>
>
>
>
>>
>>
>>
>>>
>>>
>>>
>>>
>>>>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>> ---
>>>>  drivers/power/supply/bq24190_charger.c | 8 ++------
>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index fa29cb3..71a796e 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -543,18 +543,14 @@ static int bq24190_register_reset(struct
>>>> bq24190_dev_info *bdi)
>>>>
>>>>         /* Reset bit will be cleared by hardware so poll until it is */
>>>>         do {
>>>> +               udelay(10);
>>>>                 ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>>>>                                 BQ24190_REG_POC_RESET_MASK,
>>>>                                 BQ24190_REG_POC_RESET_SHIFT,
>>>>                                 &v);
>>>>                 if (ret < 0)
>>>>                         return ret;
>>>> -
>>>> -               if (!v)
>>>> -                       break;
>>>> -
>>>> -               udelay(10);
>>>> -       } while (--limit);
>>>> +       } while (v && --limit);
>>>>
>>>>         if (!limit)
>>>>                 return -EIO;
>>>>
>>>
>