From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 36E56C67863 for ; Wed, 24 Oct 2018 09:15:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F184920824 for ; Wed, 24 Oct 2018 09:15:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F184920824 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=roeck-us.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727981AbeJXRmz (ORCPT ); Wed, 24 Oct 2018 13:42:55 -0400 Received: from linux-outbound-1.webhostbox.net ([207.174.213.190]:49437 "EHLO linux-outbound-1.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726256AbeJXRmy (ORCPT ); Wed, 24 Oct 2018 13:42:54 -0400 X-Greylist: delayed 302 seconds by postgrey-1.27 at vger.kernel.org; Wed, 24 Oct 2018 13:42:54 EDT Received: from bh-25.webhostbox.net (unknown [172.16.210.78]) by linux-outbound-1.webhostbox.net (Postfix) with ESMTP id C6538241581; Wed, 24 Oct 2018 07:23:45 +0000 (GMT) Received: from [127.0.0.1] (port=35345 helo=bh-25.webhostbox.net) by bh-25.webhostbox.net with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.91) (envelope-from ) id 1gFFBT-00Crh3-RS; Wed, 24 Oct 2018 09:10:36 +0000 Received: from 185.7.230.214 ([185.7.230.214]) by cp2.active-venture.com (Horde Framework) with HTTP; Wed, 24 Oct 2018 09:10:35 +0000 Date: Wed, 24 Oct 2018 09:10:35 +0000 Message-ID: <20181024091035.Horde.F-g3MefpEv95EQd6w9-rB3D@cp2.active-venture.com> From: linux@roeck-us.net To: Nicolin Chen Cc: jdelvare@suse.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 4/5] hwmon: (ina3221) Make sure data is ready before reading References: <20181024023623.4231-1-nicoleotsuka@gmail.com> <20181024023623.4231-5-nicoleotsuka@gmail.com> In-Reply-To: <20181024023623.4231-5-nicoleotsuka@gmail.com> User-Agent: Horde Application Framework 5 Content-Type: text/plain; charset=utf-8; format=flowed; DelSp=Yes MIME-Version: 1.0 Content-Disposition: inline X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Nicolin Chen : > The data might need some time to get ready after channel enabling, > although the data register is always readable. The CVRF bit is to > indicate that data conversion is finished, so polling the CVRF bit > before data reading could ensure the result being valid. > > An alternative way could be to wait for expected time between the > channel enabling and the data reading. And this could avoid extra > I2C communications. However, INA3221 seemly takes longer time than > what's stated in the datasheet. Test results show that sometimes > it couldn't finish data conversion in time. > > So this patch plays safe by adding a CVRF polling to make sure the > data register is updated with the new data. > > Signed-off-by: Nicolin Chen > --- > * Moved CVRF polling to data read routine > * Added calculation of wait time based on conversion time setting > > drivers/hwmon/ina3221.c | 46 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c > index 10e8347a3c80..9bbac826e50b 100644 > --- a/drivers/hwmon/ina3221.c > +++ b/drivers/hwmon/ina3221.c > @@ -44,6 +44,13 @@ > #define INA3221_CONFIG_MODE_SHUNT BIT(0) > #define INA3221_CONFIG_MODE_BUS BIT(1) > #define INA3221_CONFIG_MODE_CONTINUOUS BIT(2) > +#define INA3221_CONFIG_VSH_CT_SHIFT 3 > +#define INA3221_CONFIG_VSH_CT_MASK GENMASK(5, 3) > +#define INA3221_CONFIG_VSH_CT(x) (((x) & GENMASK(5, 3)) >> 3) > +#define INA3221_CONFIG_VBUS_CT_SHIFT 6 > +#define INA3221_CONFIG_VBUS_CT_MASK GENMASK(8, 6) > +#define INA3221_CONFIG_VBUS_CT(x) (((x) & GENMASK(8, 6)) >> 6) > +#define INA3221_CONFIG_CHs_EN_MASK GENMASK(14, 12) > #define INA3221_CONFIG_CHx_EN(x) BIT(14 - (x)) > > #define INA3221_RSHUNT_DEFAULT 10000 > @@ -52,6 +59,9 @@ enum ina3221_fields { > /* Configuration */ > F_RST, > > + /* Status Flags */ > + F_CVRF, > + > /* Alert Flags */ > F_WF3, F_WF2, F_WF1, > F_CF3, F_CF2, F_CF1, > @@ -63,6 +73,7 @@ enum ina3221_fields { > static const struct reg_field ina3221_reg_fields[] = { > [F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15), > > + [F_CVRF] = REG_FIELD(INA3221_MASK_ENABLE, 0, 0), > [F_WF3] = REG_FIELD(INA3221_MASK_ENABLE, 3, 3), > [F_WF2] = REG_FIELD(INA3221_MASK_ENABLE, 4, 4), > [F_WF1] = REG_FIELD(INA3221_MASK_ENABLE, 5, 5), > @@ -111,6 +122,28 @@ static inline bool ina3221_is_enabled(struct > ina3221_data *ina, int channel) > return ina->reg_config & INA3221_CONFIG_CHx_EN(channel); > } > > +/* Lookup table for Bus and Shunt conversion times in usec */ > +static const u16 ina3221_conv_time[] = { > + 140, 204, 332, 588, 1100, 2116, 4156, 8244, > +}; > + > +static inline int ina3221_wait_for_data(struct ina3221_data *ina) > +{ > + u32 channels = hweight16(ina->reg_config & INA3221_CONFIG_CHs_EN_MASK); > + u32 vbus_ct_idx = INA3221_CONFIG_VBUS_CT(ina->reg_config); > + u32 vsh_ct_idx = INA3221_CONFIG_VSH_CT(ina->reg_config); > + u32 vbus_ct = ina3221_conv_time[vbus_ct_idx]; > + u32 vsh_ct = ina3221_conv_time[vsh_ct_idx]; > + u32 wait, cvrf; > + > + /* Calculate total conversion time */ > + wait = channels * (vbus_ct + vsh_ct); > + > + /* Polling the CVRF bit to make sure read data is ready */ > + return regmap_field_read_poll_timeout(ina->fields[F_CVRF], > + cvrf, cvrf, wait, 100000); > +} > + > static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg, > int *val) > { > @@ -150,6 +183,12 @@ static int ina3221_read_in(struct device *dev, > u32 attr, int channel, long *val) > if (!ina3221_is_enabled(ina, channel)) > return -ENODATA; > > + ret = ina3221_wait_for_data(ina); > + if (ret) { > + dev_err(dev, "Timed out at waiting for CVRF bit\n"); > + return ret; > + } Thanks for explaining why we can't just blindly wait. However, I am concerned about this log message: If something is wrong with the chip, this will spam the kernel log. Can you drop the message here and below ? After all, the error will be reported to userspace, and a kernel log message should not be necessary. Thanks, Guenter > + > ret = ina3221_read_value(ina, reg, ®val); > if (ret) > return ret; > @@ -189,6 +228,13 @@ static int ina3221_read_curr(struct device > *dev, u32 attr, > case hwmon_curr_input: > if (!ina3221_is_enabled(ina, channel)) > return -ENODATA; > + > + ret = ina3221_wait_for_data(ina); > + if (ret) { > + dev_err(dev, "Timed out at waiting for CVRF bit\n"); > + return ret; > + } > + > /* fall through */ > case hwmon_curr_crit: > case hwmon_curr_max: > -- > 2.17.1