Add Richtek RTQ6056 support

From: ChiYuan Huang <[email protected]>

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 <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

---
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

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +0 -0

new file mode 100644
index 00000000..e89d15b

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +12 -0

+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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +4 -0

+
+	return IIO_VAL_INT;
+}
+

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +9 -0

+		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;

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +50 -0

+}
+
 
+
+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.
+	 */

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +22 -0

+	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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+};
+

Quoted 9 lines · On Wed, 6 Jul 2022 22:11:42 +0800 cy_huang <[email protected]> wrote:

From: ChiYuan Huang <[email protected]>

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 <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Various feedback inline.

Thanks,

Jonathan

Quoted 51 lines

---
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

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +0 -0

new file mode 100644
index 00000000..e89d15b

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 6

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

@@ -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:     [email protected]
+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...

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +12 -0

+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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 4 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +4 -0

+
+     return IIO_VAL_INT;
+}
+

...

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 8 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +8 -0

+
+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.

Quoted 11 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +11 -0

+             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;
+     }
+}

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

+
+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?

Quoted 51 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +50 -0

+}
+
 
+
+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.
+      */

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+
+     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).

Quoted 28 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +28 -0

+     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);
+}

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 3 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +3 -0

+
+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.

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+};
+

Jonathan Cameron <[email protected]> 於 2022年7月8日 週五 凌晨1:20寫道：

Quoted 80 lines

Quoted 9 lines · On Wed, 6 Jul 2022 22:11:42 +0800 cy_huang <[email protected]> wrote:

From: ChiYuan Huang <[email protected]>

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 <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Various feedback inline.

Thanks,

Jonathan

Quoted 51 lines

---
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

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +0 -0

new file mode 100644
index 00000000..e89d15b

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 6

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

@@ -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:     [email protected]
+Description:
+             Each voltage conversion time in uS

Please move this entry to sysfs-bus-iio

Quoted 6 lines

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?

Quoted 34 lines

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +12 -0

+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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 4 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +4 -0

+
+     return IIO_VAL_INT;
+}
+

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 8 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +8 -0

+
+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?

Quoted 13 lines

Quoted 11 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +11 -0

+             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;
+     }
+}

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

+
+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?

pm_runtime is already disabled, the IC will be kept in 'active', right?

Quoted 65 lines

Quoted 51 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +50 -0

+}
+
 
+
+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.
+      */

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 6 lines

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.
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

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+
+     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.

Quoted 29 lines

Quoted 28 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +28 -0

+     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);
+}

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 3 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +3 -0

+
+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?

Quoted 5 lines

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'?

Quoted 3 lines

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+};
+

Quoted 74 lines

Quoted 63 lines

Quoted 49 lines

drivers/iio/adc/rtq6056.c new file +48 -0

+
+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.
+      */

drivers/iio/adc/rtq6056.c line 572

Quoted 10 lines

drivers/iio/adc/rtq6056.c new file +10 -0

+     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.

Quoted 6 lines

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.
The calling order may need to be changed as below devm_add_action_or_reset...

Quoted 4 lines

pm_runtime_set_autosuspend_delay
pm_runtime_use_auto_suspend
devm_pm_runtime_enable

Ah, not correct. How about if 'PM_RUNTIME' is not enabled? Do we need to consider about this case?

If yes, the original flow about 'pm_runtime' is correct.

Quoted 79 lines

Quoted 79 lines

Quoted 64 lines

Quoted 50 lines

drivers/iio/adc/rtq6056.c new file +48 -0

+
+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.
+      */

drivers/iio/adc/rtq6056.c line 572

Quoted 10 lines

drivers/iio/adc/rtq6056.c new file +10 -0

+     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.

Quoted 6 lines

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.
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

Quoted 4 lines

Ah, not correct. How about if 'PM_RUNTIME' is not enabled? Do we need to consider about this case?

If yes, the original flow about 'pm_runtime' is correct.

I don't follow. Perhaps next version will make it clear what you mean.

Jonathan

Jonathan Cameron <[email protected]> 於 2022年7月8日 週五 凌晨1:20寫道：

Quoted 80 lines

Quoted 9 lines · On Wed, 6 Jul 2022 22:11:42 +0800 cy_huang <[email protected]> wrote:

From: ChiYuan Huang <[email protected]>

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 <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Various feedback inline.

Thanks,

Jonathan

Quoted 51 lines

---
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

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +0 -0

new file mode 100644
index 00000000..e89d15b

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 6

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

@@ -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:     [email protected]
+Description:
+             Each voltage conversion time in uS  

Please move this entry to sysfs-bus-iio

Quoted 6 lines

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?

Quoted 34 lines

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +12 -0

+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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 4 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +4 -0

+
+     return IIO_VAL_INT;
+}
+  

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 8 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +8 -0

+
+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?

Quoted 13 lines

Quoted 11 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +11 -0

+             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;
+     }
+}  

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

+
+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?

pm_runtime is already disabled, the IC will be kept in 'active', right?

Quoted 65 lines

Quoted 51 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +50 -0

+}
+
 
+
+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.
+      */

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 6 lines

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.

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

Quoted 3 lines

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+
+     ret = devm_add_action_or_reset(dev, rtq6056_remove, dev);  

Quoted 3 lines

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.

Quoted 29 lines

Quoted 28 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +28 -0

+     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);
+}  

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 3 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +3 -0

+
+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?

Quoted 5 lines

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'?

Quoted 3 lines

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+};
+  

Quoted 95 lines · On Mon, 11 Jul 2022 10:48:17 +0800 ChiYuan Huang <[email protected]> wrote:

Jonathan Cameron <[email protected]> 於 2022年7月8日 週五 凌晨1:20寫道：

Quoted 80 lines

Quoted 9 lines · On Wed, 6 Jul 2022 22:11:42 +0800 cy_huang <[email protected]> wrote:

From: ChiYuan Huang <[email protected]>

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 <[email protected]>

Reviewed-by Andy Shevchenko <[email protected]>

Various feedback inline.

Thanks,

Jonathan

Quoted 51 lines

---
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

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +0 -0

new file mode 100644
index 00000000..e89d15b

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 line 6

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

@@ -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:     [email protected]
+Description:
+             Each voltage conversion time in uS

Please move this entry to sysfs-bus-iio

Quoted 6 lines

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)

Quoted 4 lines

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.

Quoted 70 lines

Quoted 34 lines

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +12 -0

+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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 4 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +4 -0

+
+     return IIO_VAL_INT;
+}
+

...

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 8 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +8 -0

+
+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?

Quoted 13 lines

Quoted 11 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +11 -0

+             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;
+     }
+}

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +6 -0

+
+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?

Quoted 4 lines

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!)

Quoted 67 lines

pm_runtime is already disabled, the IC will be kept in 'active', right?

Quoted 65 lines

Quoted 51 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +50 -0

+}
+
 
+
+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.
+      */

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +10 -0

+     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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 16 lines

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

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+
+     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).

Quoted 3 lines

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).

Quoted 35 lines

Quoted 35 lines

Quoted 22 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +22 -0

+     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);

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +9 -0

+     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()

Quoted 2 lines

~~~~~~~~~~~~~~~~~~~~~~~
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

Quoted 10 lines

Quoted 5 lines

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'?

Quoted 3 lines

Quoted 2 lines

Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056 new file +2 -0

+};
+