From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liam Breck Subject: Re: [PATCH v4 4/4] power: bq24190_charger: Delay before polling reset flag Date: Sat, 8 Apr 2017 12:25:41 -0700 Message-ID: References: <20170406031048.12401-1-liam@networkimprov.net> <20170406031048.12401-5-liam@networkimprov.net> <3a514e1e-8ad0-4951-c675-10c2e7de37ec@redhat.com> <0ce727cf-7871-89a8-7bec-7915f2699a73@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-io0-f195.google.com ([209.85.223.195]:34509 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750997AbdDHTZn (ORCPT ); Sat, 8 Apr 2017 15:25:43 -0400 Received: by mail-io0-f195.google.com with SMTP id n76so10368463ioe.1 for ; Sat, 08 Apr 2017 12:25:42 -0700 (PDT) In-Reply-To: <0ce727cf-7871-89a8-7bec-7915f2699a73@redhat.com> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Hans de Goede Cc: Sebastian Reichel , linux-pm@vger.kernel.org, Tony Lindgren , Liam Breck On Sat, Apr 8, 2017 at 2:58 AM, Hans de Goede 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 >> wrote: >>> >>> Hi, >>> >>> On 06-04-17 05:10, Liam Breck wrote: >>>> >>>> >>>> From: Liam Breck >>>> >>>> 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 >>>> Cc: Hans de Goede >>>> Signed-off-by: Liam Breck >>>> --- >>>> 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; >>>> >>> >