Message-ID: <20220716183750.311f449c@jic23-huawei>
Date:   Sat, 16 Jul 2022 18:37:50 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     ChiYuan Huang <u0084500@...il.com>
Cc:     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 v5 2/2] iio: adc: Add rtq6056 support

On Mon, 11 Jul 2022 10:48:17 +0800
ChiYuan Huang <u0084500@...il.com> wrote:

> Jonathan Cameron <jic23@...nel.org> 於 2022年7月8日 週五 凌晨1:20寫道：
> >
> > On Wed,  6 Jul 2022 22:11:42 +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 16-bit
> > > resolution.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>  
> >
> > Various feedback inline.
> >
> > Thanks,
> >
> > Jonathan
> >  
> > > ---
> > > Since v5
> > > - Fix kernel version text for ABI.
> > >
> > > Since v4
> > > - Add '__aligned(8)' for timestamp member in buffer_trigger_handler function.
> > > - Declare timestamp from 'int64_t' to more unified 's64'.
> > >
> > > Since v3
> > > - Refine pm_runtime API calling order in 'read_channel' API.
> > > - Fix vshunt wrong scale for divider.
> > > - Refine the comment text.
> > > - Use 'devm_add_action_or_reset' to decrease the code usage in probe
> > >   function.
> > > - Use RUNTIME_PM_OPS to replace SET_RUNTIME_PM_OPS.
> > > - minor fix for the comma.
> > > - Use pm_ptr to replace the direct assigned pm_ops.
> > >
> > > Since v2
> > > - Rename file from 'rtq6056-adc' to 'rtq6056'.
> > > - Refine the ABI, if generic already defined it, remove it and check the channel
> > >   report unit.
> > > - Add copyright text.
> > > - include the correct header.
> > > - change the property parsing name.
> > > - To use iio_chan_spec address field.
> > > - Refine each channel separate and shared_by_all.
> > > - Use pm_runtime and pm_runtime_autosuspend.
> > > - Remove the shutdown callback. From the HW suggestion, it's not recommended to
> > >   use battery as the power supply.
> > > - Check all scale unit (voltage->mV, current->mA, power->milliWatt).
> > > - Use the read_avail to provide the interface for attribute value list.
> > > - Add comma for the last element in the const integer array.
> > > - Refine each ADC label text.
> > > - In read_label callback, replace snprintf to sysfs_emit.
> > >
> > > ---
> > >  .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |   6 +
> > >  drivers/iio/adc/Kconfig                            |  15 +
> > >  drivers/iio/adc/Makefile                           |   1 +
> > >  drivers/iio/adc/rtq6056.c                          | 651 +++++++++++++++++++++
> > >  4 files changed, 673 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
> > >  create mode 100644 drivers/iio/adc/rtq6056.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..e89d15b
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
> > > @@ -0,0 +1,6 @@
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time
> > > +What:                /sys/bus/iio/devices/iio:deviceX/in_voltage1_integration_time
> > > +KernelVersion:       5.20
> > > +Contact:     cy_huang@...htek.com
> > > +Description:
> > > +             Each voltage conversion time in uS  
> >
> > Please move this entry to sysfs-bus-iio
> >
> > It's a natural extension of existing standard ABI so doesn't need to be in
> > a driver specific documentation file.
> >
> > However, way back in patch 1 I gave feedback on why we don't normally use integration time
> > for voltage channels and I thought you were changing this...
> >  
> I didn't intend to change this. Just cannot find any suitable
> attribute for this feature.
> From the IC interrnal, there's only one set of ADC.
> And the conversion order is bus/shunt......, average sample count to
> control the sample update interval.
> That' why the sample frequency is calculated by one second to divide
> [(bus_ct + shunt_ct) *  average sample bit] (us)
> 
> If it's not suitable for this attribute, I think it's better to change
> it as file attribute, not IIO channel attribute.
> 
> How do you think?

As mentioned in patch 1 discussion, we've done this before (IIRC) by defining per channel
sampling frequencies and not providing a general one.

We might want to consider improving the documentation in ABI/testing/sysfs-bus-iio
to make that clear however.

