On Wed, 6 Jul 2022 22:11:42 +0800
cy_huang <u0084500@xxxxxxxxx> wrote:
> From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
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@xxxxxxxxxxx
> +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...
...
> +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, ®val);
> + 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.
> + 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?
> +}
> +
>
> +
> +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.
> +
> + 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).
> + 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()
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.
> +};
> +
