Date:   Sun, 19 Jun 2022 12:32:21 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     ChiYuan Huang <u0084500@...il.com>
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        cy_huang <cy_huang@...htek.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: adc: Add rtq6056 support

On Sat, 18 Jun 2022 23:49:27 +0800
ChiYuan Huang <u0084500@...il.com> wrote:

> Jonathan Cameron <Jonathan.Cameron@...wei.com> 於 2022年6月18日 週六 凌晨1:10寫道：
> >
> > On Fri, 17 Jun 2022 17:32:55 +0800
> > cy_huang <u0084500@...il.com> wrote:
> >  
> > > From: ChiYuan Huang <cy_huang@...htek.com>
> > >
> > > Add Richtek RTQ6056 supporting.
> > >
> > > It can be used for the system to monitor load current and power with 16bit
> > > resolution.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>  
> >
> > Hi ChiYuan,
> >
> > Comments inline.
> >
> > It would be nice to consider how to take advantage of the fact we know which
> > channels are of interest in buffered mode, and change the operating mode to
> > suite.
> >  
> I implement the  buffered mode is for the debugging or continuous
> tracking purpose.
> 
> BUS channel is for input power source voltage
> Shunt channel is for the cross voltage difference for Rshunt.
> Current channel is calculated by Vshunt/Rshunt and the value converted by HW.
> Power channel is calculated by BUS voltage multiple Current channel,
> and also the value converted by HW.
> 
> For the opmode question, this IC already designed for low quiescent.
> If IC's in continuous mode, the typical quiescent current is still around 550uA.
> 'Shutdown' mode only use typical 3.5uA.
> 
> Like as you said, I may consider to use pm_runtime plus autosuspend to
> minimize the wait time for the first new sample ready.
Ok.

> 
> > > ---
> > >  .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |  58 +++
> > >  drivers/iio/adc/Kconfig                            |  15 +
> > >  drivers/iio/adc/Makefile                           |   1 +
> > >  drivers/iio/adc/rtq6056-adc.c                      | 548 +++++++++++++++++++++
> > >  4 files changed, 622 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
> > >  create mode 100644 drivers/iio/adc/rtq6056-adc.c
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 b/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
> > > new file mode 100644
> > > index 00000000..0516429
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
> > > @@ -0,0 +1,58 @@
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_voltage0_raw  
> >
> > Documentation can't be duplicated in mulitple files so these standard
> > attributes should rely only on the main docs.  If we duplicate it breaks
> > building the html docs unfortunately.
> >  
> Does it mean there's no need to explain it here and covert this
> voltage to millivolt?

Yes, in_voltage0_raw * in_voltage0_scale must be in millivolts
and you can't have extra documentation here without breaking the
docs build.


> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 48ace74..0b2d17c 100644
> > > diff --git a/drivers/iio/adc/rtq6056-adc.c b/drivers/iio/adc/rtq6056-adc.c
> > > new file mode 100644
> > > index 00000000..8374fce
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/rtq6056-adc.c
> > > @@ -0,0 +1,548 @@

> > > +
> > > +#define RTQ6056_VENDOR_ID    0x1214
> > > +#define RTQ6056_DEFAULT_CONFIG       0x4127  
> >
> > This is not defined in terms of the field provided below
> > so it's not obvious what the initial state is.
> >  
> To leave a comment about the default state?
> Or use the macro to define the all field?

Macro to define all fields means we don't have to check the
comment matches the code, so I would prefer that approach.
If hard to use macros for some reason, comment would be acceptable.




> > > +
> > > +static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
> > > +     [RTQ6056_CH_VSHUNT] = "Vshunt(uV)",  
> >
> > Units must be the standard ones for the IIO channel types.
> > Hence documenting them here is misleading.
> >  
> Can I use the "extend_name' and remove all the 'read_label' and
> channel_label variable?

No.  We very very rarely use extend name these days.  It was a bad
design decision and makes it harder to write userspace code.  Label
is the preferred way to provide this information today.

static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
     [RTQ6056_CH_VSHUNT] = "Vshunt",  
     [RTQ6056_CH_VBUS] = "Vbus",
     [RTQ6056_CH_POWER] = "Power",
     [RTQ6056_CH_CURRENT] = "Current",
};

would be my suggestion for the labels.  Power and Current are a bit
redundant but we need to provide something alongside the useful
Vshunt and Vbus.




> And also , the unit will be removed. Thx.
> > > +     [RTQ6056_CH_VBUS] = "Vbus(mV)",
> > > +     [RTQ6056_CH_POWER] = "Power(mW)",
> > > +     [RTQ6056_CH_CURRENT] = "Current(mA)",
> > > +};
> > ...  
> > > +static int rtq6056_probe(struct i2c_client *i2c)
> > > +{
> > > +     struct iio_dev *indio_dev;
> > > +     struct rtq6056_priv *priv;
> > > +     unsigned int vendor_id, shunt_resistor_uohm;
> > > +     int ret;
> > > +
> > > +     if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*priv));
> > > +     if (!indio_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     priv = iio_priv(indio_dev);
> > > +     priv->dev = &i2c->dev;
> > > +     priv->vshuntct = priv->vbusct = RTQ6056_DEFAULT_CONVT;
> > > +     priv->avg_sample = RTQ6056_DEFAULT_AVG;
> > > +     i2c_set_clientdata(i2c, priv);
> > > +
> > > +     priv->regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);  
> > You make a lot of use of regmap in this function. I would suggest
> > first allocating into a local variable, then using that to set
> > priv->regmap.  The local variable can then be used avoid having
> > priv->regmap everywhere.
> >  
> dev_get_regmap(), instead?

You could use that, but I was thinking just

	struct regmap *regmap;
	...
	regmap = devm_regmap_init_i2c(i2c, &...)
	if (IS_ERR(regmap))
		return ...

	priv->regmap = regmap;

	ret = regmap_read(regmap, ....
etc.	


...

> > > +static void rtq6056_shutdown(struct i2c_client *i2c)
> > > +{
> > > +     struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> > > +
> > > +     /* Config opmode to 'shutdown' mode to minimize quiescient current */
> > > +     regmap_field_write(priv->rm_fields[F_OPMODE], 0);  
> >
> > Unusual to provide a shutdown for a device as simple as an ADC.  I'd expect
> > the overall system to power down if we hit this and hence also cover
> > the ADC.  If that's not the case, then perhaps add a comment explaining
> > that.
> >  
> That's because some application use VBAT as the VDD source.
> Not all applcation use the PMIC buck or ldo as the supply for RTQ6056.
> If use VBAT, then the shutdown quiescent current is important.

Ok. Add that information as a comment so we know why this is done
when looking back at the driver in future.

Btw, to save on long emails that need scrolling through it's fine
to just crop out any sections where you are accepting the feedback
and no additional discussion is needed.  Makes the whole process
more efficient as if you don't reply to some feedback I'll assume
you are accepting it.

Thanks,

Jonathan

Powered by blists - more mailing lists