> > ...
> >  
> > > +static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
> > > +                                 struct iio_chan_spec const *ch,
> > > +                                 int *val)
> > > +{
> > > +     struct device *dev = priv->dev;
> > > +     unsigned int addr = ch->address;
> > > +     unsigned int regval;
> > > +     int ret;
> > > +
> > > +     pm_runtime_get_sync(dev);
> > > +     ret = regmap_read(priv->regmap, addr, &regval);
> > > +     pm_runtime_mark_last_busy(dev);
> > > +     pm_runtime_put(dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
> > > +     if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
> > > +             *val = regval;
> > > +     else
> > > +             *val = sign_extend32(regval, 16);
> > > +  
> >
> > One blank line only.
> >  
> > > +
> > > +     return IIO_VAL_INT;
> > > +}
> > > +  
> > ...
> >
> >  
> > > +
> > > +static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
> > > +                              struct iio_chan_spec const *chan, int val,
> > > +                              int val2, long mask)
> > > +{
> > > +     struct rtq6056_priv *priv = iio_priv(indio_dev);
> > > +
> > > +     if (iio_buffer_enabled(indio_dev))  
> >
> > This is racy as can enter buffered mode immediately after this check.
> > Use iio_device_claim_direct_mode() to avoid any races around this.
> >  
> for the shunt resistor attribute write, also?
> > > +             return -EBUSY;
> > > +
> > > +     switch (mask) {
> > > +     case IIO_CHAN_INFO_INT_TIME:
> > > +             return rtq6056_adc_set_conv_time(priv, chan, val);
> > > +     case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > > +             return rtq6056_adc_set_average(priv, val);
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}  
> >
> >  
> > > +
> > > +static void rtq6056_remove(void *dev)
> > > +{
> > > +     pm_runtime_dont_use_autosuspend(dev);
> > > +     pm_runtime_disable(dev);
> > > +     pm_runtime_set_suspended(dev);  
> >
> > There isn't anything here to push the device into a suspend state, so why
> > does calling pm_runtime_set_suspended() make sense?
> >  
> As I know, It is needed, at least 'pm_runtime_set_suspended' must be kept.
> 
> To think one case, adc is reading, module is removing.
> Who  will change the IC state to off?

That's not what set_suspended does.  We aren't telling the device to
'suspend' we are telling the runtime pm code that it already is.
If you want that to be the case, then you need to manually call whatever your
driver needs to do to suspend the device.

Note that if runtime pm is not configured into the kernel, everything should
still work. That is you should always power the device up in probe() and down
in remove().  That powerdown is needs to not use the runtime pm paths (as they
aren't being built in such a kernel!)

> 
> pm_runtime is already disabled, the IC will be kept in 'active', right?
> > > +}
> > > +
> > >
> > > +
> > > +static int rtq6056_probe(struct i2c_client *i2c)
> > > +{
> > > +     struct iio_dev *indio_dev;
> > > +     struct rtq6056_priv *priv;
> > > +     struct device *dev = &i2c->dev;
> > > +     struct regmap *regmap;
> > > +     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(dev, sizeof(*priv));
> > > +     if (!indio_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     priv = iio_priv(indio_dev);
> > > +     priv->dev = dev;
> > > +     priv->vshuntct_us = priv->vbusct_us = 1037;
> > > +     priv->avg_sample = 1;
> > > +     i2c_set_clientdata(i2c, priv);
> > > +
> > > +     regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
> > > +     if (IS_ERR(regmap))
> > > +             return dev_err_probe(dev, PTR_ERR(regmap),
> > > +                                  "Failed to init regmap\n");
> > > +
> > > +     priv->regmap = regmap;
> > > +
> > > +     ret = regmap_read(regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret,
> > > +                                  "Failed to get manufacturer info\n");
> > > +
> > > +     if (vendor_id != RTQ6056_VENDOR_ID)
> > > +             return dev_err_probe(dev, -ENODEV,
> > > +                                  "Invalid vendor id 0x%04x\n", vendor_id);
> > > +
> > > +     ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
> > > +                                        rtq6056_reg_fields, F_MAX_FIELDS);
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret, "Failed to init regmap field\n");
> > > +
> > > +     /*
> > > +      * By default, configure average sample as 1, bus and shunt conversion
> > > +      * timea as 1037 microsecond, and operating mode to all on.
> > > +      */
> > > +     ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret,
> > > +                                  "Failed to enable continuous sensing\n");
> > > +
> > > +     pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> > > +     pm_runtime_use_autosuspend(dev);
> > > +     pm_runtime_set_active(dev);
> > > +     pm_runtime_mark_last_busy(dev);
> > > +     pm_runtime_enable(dev);  
> >
> > Look at whether you can use devm_pm_runtime_enable()
> > Note it handles disabling autosuspend for you.
> >
> > When using runtime_pm() you want to ensure that the device works without
> > runtime pm support being enabled.  As such, you turn the device on before
> > enabling runtime_pm() and (this is missing I think) turn it off after disabling
> > runtime pm.  So I'd expect a devm_add_action_or_reset() call to unwind
> > setting the device into continuous sending above.
> >  
> If so, I think it's better to configure the device keep in off state
> in probe stage.

Only keep it in off state 'if' runtime pm is configured in.
Normally you need to power the device up in probe then
enable runtime pm to turn it off again (if runtime pm is supported).
If runtime pm isn't supported, we just leave the device powered up the whole
time until remove() when we power it down.

> The calling order may need to be changed as below
> devm_add_action_or_reset...
> 
> pm_runtime_set_autosuspend_delay
> pm_runtime_use_auto_suspend
> devm_pm_runtime_enable



> 
> > > +
> > > +     ret = devm_add_action_or_reset(dev, rtq6056_remove, dev);  
> >
> > The callback naming is too generic. It should give some indication
> > of what it is undoing (much of probe is handled by other devm_ callbacks).
> >  
> How about to change the name to 'rtq6056_enter_shutdown_state'?
> And in this function, to change the device state in shutdown with
> 'pm_runtime_set_suspended' API.

I think this reflects back to earlier misunderstanding of what
pm_runtime_set_suspended() actually does (assuming I have understood it
correctly).

> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /* By default, use 2000 micro-ohm resistor */
> > > +     shunt_resistor_uohm = 2000;
> > > +     device_property_read_u32(dev, "shunt-resistor-micro-ohms",
> > > +                              &shunt_resistor_uohm);
> > > +
> > > +     ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret,
> > > +                                  "Failed to init shunt resistor\n");
> > > +
> > > +     indio_dev->name = "rtq6056";
> > > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > > +     indio_dev->channels = rtq6056_channels;
> > > +     indio_dev->num_channels = ARRAY_SIZE(rtq6056_channels);
> > > +     indio_dev->info = &rtq6056_info;
> > > +
> > > +     ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > +                                           rtq6056_buffer_trigger_handler,
> > > +                                           NULL);
> > > +     if (ret)
> > > +             return dev_err_probe(dev, ret,
> > > +                                  "Failed to allocate iio trigger buffer\n");
> > > +
> > > +     return devm_iio_device_register(dev, indio_dev);
> > > +}  
> >  
> > > +
> > > +static const struct dev_pm_ops rtq6056_pm_ops = {
> > > +     RUNTIME_PM_OPS(rtq6056_runtime_suspend, rtq6056_runtime_resume, NULL)  
> >
> > Is there any reason we can't use these same ops to achieve at least some power
> > saving in suspend?  i.e. use DEFINE_RUNTIME_PM_OPS()  
>                                                  ~~~~~~~~~~~~~~~~~~~~~~~
>                                                  Where can I find this?

oops. DEFINE_RUNTIME_DEV_PM_OPS()
https://elixir.bootlin.com/linux/v5.19-rc6/source/include/linux/pm_runtime.h#L37

> >
> > I have tidying this up in existing drivers on my todo list as I think it is almost
> > always a good idea.  Note this is why there isn't a define to create the
> > particular combination you have here.
> >  
> If there's no combination like as that one, why  not unify it  to
> '_DEFINE_DEV_PM_OPS'?
> > > +};
> > > +  
> >  

Powered by blists - more mailing lists