[PATCH 0/2] Add Richtek RTQ6056 support

[PATCH 0/2] Add Richtek RTQ6056 support

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

This patch series is to enable Richtek RTQ6056 support.

The RTQ6056 is a high accuracy current-sense monitor with I2C interface, and
the device provides full information for system by reading out the load current
and power.

ChiYuan Huang (2):
  dt-bindings: iio: adc: Add rtq6056 adc support
  iio: adc: Add rtq6056 support

 .../ABI/testing/sysfs-bus-iio-adc-rtq6056          |  58 +++
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      |  57 +++
 drivers/iio/adc/Kconfig                            |  15 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/rtq6056-adc.c                      | 548 +++++++++++++++++++++
 5 files changed, 679 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-rtq6056
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
 create mode 100644 drivers/iio/adc/rtq6056-adc.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add the documentation for Richtek RTQ6056.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
new file mode 100644
index 00000000..6b4e0e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+  interface, and the device provides full information for system by reading
+  out the loading current and power.
+
+  The device monitors both of the drops across sense resistor and the BUS
+  voltage, converts into the current in amperes, and power in watts through
+  internal analog-to-digital converter ADC. The programmable calibration,
+  adjustable conversion time, and averaging function are also built in for
+  more design flexibility.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
+
+properties:
+  compatible:
+    const: richtek,rtq6056
+
+  reg:
+    maxItems: 1
+
+  "#io-channel-cells":
+    const: 1
+
+  richtek,shunt-resistor-uohm:
+    description: Shunt IN+/IN- sensing node resistor in microohm.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      rtq6056@40 {
+        compatible = "richtek,rtq6056";
+        reg = <0x40>;
+        #io-channel-cells = <1>;
+      };
+    };
-- 
2.7.4

[PATCH 2/2] iio: adc: Add rtq6056 support

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.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@richtek.com>
---
 .../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
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		Shunt IN +/- sensing range from -82mV to +81.9175mV
+		Calculating with scale (2.5 uV)
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage1_raw
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		BUS voltage sensing range from 0V to 36V.
+		Calculating with scale (1.25 mV)
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_power2_raw
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		Power loading that equals to bus voltage multiple loading
+		current.
+		Calculating with scale (25 mWatt)
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_current3_raw
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		Consuming current from bus voltage.
+		Directly report loading current in mA
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		Shunt voltage conversion time in uS
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltage1_integration_time
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		BUS voltage conversion time in uS
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_oversampling_ratio
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		The number of average sample
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_sampling_frequency
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		Sampling frequency in HZ for power and current report
+
+What:		/sys/bus/iio/devices/iio:deviceX/integration_time_available
+KernelVersion:	5.18.2
+Contact:	cy_huang@richtek.com
+Description:
+		Sample conversion time available for BUS and Shunt, unit in second
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 48ace74..0b2d17c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -908,6 +908,21 @@ config ROCKCHIP_SARADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called rockchip_saradc.
 
+config RICHTEK_RTQ6056_ADC
+	tristate "Richtek RTQ6056 Current and Power Monitor ADC"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to enable RQT6056 ADC support.
+	  RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
+	  compatible interface, and the device provides full information for
+	  system by reading out the load current and power.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called rtq6056-adc.
+
 config RZG2L_ADC
 	tristate "Renesas RZ/G2L ADC driver"
 	depends on ARCH_RZG2L || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 39d806f..e8c6e6e 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
 obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
 obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_RICHTEK_RTQ6056_ADC) += rtq6056-adc.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
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 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/util_macros.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define RTQ6056_REG_CONFIG	0x00
+#define RTQ6056_REG_SHUNTVOLT	0x01
+#define RTQ6056_REG_CALIBRATION	0x05
+#define RTQ6056_REG_MASKENABLE	0x06
+#define RTQ6056_REG_ALERTLIMIT	0x07
+#define RTQ6056_REG_MANUFACTID	0xFE
+#define RTQ6056_REG_DIEID	0xFF
+
+#define RTQ6056_VENDOR_ID	0x1214
+#define RTQ6056_DEFAULT_CONFIG	0x4127
+#define RTQ6056_DEFAULT_CONVT	1037
+#define RTQ6056_DEFAULT_AVG	1
+#define RTQ6056_DEFAULT_RSHUNT	2000
+
+enum {
+	RTQ6056_CH_VSHUNT = 0,
+	RTQ6056_CH_VBUS,
+	RTQ6056_CH_POWER,
+	RTQ6056_CH_CURRENT,
+	RTQ6056_MAX_CHANNEL
+};
+
+enum {
+	F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
+	F_MAX_FIELDS
+};
+
+struct rtq6056_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_field *rm_fields[F_MAX_FIELDS];
+	u32 shunt_resistor_uohm;
+	int vshuntct; /* vshunt conversion time in uS */
+	int vbusct; /* vbus conversion time in uS */
+	int avg_sample;
+};
+
+static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
+	[F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
+	[F_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 5),
+	[F_VBUSCT] = REG_FIELD(RTQ6056_REG_CONFIG, 6, 8),
+	[F_AVG]	= REG_FIELD(RTQ6056_REG_CONFIG, 9, 11),
+	[F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15)
+};
+
+static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = RTQ6056_CH_VSHUNT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = RTQ6056_CH_VBUS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_POWER,
+		.indexed = 1,
+		.channel = RTQ6056_CH_POWER,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_CURRENT,
+		.indexed = 1,
+		.channel = RTQ6056_CH_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 3,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
+};
+
+static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, int channel,
+				    int *val)
+{
+	unsigned int reg = RTQ6056_REG_SHUNTVOLT + channel;
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(priv->regmap, reg, &regval);
+	if (ret)
+		return ret;
+
+	/* Only power and vbus channel is unsigned */
+	if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
+		*val = regval;
+	else
+		*val = (s16)regval;
+
+	return IIO_VAL_INT;
+}
+
+static int rtq6056_adc_read_scale(int channel, int *val, int *val2)
+{
+	switch (channel) {
+	case RTQ6056_CH_VSHUNT:
+		/* VSHUNT lsb  2.5uV */
+		*val = 2500;
+		*val2 = 1000;
+		return IIO_VAL_FRACTIONAL;
+	case RTQ6056_CH_VBUS:
+		/* VBUS lsb 1.25mV */
+		*val = 1250;
+		*val2 = 1000;
+		return IIO_VAL_FRACTIONAL;
+	case RTQ6056_CH_POWER:
+		/* Power lsb 25mV */
+		*val = 25;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+/* Conversion time in uS for channel VSHUNT and VBUS */
+static const int rtq6056_conv_time_list[] = {
+	139, 203, 269, 525, 1037, 2061, 4109, 8205
+};
+
+static int rtq6056_adc_set_conv_time(struct rtq6056_priv *priv, int channel,
+				     int val)
+{
+	struct regmap_field *rm_field;
+	unsigned int selector;
+	int *ct, ret;
+
+	if (val > 8205 || val < 139)
+		return -EINVAL;
+
+	if (channel == RTQ6056_CH_VSHUNT) {
+		rm_field = priv->rm_fields[F_VSHUNTCT];
+		ct = &priv->vshuntct;
+	} else {
+		rm_field = priv->rm_fields[F_VBUSCT];
+		ct = &priv->vbusct;
+	}
+
+	selector = find_closest(val, rtq6056_conv_time_list,
+				ARRAY_SIZE(rtq6056_conv_time_list));
+
+	ret = regmap_field_write(rm_field, selector);
+	if (ret)
+		return ret;
+
+	*ct = rtq6056_conv_time_list[selector];
+	return 0;
+}
+
+static const int rtq6056_avg_sample_list[] = {
+	1, 4, 16, 64, 128, 256, 512, 1024
+};
+
+static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
+{
+	unsigned int selector;
+	int ret;
+
+	if (val > 1024 || val < 1)
+		return -EINVAL;
+
+	selector = find_closest(val, rtq6056_avg_sample_list,
+				ARRAY_SIZE(rtq6056_avg_sample_list));
+
+	ret = regmap_field_write(priv->rm_fields[F_AVG], selector);
+	if (ret)
+		return ret;
+
+	priv->avg_sample = rtq6056_avg_sample_list[selector];
+	return 0;
+}
+
+static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, int *val)
+{
+	int sample_time;
+
+	sample_time = priv->vshuntct + priv->vbusct;
+	sample_time *= priv->avg_sample;
+
+	*val = DIV_ROUND_UP(1000000, sample_time);
+	return IIO_VAL_INT;
+}
+
+static int rtq6056_adc_read_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);
+	int channel = chan->channel;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return rtq6056_adc_read_channel(priv, channel, val);
+	case IIO_CHAN_INFO_SCALE:
+		return rtq6056_adc_read_scale(channel, val, val2);
+	case IIO_CHAN_INFO_INT_TIME:
+		if (chan->channel == RTQ6056_CH_VSHUNT)
+			*val = priv->vshuntct;
+		else
+			*val = priv->vbusct;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = priv->avg_sample;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return rtq6056_adc_get_sample_freq(priv, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+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);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return rtq6056_adc_set_conv_time(priv, chan->channel, val);
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return rtq6056_adc_set_average(priv, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
+	[RTQ6056_CH_VSHUNT] = "Vshunt(uV)",
+	[RTQ6056_CH_VBUS] = "Vbus(mV)",
+	[RTQ6056_CH_POWER] = "Power(mW)",
+	[RTQ6056_CH_CURRENT] = "Current(mA)",
+};
+
+static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  char *label)
+{
+	return snprintf(label, PAGE_SIZE, "%s\n",
+			rtq6056_channel_labels[chan->channel]);
+}
+
+static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
+				      int resistor_uohm)
+{
+	unsigned int calib_val;
+	int ret;
+
+	if (resistor_uohm <= 0) {
+		dev_err(priv->dev, "Invalid resistor [%d]\n", resistor_uohm);
+		return -EINVAL;
+	}
+
+	/* calibration = 5120000 / (Rshunt (uohm) * current lsb (1mA)) */
+	calib_val = 5120000 / resistor_uohm;
+	ret = regmap_write(priv->regmap, RTQ6056_REG_CALIBRATION, calib_val);
+	if (ret)
+		return ret;
+
+	priv->shunt_resistor_uohm = resistor_uohm;
+	return 0;
+}
+
+static ssize_t rtq6056_shunt_resistor_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct rtq6056_priv *priv = iio_priv(dev_to_iio_dev(dev));
+	int vals[2] = { priv->shunt_resistor_uohm, 1000000 };
+
+	return iio_format_value(buf, IIO_VAL_FRACTIONAL, 1, vals);
+}
+
+static ssize_t rtq6056_shunt_resistor_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t len)
+{
+	struct rtq6056_priv *priv = iio_priv(dev_to_iio_dev(dev));
+	int val, val_fract, ret;
+
+	ret = iio_str_to_fixpoint(buf, 100000, &val, &val_fract);
+	if (ret)
+		return ret;
+
+	ret = rtq6056_set_shunt_resistor(priv, val * 1000000 + val_fract);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static IIO_CONST_ATTR_NAMED(rtq6056_conv_time_available,
+			    integration_time_available,
+			    "0.000139 0.000203 0.000269 0.000525 0.001037 0.002061 0.004109 0.008205");
+
+static IIO_DEVICE_ATTR(shunt_resistor, 0644,
+		       rtq6056_shunt_resistor_show,
+		       rtq6056_shunt_resistor_store, 0);
+
+static struct attribute *rtq6056_attributes[] = {
+	&iio_const_attr_rtq6056_conv_time_available.dev_attr.attr,
+	&iio_dev_attr_shunt_resistor.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group rtq6056_attribute_group = {
+	.attrs = rtq6056_attributes,
+};
+
+static const struct iio_info rtq6056_info = {
+	.attrs = &rtq6056_attribute_group,
+	.read_raw = rtq6056_adc_read_raw,
+	.write_raw = rtq6056_adc_write_raw,
+	.read_label = rtq6056_adc_read_label,
+};
+
+static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct rtq6056_priv *priv = iio_priv(indio_dev);
+	struct {
+		u16 vals[RTQ6056_MAX_CHANNEL];
+		int64_t timestamp;
+	} data __aligned(8);
+	unsigned int raw;
+	int i = 0, bit, ret;
+
+	memset(&data, 0, sizeof(data));
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+
+		ret = regmap_read(priv->regmap, RTQ6056_REG_SHUNTVOLT + bit,
+				  &raw);
+		if (ret)
+			goto out;
+
+		data.vals[i++] = raw;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data, iio_get_time_ns(indio_dev));
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static bool rtq6056_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case RTQ6056_REG_CONFIG ... RTQ6056_REG_ALERTLIMIT:
+	case RTQ6056_REG_MANUFACTID ... RTQ6056_REG_DIEID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool rtq6056_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case RTQ6056_REG_CONFIG:
+	case RTQ6056_REG_CALIBRATION ... RTQ6056_REG_ALERTLIMIT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config rtq6056_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.max_register = RTQ6056_REG_DIEID,
+	.readable_reg = rtq6056_is_readable_reg,
+	.writeable_reg = rtq6056_is_writeable_reg,
+};
+
+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);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(&i2c->dev, PTR_ERR(priv->regmap),
+				     "Failed to init regmap\n");
+
+	ret = regmap_read(priv->regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to get manufacturer info\n");
+
+	if (vendor_id != RTQ6056_VENDOR_ID)
+		return dev_err_probe(&i2c->dev, -ENODEV,
+				     "invalid vendor id 0x%04x\n", vendor_id);
+
+	ret = devm_regmap_field_bulk_alloc(&i2c->dev, priv->regmap,
+					   priv->rm_fields, rtq6056_reg_fields,
+					   F_MAX_FIELDS);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to init regmap field\n");
+
+	/* Write the default config value */
+	ret = regmap_write(priv->regmap, RTQ6056_REG_CONFIG,
+			   RTQ6056_DEFAULT_CONFIG);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to write default config\n");
+
+	ret = of_property_read_u32(i2c->dev.of_node,
+				   "richtek,shunt-resistor-uohm",
+				   &shunt_resistor_uohm);
+	if (ret)
+		shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;
+
+	ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
+	if (ret)
+		dev_err_probe(&i2c->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(&i2c->dev, indio_dev, NULL,
+					      rtq6056_buffer_trigger_handler,
+					      NULL);
+	if (ret)
+		return dev_err_probe(&i2c->dev, ret,
+				     "Failed to allocate iio trigger buffer\n");
+
+	return devm_iio_device_register(&i2c->dev, indio_dev);
+}
+
+static int rtq6056_remove(struct i2c_client *i2c)
+{
+	struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
+
+	/* Config opmode to 'shutdown' mode to minimize quiescient current */
+	return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
+}
+
+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);
+}
+
+static const struct of_device_id rtq6056_device_match[] = {
+	{ .compatible = "richtek,rtq6056", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rtq6056_device_match);
+
+static struct i2c_driver rtq6056_driver = {
+	.driver = {
+		.name = "rtq6056",
+		.of_match_table = rtq6056_device_match,
+	},
+	.probe_new = rtq6056_probe,
+	.remove = rtq6056_remove,
+	.shutdown = rtq6056_shutdown,
+};
+module_i2c_driver(rtq6056_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_DESCRIPTION("Richtek RTQ6056 ADC Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[Andy Shevchenko]

On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote:
>
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> Add Richtek RTQ6056 supporting.
>
> It can be used for the system to monitor load current and power with 16bit

16-bit

> resolution.

Overall looks good, needs some cosmetic work.

...

> +KernelVersion: 5.18.2

Wrong version, this won't be part of a stable kernel.

...

> +#include <linux/of.h>

Any users of this?

But for sure you missed

  mod_devicetable.h
  types.h

...

> +#define RTQ6056_DEFAULT_RSHUNT 2000

_mOHMs ?

...

> +enum {
> +       F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
> +       F_MAX_FIELDS

Hard to read this way. Split to be one emum entry per line.

> +};

...

> +struct rtq6056_priv {
> +       struct device *dev;
> +       struct regmap *regmap;

Swapping these two might give less code in the generated binary. Have
you run bloat-o-meter?

> +       struct regmap_field *rm_fields[F_MAX_FIELDS];
> +       u32 shunt_resistor_uohm;
> +       int vshuntct; /* vshunt conversion time in uS */
> +       int vbusct; /* vbus conversion time in uS */
> +       int avg_sample;
> +};

...

> +       IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)

Keep a comma.

...


> +       /* Only power and vbus channel is unsigned */
> +       if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
> +               *val = regval;
> +       else

> +               *val = (s16)regval;

Why casting? At very minimum this requires a comment.

...

> +       if (val > 8205 || val < 139)
> +               return -EINVAL;

This strange range requires a good comment with possible references to
the datasheet.

...

> +static const int rtq6056_avg_sample_list[] = {
> +       1, 4, 16, 64, 128, 256, 512, 1024

Keep a comma at the end.

> +};

...

> +static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
> +                                 struct iio_chan_spec const *chan,
> +                                 char *label)
> +{
> +       return snprintf(label, PAGE_SIZE, "%s\n",
> +                       rtq6056_channel_labels[chan->channel]);

sysfs_emit()

> +}

...

> +static IIO_DEVICE_ATTR(shunt_resistor, 0644,
> +                      rtq6056_shunt_resistor_show,
> +                      rtq6056_shunt_resistor_store, 0);

IIO_DEVICE_ATTR_RW()

...

> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> +                        indio_dev->masklength) {

On one line it's better.

> +

Redundant blank line.

> +               ret = regmap_read(priv->regmap, RTQ6056_REG_SHUNTVOLT + bit,
> +                                 &raw);
> +               if (ret)
> +                       goto out;
> +
> +               data.vals[i++] = raw;
> +       }

> +       ret = of_property_read_u32(i2c->dev.of_node,
> +                                  "richtek,shunt-resistor-uohm",
> +                                  &shunt_resistor_uohm);

device_property_read()

> +       if (ret)
> +               shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;

Can be done without branch

... = DEFAULT;
device_property_read_u32(...); // no error checking.

...

> +static int rtq6056_remove(struct i2c_client *i2c)
> +{
> +       struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> +
> +       /* Config opmode to 'shutdown' mode to minimize quiescient current */

quiescent

> +       return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> +}
> +
> +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 */

quiescent

> +       regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[Jonathan Cameron]

On Fri, 17 Jun 2022 17:32:55 +0800
cy_huang <u0084500@gmail.com> wrote:

> From: ChiYuan Huang <cy_huang@richtek.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@richtek.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.

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


> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		Shunt IN +/- sensing range from -82mV to +81.9175mV
> +		Calculating with scale (2.5 uV)
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage1_raw
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		BUS voltage sensing range from 0V to 36V.
> +		Calculating with scale (1.25 mV)
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_power2_raw
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		Power loading that equals to bus voltage multiple loading
> +		current.
> +		Calculating with scale (25 mWatt)
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_current3_raw
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		Consuming current from bus voltage.
> +		Directly report loading current in mA
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		Shunt voltage conversion time in uS
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_voltage1_integration_time
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		BUS voltage conversion time in uS

integration time is not normally about conversion time. It's normally for things
like light sensors where they are charging up a capacitor for a fixed time period.

For ADC channels we tend to use _sampling_frequency (so 1/period). An example of
this being per channel is ad7124.

That does mean we tend not to provide the overall 'sampling_frequency' on these
devices, but rather let an interested userspace program figure it out - often
it depends on which channels are enabled.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_oversampling_ratio
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		The number of average sample
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_sampling_frequency
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		Sampling frequency in HZ for power and current report
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/integration_time_available
> +KernelVersion:	5.18.2
> +Contact:	cy_huang@richtek.com
> +Description:
> +		Sample conversion time available for BUS and Shunt, unit in second
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 48ace74..0b2d17c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -908,6 +908,21 @@ config ROCKCHIP_SARADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rockchip_saradc.
>  
> +config RICHTEK_RTQ6056_ADC
> +	tristate "Richtek RTQ6056 Current and Power Monitor ADC"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to enable RQT6056 ADC support.
> +	  RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
> +	  compatible interface, and the device provides full information for
> +	  system by reading out the load current and power.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called rtq6056-adc.
> +
>  config RZG2L_ADC
>  	tristate "Renesas RZ/G2L ADC driver"
>  	depends on ARCH_RZG2L || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 39d806f..e8c6e6e 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
>  obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
>  obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_RICHTEK_RTQ6056_ADC) += rtq6056-adc.o

Is the device anything other than ADC?  If not, drop the -adc.
The other entries you see here are ADCs that form part of a much
larger SoC.

>  obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
>  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>  obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
> 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 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

property.h and mod_devicetable.h instead of of.h
once you have moved to generic firmware properties.

> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define RTQ6056_REG_CONFIG	0x00
> +#define RTQ6056_REG_SHUNTVOLT	0x01
> +#define RTQ6056_REG_CALIBRATION	0x05
> +#define RTQ6056_REG_MASKENABLE	0x06
> +#define RTQ6056_REG_ALERTLIMIT	0x07
> +#define RTQ6056_REG_MANUFACTID	0xFE
> +#define RTQ6056_REG_DIEID	0xFF
> +
> +#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.

> +#define RTQ6056_DEFAULT_CONVT	1037
> +#define RTQ6056_DEFAULT_AVG	1
This and the next are real world values. Having them as defines
makes the code less readable.

> +#define RTQ6056_DEFAULT_RSHUNT	2000
> +
> +enum {
> +	RTQ6056_CH_VSHUNT = 0,
> +	RTQ6056_CH_VBUS,
> +	RTQ6056_CH_POWER,
> +	RTQ6056_CH_CURRENT,
> +	RTQ6056_MAX_CHANNEL
> +};
> +
> +enum {
> +	F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
> +	F_MAX_FIELDS
> +};
> +
> +struct rtq6056_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct regmap_field *rm_fields[F_MAX_FIELDS];
> +	u32 shunt_resistor_uohm;
> +	int vshuntct; /* vshunt conversion time in uS */
> +	int vbusct; /* vbus conversion time in uS */
> +	int avg_sample;
> +};
> +
> +static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
> +	[F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
> +	[F_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 5),
> +	[F_VBUSCT] = REG_FIELD(RTQ6056_REG_CONFIG, 6, 8),
> +	[F_AVG]	= REG_FIELD(RTQ6056_REG_CONFIG, 9, 11),
> +	[F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15)
> +};
> +
> +static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.channel = RTQ6056_CH_VSHUNT,
Where we have a bunch of different types of channel we generally
count the channel numbers separately.  Where this is only one of a give
type, don't bother making it indexed.

Where you need an offset to use for accessing an actual register,
the .address field is more appropriate.

> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),

Why _by_dir as opposed to by_all given there are no output channels registered?


> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.channel = RTQ6056_CH_VBUS,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_POWER,
> +		.indexed = 1,
> +		.channel = RTQ6056_CH_POWER,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +		.scan_index = 2,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_CURRENT,
> +		.indexed = 1,
> +		.channel = RTQ6056_CH_CURRENT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +					   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> +		.scan_index = 3,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
> +};
> +
> +static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, int channel,
> +				    int *val)
> +{
> +	unsigned int reg = RTQ6056_REG_SHUNTVOLT + channel;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(priv->regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* Only power and vbus channel is unsigned */
> +	if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
> +		*val = regval;
> +	else
> +		*val = (s16)regval;

Prefer making it clear this is sign extension with sign_extend32()

> +
> +	return IIO_VAL_INT;
> +}
> +

...

> +
> +static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
> +{
> +	unsigned int selector;
> +	int ret;
> +
> +	if (val > 1024 || val < 1)
> +		return -EINVAL;
> +
> +	selector = find_closest(val, rtq6056_avg_sample_list,
> +				ARRAY_SIZE(rtq6056_avg_sample_list));
> +
> +	ret = regmap_field_write(priv->rm_fields[F_AVG], selector);
> +	if (ret)
> +		return ret;
> +
> +	priv->avg_sample = rtq6056_avg_sample_list[selector];

Blank line - see below. Same for other similar cases.

> +	return 0;
> +}
> +
> +static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, int *val)
> +{
> +	int sample_time;
> +
> +	sample_time = priv->vshuntct + priv->vbusct;
> +	sample_time *= priv->avg_sample;

> +
> +	*val = DIV_ROUND_UP(1000000, sample_time);
> +	return IIO_VAL_INT;
> +}
> +


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

> +	[RTQ6056_CH_VBUS] = "Vbus(mV)",
> +	[RTQ6056_CH_POWER] = "Power(mW)",
> +	[RTQ6056_CH_CURRENT] = "Current(mA)",
> +};
> +
> +static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  char *label)
> +{
> +	return snprintf(label, PAGE_SIZE, "%s\n",
> +			rtq6056_channel_labels[chan->channel]);
> +}
> +
> +static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
> +				      int resistor_uohm)
> +{
> +	unsigned int calib_val;
> +	int ret;
> +
> +	if (resistor_uohm <= 0) {
> +		dev_err(priv->dev, "Invalid resistor [%d]\n", resistor_uohm);
> +		return -EINVAL;
> +	}
> +
> +	/* calibration = 5120000 / (Rshunt (uohm) * current lsb (1mA)) */
> +	calib_val = 5120000 / resistor_uohm;
> +	ret = regmap_write(priv->regmap, RTQ6056_REG_CALIBRATION, calib_val);
> +	if (ret)
> +		return ret;
> +
> +	priv->shunt_resistor_uohm = resistor_uohm;

Blank line before a simple return like this. It slightly helps readability.

> +	return 0;
> +}

...

> +static IIO_CONST_ATTR_NAMED(rtq6056_conv_time_available,
> +			    integration_time_available,
> +			    "0.000139 0.000203 0.000269 0.000525 0.001037 0.002061 0.004109 0.008205");
> +
> +static IIO_DEVICE_ATTR(shunt_resistor, 0644,
> +		       rtq6056_shunt_resistor_show,
> +		       rtq6056_shunt_resistor_store, 0);
> +
> +static struct attribute *rtq6056_attributes[] = {
> +	&iio_const_attr_rtq6056_conv_time_available.dev_attr.attr,
Whilst I think this particular interface will change anyway, you should
use the read_avail callback for available attributes for standard ABI.

> +	&iio_dev_attr_shunt_resistor.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group rtq6056_attribute_group = {
> +	.attrs = rtq6056_attributes,
> +};
> +
> +static const struct iio_info rtq6056_info = {
> +	.attrs = &rtq6056_attribute_group,
> +	.read_raw = rtq6056_adc_read_raw,
> +	.write_raw = rtq6056_adc_write_raw,
> +	.read_label = rtq6056_adc_read_label,
> +};

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

Similar for dev. once a function has multiple accesses to i2c->dev,
things are cleaner if you just have

struct device *dev = &i2c->dev; and use dev after that.

> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(priv->regmap),
> +				     "Failed to init regmap\n");
> +
> +	ret = regmap_read(priv->regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to get manufacturer info\n");
> +
> +	if (vendor_id != RTQ6056_VENDOR_ID)
> +		return dev_err_probe(&i2c->dev, -ENODEV,
> +				     "invalid vendor id 0x%04x\n", vendor_id);
> +
> +	ret = devm_regmap_field_bulk_alloc(&i2c->dev, priv->regmap,
> +					   priv->rm_fields, rtq6056_reg_fields,
> +					   F_MAX_FIELDS);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to init regmap field\n");
> +
> +	/* Write the default config value */
> +	ret = regmap_write(priv->regmap, RTQ6056_REG_CONFIG,
> +			   RTQ6056_DEFAULT_CONFIG);

This write effectively turns the device on so it would be good
to register a devm_add_action_or_reset() callback here to do what
you currently have in remove.  That will mean you don't need a separate
remove at all and that you will put the device in a low power state
if you get an error after this point.

At some stage it would be nice to do runtime pm with autosuspend so that
we enter a low power state if no one is looking at the readings.

> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to write default config\n");
> +
> +	ret = of_property_read_u32(i2c->dev.of_node,

Please use generic firmware properties not device tree specific ones.
That lets people use other firmware interfaces such as ACPI using the
PRP0001 HID.

> +				   "richtek,shunt-resistor-uohm",
> +				   &shunt_resistor_uohm);
> +	if (ret)
> +		shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;
Flip this around

	shunt_resistor_uohm = 2000;
	device_property_read_u32(dev, "rictek,shunt-resistor-uohm",
				 &shunt_resistor_uohm);

if the property read fails, it won't change the value stored.

> +
> +	ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
> +	if (ret)
> +		dev_err_probe(&i2c->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(&i2c->dev, indio_dev, NULL,
> +					      rtq6056_buffer_trigger_handler,
> +					      NULL);
> +	if (ret)
> +		return dev_err_probe(&i2c->dev, ret,
> +				     "Failed to allocate iio trigger buffer\n");
> +
> +	return devm_iio_device_register(&i2c->dev, indio_dev);
> +}
> +
> +static int rtq6056_remove(struct i2c_client *i2c)
> +{
> +	struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> +
> +	/* Config opmode to 'shutdown' mode to minimize quiescient current */
> +	return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> +}
> +
> +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.

> +}
> +
> +static const struct of_device_id rtq6056_device_match[] = {
> +	{ .compatible = "richtek,rtq6056", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rtq6056_device_match);
> +
> +static struct i2c_driver rtq6056_driver = {
> +	.driver = {
> +		.name = "rtq6056",
> +		.of_match_table = rtq6056_device_match,
> +	},
> +	.probe_new = rtq6056_probe,
> +	.remove = rtq6056_remove,
> +	.shutdown = rtq6056_shutdown,
> +};
> +module_i2c_driver(rtq6056_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_DESCRIPTION("Richtek RTQ6056 ADC Driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[Krzysztof Kozlowski]

On 17/06/2022 02:32, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add the documentation for Richtek RTQ6056.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> new file mode 100644
> index 00000000..6b4e0e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
> +
> +maintainers:
> +  - ChiYuan Huang <cy_huang@richtek.com>
> +
> +description: |
> +  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
> +  interface, and the device provides full information for system by reading
> +  out the loading current and power.
> +
> +  The device monitors both of the drops across sense resistor and the BUS
> +  voltage, converts into the current in amperes, and power in watts through
> +  internal analog-to-digital converter ADC. The programmable calibration,
> +  adjustable conversion time, and averaging function are also built in for
> +  more design flexibility.
> +
> +  Datasheet is available at
> +  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
> +
> +properties:
> +  compatible:
> +    const: richtek,rtq6056
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  richtek,shunt-resistor-uohm:

Use standard properties, so "-micro-ohms". Drop the unit from
description and drop the ref.

> +    description: Shunt IN+/IN- sensing node resistor in microohm.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#io-channel-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      rtq6056@40 {

Generic node name, so probably "adc" (or something else appropriate).

> +        compatible = "richtek,rtq6056";
> +        reg = <0x40>;
> +        #io-channel-cells = <1>;
> +      };
> +    };


Best regards,
Krzysztof

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[ChiYuan Huang]

Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月18日 週六 凌晨1:08寫道：
>
> On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote:
> >
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add Richtek RTQ6056 supporting.
> >
> > It can be used for the system to monitor load current and power with 16bit
>
> 16-bit
>
Ack in next.
> > resolution.
>
> Overall looks good, needs some cosmetic work.
>
> ...
>
> > +KernelVersion: 5.18.2
>
> Wrong version, this won't be part of a stable kernel.
>
From kernel.org, currently the stable kernel version is 5.18.5.
Change to 5.18.5?
> ...
>
> > +#include <linux/of.h>
>
> Any users of this?
>
function 'of_property_read_u32'.
But from Jonathan's reply, I may change it to 'device_property_read_u32'.
And also property.h will be included.
> But for sure you missed
>
>   mod_devicetable.h
>   types.h
>
Ack in next. But for types.h, i2c.h already include device.h.
And device.h already include types.h.
Is it still needed to declare explicitly for types.h??
> ...
>
> > +#define RTQ6056_DEFAULT_RSHUNT 2000
>
> _mOHMs ?
>
From Jonathan's reply, I may remove this value defined.
Since it's already a straight value. To define it, seems to decrease
the readability.
> ...
>
> > +enum {
> > +       F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
> > +       F_MAX_FIELDS
>
> Hard to read this way. Split to be one emum entry per line.
>
Ack in next.
> > +};
>
> ...
>
> > +struct rtq6056_priv {
> > +       struct device *dev;
> > +       struct regmap *regmap;
>
> Swapping these two might give less code in the generated binary. Have
> you run bloat-o-meter?
>
I never know about this tool.
I'll check it before I submit the next revision.
Thanks for the reminding.

But from Jonathan's reply, I may remove 'struct regmap *regmap'.
If all function need the 'regmap', a local variable 'regmap' need to
be declared.
To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.
> > +       struct regmap_field *rm_fields[F_MAX_FIELDS];
> > +       u32 shunt_resistor_uohm;
> > +       int vshuntct; /* vshunt conversion time in uS */
> > +       int vbusct; /* vbus conversion time in uS */
> > +       int avg_sample;
> > +};
>
> ...
>
> > +       IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
>
> Keep a comma.
>
Ack in next
> ...
>
>
> > +       /* Only power and vbus channel is unsigned */
> > +       if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
> > +               *val = regval;
> > +       else
>
> > +               *val = (s16)regval;
>
> Why casting? At very minimum this requires a comment.
The value is already the 16-bit 2's complement value. That's why the
casting is here.
From Jonathan's reply, will replace it by sign_extend32.
>
> ...
>
> > +       if (val > 8205 || val < 139)
> > +               return -EINVAL;
>
> This strange range requires a good comment with possible references to
> the datasheet.
>
Ack in next.
> ...
>
> > +static const int rtq6056_avg_sample_list[] = {
> > +       1, 4, 16, 64, 128, 256, 512, 1024
>
> Keep a comma at the end.
>
> > +};
>
> ...
>
> > +static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
> > +                                 struct iio_chan_spec const *chan,
> > +                                 char *label)
> > +{
> > +       return snprintf(label, PAGE_SIZE, "%s\n",
> > +                       rtq6056_channel_labels[chan->channel]);
>
> sysfs_emit()
>
> > +}
>
> ...
>
> > +static IIO_DEVICE_ATTR(shunt_resistor, 0644,
> > +                      rtq6056_shunt_resistor_show,
> > +                      rtq6056_shunt_resistor_store, 0);
>
> IIO_DEVICE_ATTR_RW()
>
> ...
>
> > +       for_each_set_bit(bit, indio_dev->active_scan_mask,
> > +                        indio_dev->masklength) {
>
> On one line it's better.
>
Ack in next
> > +
>
> Redundant blank line.
>
Ack in next.
> > +               ret = regmap_read(priv->regmap, RTQ6056_REG_SHUNTVOLT + bit,
> > +                                 &raw);
> > +               if (ret)
> > +                       goto out;
> > +
> > +               data.vals[i++] = raw;
> > +       }
>
> > +       ret = of_property_read_u32(i2c->dev.of_node,
> > +                                  "richtek,shunt-resistor-uohm",
> > +                                  &shunt_resistor_uohm);
>
> device_property_read()
>
From you and other's reply, I may refine this part about the resistor parsing.
> > +       if (ret)
> > +               shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;
>
> Can be done without branch
>
OK
> ... = DEFAULT;
> device_property_read_u32(...); // no error checking.
>
> ...
>
> > +static int rtq6056_remove(struct i2c_client *i2c)
> > +{
> > +       struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> > +
> > +       /* Config opmode to 'shutdown' mode to minimize quiescient current */
>
> quiescent
>
Sorry for the typo
> > +       return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> > +}
> > +
> > +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 */
>
> quiescent
>
Sorry for the typo
> > +       regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[ChiYuan Huang]

Jonathan Cameron <Jonathan.Cameron@huawei.com> 於 2022年6月18日 週六 凌晨1:10寫道：
>
> On Fri, 17 Jun 2022 17:32:55 +0800
> cy_huang <u0084500@gmail.com> wrote:
>
> > From: ChiYuan Huang <cy_huang@richtek.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@richtek.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.

> > ---
> >  .../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?
>
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             Shunt IN +/- sensing range from -82mV to +81.9175mV
> > +             Calculating with scale (2.5 uV)
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_voltage1_raw
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             BUS voltage sensing range from 0V to 36V.
> > +             Calculating with scale (1.25 mV)
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_power2_raw
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             Power loading that equals to bus voltage multiple loading
> > +             current.
> > +             Calculating with scale (25 mWatt)
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_current3_raw
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             Consuming current from bus voltage.
> > +             Directly report loading current in mA
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_voltage0_integration_time
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             Shunt voltage conversion time in uS
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_voltage1_integration_time
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             BUS voltage conversion time in uS
>
> integration time is not normally about conversion time. It's normally for things
> like light sensors where they are charging up a capacitor for a fixed time period.
>
> For ADC channels we tend to use _sampling_frequency (so 1/period). An example of
> this being per channel is ad7124.
>
> That does mean we tend not to provide the overall 'sampling_frequency' on these
> devices, but rather let an interested userspace program figure it out - often
> it depends on which channels are enabled.
>
Ack in next
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_oversampling_ratio
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             The number of average sample
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_sampling_frequency
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             Sampling frequency in HZ for power and current report
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/integration_time_available
> > +KernelVersion:       5.18.2
> > +Contact:     cy_huang@richtek.com
> > +Description:
> > +             Sample conversion time available for BUS and Shunt, unit in second
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 48ace74..0b2d17c 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -908,6 +908,21 @@ config ROCKCHIP_SARADC
> >         To compile this driver as a module, choose M here: the
> >         module will be called rockchip_saradc.
> >
> > +config RICHTEK_RTQ6056_ADC
> > +     tristate "Richtek RTQ6056 Current and Power Monitor ADC"
> > +     depends on I2C
> > +     select REGMAP_I2C
> > +     select IIO_BUFFER
> > +     select IIO_TRIGGERED_BUFFER
> > +     help
> > +       Say yes here to enable RQT6056 ADC support.
> > +       RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
> > +       compatible interface, and the device provides full information for
> > +       system by reading out the load current and power.
> > +
> > +       This driver can also be built as a module. If so, the module will be
> > +       called rtq6056-adc.
> > +
> >  config RZG2L_ADC
> >       tristate "Renesas RZ/G2L ADC driver"
> >       depends on ARCH_RZG2L || COMPILE_TEST
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 39d806f..e8c6e6e 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -84,6 +84,7 @@ obj-$(CONFIG_QCOM_PM8XXX_XOADC) += qcom-pm8xxx-xoadc.o
> >  obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
> >  obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
> >  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> > +obj-$(CONFIG_RICHTEK_RTQ6056_ADC) += rtq6056-adc.o
>
> Is the device anything other than ADC?  If not, drop the -adc.
> The other entries you see here are ADCs that form part of a much
> larger SoC.
>
No, like you said, only ADC. To rename it is OK.
> >  obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
> >  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
> >  obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
> > 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 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
>
> property.h and mod_devicetable.h instead of of.h
> once you have moved to generic firmware properties.
>
Ack  in next.
> > +#include <linux/regmap.h>
> > +#include <linux/util_macros.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> > +#define RTQ6056_REG_CONFIG   0x00
> > +#define RTQ6056_REG_SHUNTVOLT        0x01
> > +#define RTQ6056_REG_CALIBRATION      0x05
> > +#define RTQ6056_REG_MASKENABLE       0x06
> > +#define RTQ6056_REG_ALERTLIMIT       0x07
> > +#define RTQ6056_REG_MANUFACTID       0xFE
> > +#define RTQ6056_REG_DIEID    0xFF
> > +
> > +#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?
> > +#define RTQ6056_DEFAULT_CONVT        1037
> > +#define RTQ6056_DEFAULT_AVG  1
> This and the next are real world values. Having them as defines
> makes the code less readable.
>
Will remove it. Thx.
> > +#define RTQ6056_DEFAULT_RSHUNT       2000
> > +
> > +enum {
> > +     RTQ6056_CH_VSHUNT = 0,
> > +     RTQ6056_CH_VBUS,
> > +     RTQ6056_CH_POWER,
> > +     RTQ6056_CH_CURRENT,
> > +     RTQ6056_MAX_CHANNEL
> > +};
> > +
> > +enum {
> > +     F_OPMODE = 0, F_VSHUNTCT, F_VBUSCT, F_AVG, F_RESET,
> > +     F_MAX_FIELDS
> > +};
> > +
> > +struct rtq6056_priv {
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     struct regmap_field *rm_fields[F_MAX_FIELDS];
> > +     u32 shunt_resistor_uohm;
> > +     int vshuntct; /* vshunt conversion time in uS */
> > +     int vbusct; /* vbus conversion time in uS */
> > +     int avg_sample;
> > +};
> > +
> > +static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
> > +     [F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
> > +     [F_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 5),
> > +     [F_VBUSCT] = REG_FIELD(RTQ6056_REG_CONFIG, 6, 8),
> > +     [F_AVG] = REG_FIELD(RTQ6056_REG_CONFIG, 9, 11),
> > +     [F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15)
> > +};
> > +
> > +static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
> > +     {
> > +             .type = IIO_VOLTAGE,
> > +             .indexed = 1,
> > +             .channel = RTQ6056_CH_VSHUNT,
> Where we have a bunch of different types of channel we generally
> count the channel numbers separately.  Where this is only one of a give
> type, don't bother making it indexed.
>
> Where you need an offset to use for accessing an actual register,
> the .address field is more appropriate.
>
Ack in next.
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                   BIT(IIO_CHAN_INFO_SCALE) |
> > +                                   BIT(IIO_CHAN_INFO_INT_TIME),
> > +             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
>
> Why _by_dir as opposed to by_all given there are no output channels registered?
>
Thanks, I think I need to check all channel attributes again.
>
> > +             .scan_index = 0,
> > +             .scan_type = {
> > +                     .sign = 's',
> > +                     .realbits = 16,
> > +                     .storagebits = 16,
> > +                     .endianness = IIO_CPU,
> > +             },
> > +     },
> > +     {
> > +             .type = IIO_VOLTAGE,
> > +             .indexed = 1,
> > +             .channel = RTQ6056_CH_VBUS,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                   BIT(IIO_CHAN_INFO_SCALE) |
> > +                                   BIT(IIO_CHAN_INFO_INT_TIME),
> > +             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> > +             .scan_index = 1,
> > +             .scan_type = {
> > +                     .sign = 'u',
> > +                     .realbits = 16,
> > +                     .storagebits = 16,
> > +                     .endianness = IIO_CPU,
> > +             },
> > +     },
> > +     {
> > +             .type = IIO_POWER,
> > +             .indexed = 1,
> > +             .channel = RTQ6056_CH_POWER,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                   BIT(IIO_CHAN_INFO_SCALE),
> > +             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> > +             .scan_index = 2,
> > +             .scan_type = {
> > +                     .sign = 'u',
> > +                     .realbits = 16,
> > +                     .storagebits = 16,
> > +                     .endianness = IIO_CPU,
> > +             },
> > +     },
> > +     {
> > +             .type = IIO_CURRENT,
> > +             .indexed = 1,
> > +             .channel = RTQ6056_CH_CURRENT,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +             .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +                                        BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> > +             .scan_index = 3,
> > +             .scan_type = {
> > +                     .sign = 's',
> > +                     .realbits = 16,
> > +                     .storagebits = 16,
> > +                     .endianness = IIO_CPU,
> > +             },
> > +     },
> > +     IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL)
> > +};
> > +
> > +static int rtq6056_adc_read_channel(struct rtq6056_priv *priv, int channel,
> > +                                 int *val)
> > +{
> > +     unsigned int reg = RTQ6056_REG_SHUNTVOLT + channel;
> > +     unsigned int regval;
> > +     int ret;
> > +
> > +     ret = regmap_read(priv->regmap, reg, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Only power and vbus channel is unsigned */
> > +     if (channel == RTQ6056_CH_VBUS || channel == RTQ6056_CH_POWER)
> > +             *val = regval;
> > +     else
> > +             *val = (s16)regval;
>
> Prefer making it clear this is sign extension with sign_extend32()
>
Ack in next.
> > +
> > +     return IIO_VAL_INT;
> > +}
> > +
>
> ...
>
> > +
> > +static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
> > +{
> > +     unsigned int selector;
> > +     int ret;
> > +
> > +     if (val > 1024 || val < 1)
> > +             return -EINVAL;
> > +
> > +     selector = find_closest(val, rtq6056_avg_sample_list,
> > +                             ARRAY_SIZE(rtq6056_avg_sample_list));
> > +
> > +     ret = regmap_field_write(priv->rm_fields[F_AVG], selector);
> > +     if (ret)
> > +             return ret;
> > +
> > +     priv->avg_sample = rtq6056_avg_sample_list[selector];
>
> Blank line - see below. Same for other similar cases.
>
Ack in next.
> > +     return 0;
> > +}
> > +
> > +static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv, int *val)
> > +{
> > +     int sample_time;
> > +
> > +     sample_time = priv->vshuntct + priv->vbusct;
> > +     sample_time *= priv->avg_sample;
>
> > +
> > +     *val = DIV_ROUND_UP(1000000, sample_time);
> > +     return IIO_VAL_INT;
> > +}
> > +
>
>
> > +
> > +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?
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_adc_read_label(struct iio_dev *indio_dev,
> > +                               struct iio_chan_spec const *chan,
> > +                               char *label)
> > +{
> > +     return snprintf(label, PAGE_SIZE, "%s\n",
> > +                     rtq6056_channel_labels[chan->channel]);
> > +}
> > +
> > +static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
> > +                                   int resistor_uohm)
> > +{
> > +     unsigned int calib_val;
> > +     int ret;
> > +
> > +     if (resistor_uohm <= 0) {
> > +             dev_err(priv->dev, "Invalid resistor [%d]\n", resistor_uohm);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* calibration = 5120000 / (Rshunt (uohm) * current lsb (1mA)) */
> > +     calib_val = 5120000 / resistor_uohm;
> > +     ret = regmap_write(priv->regmap, RTQ6056_REG_CALIBRATION, calib_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     priv->shunt_resistor_uohm = resistor_uohm;
>
> Blank line before a simple return like this. It slightly helps readability.
>
Ack in next.
> > +     return 0;
> > +}
>
> ...
>
> > +static IIO_CONST_ATTR_NAMED(rtq6056_conv_time_available,
> > +                         integration_time_available,
> > +                         "0.000139 0.000203 0.000269 0.000525 0.001037 0.002061 0.004109 0.008205");
> > +
> > +static IIO_DEVICE_ATTR(shunt_resistor, 0644,
> > +                    rtq6056_shunt_resistor_show,
> > +                    rtq6056_shunt_resistor_store, 0);
> > +
> > +static struct attribute *rtq6056_attributes[] = {
> > +     &iio_const_attr_rtq6056_conv_time_available.dev_attr.attr,
> Whilst I think this particular interface will change anyway, you should
> use the read_avail callback for available attributes for standard ABI.
>
Ack in next.
> > +     &iio_dev_attr_shunt_resistor.dev_attr.attr,
> > +     NULL
> > +};
> > +
> > +static const struct attribute_group rtq6056_attribute_group = {
> > +     .attrs = rtq6056_attributes,
> > +};
> > +
> > +static const struct iio_info rtq6056_info = {
> > +     .attrs = &rtq6056_attribute_group,
> > +     .read_raw = rtq6056_adc_read_raw,
> > +     .write_raw = rtq6056_adc_write_raw,
> > +     .read_label = rtq6056_adc_read_label,
> > +};
>
> ...
> > +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?
> Similar for dev. once a function has multiple accesses to i2c->dev,
> things are cleaner if you just have
>
> struct device *dev = &i2c->dev; and use dev after that.
>
Ack in next.
> > +     if (IS_ERR(priv->regmap))
> > +             return dev_err_probe(&i2c->dev, PTR_ERR(priv->regmap),
> > +                                  "Failed to init regmap\n");
> > +
> > +     ret = regmap_read(priv->regmap, RTQ6056_REG_MANUFACTID, &vendor_id);
> > +     if (ret)
> > +             return dev_err_probe(&i2c->dev, ret,
> > +                                  "Failed to get manufacturer info\n");
> > +
> > +     if (vendor_id != RTQ6056_VENDOR_ID)
> > +             return dev_err_probe(&i2c->dev, -ENODEV,
> > +                                  "invalid vendor id 0x%04x\n", vendor_id);
> > +
> > +     ret = devm_regmap_field_bulk_alloc(&i2c->dev, priv->regmap,
> > +                                        priv->rm_fields, rtq6056_reg_fields,
> > +                                        F_MAX_FIELDS);
> > +     if (ret)
> > +             return dev_err_probe(&i2c->dev, ret,
> > +                                  "Failed to init regmap field\n");
> > +
> > +     /* Write the default config value */
> > +     ret = regmap_write(priv->regmap, RTQ6056_REG_CONFIG,
> > +                        RTQ6056_DEFAULT_CONFIG);
>
> This write effectively turns the device on so it would be good
> to register a devm_add_action_or_reset() callback here to do what
> you currently have in remove.  That will mean you don't need a separate
> remove at all and that you will put the device in a low power state
> if you get an error after this point.
>
> At some stage it would be nice to do runtime pm with autosuspend so that
> we enter a low power state if no one is looking at the readings.
>
Ack in next.
> > +     if (ret)
> > +             return dev_err_probe(&i2c->dev, ret,
> > +                                  "Failed to write default config\n");
> > +
> > +     ret = of_property_read_u32(i2c->dev.of_node,
>
> Please use generic firmware properties not device tree specific ones.
> That lets people use other firmware interfaces such as ACPI using the
> PRP0001 HID.
>
Wil use 'device_property_read_u32', instead.
> > +                                "richtek,shunt-resistor-uohm",
> > +                                &shunt_resistor_uohm);
> > +     if (ret)
> > +             shunt_resistor_uohm = RTQ6056_DEFAULT_RSHUNT;
> Flip this around
>
>         shunt_resistor_uohm = 2000;
>         device_property_read_u32(dev, "rictek,shunt-resistor-uohm",
>                                  &shunt_resistor_uohm);
>
> if the property read fails, it won't change the value stored.
>
Ack in next.
> > +
> > +     ret = rtq6056_set_shunt_resistor(priv, shunt_resistor_uohm);
> > +     if (ret)
> > +             dev_err_probe(&i2c->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(&i2c->dev, indio_dev, NULL,
> > +                                           rtq6056_buffer_trigger_handler,
> > +                                           NULL);
> > +     if (ret)
> > +             return dev_err_probe(&i2c->dev, ret,
> > +                                  "Failed to allocate iio trigger buffer\n");
> > +
> > +     return devm_iio_device_register(&i2c->dev, indio_dev);
> > +}
> > +
> > +static int rtq6056_remove(struct i2c_client *i2c)
> > +{
> > +     struct rtq6056_priv *priv = i2c_get_clientdata(i2c);
> > +
> > +     /* Config opmode to 'shutdown' mode to minimize quiescient current */
> > +     return regmap_field_write(priv->rm_fields[F_OPMODE], 0);
> > +}
> > +
> > +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.
> > +}
> > +
> > +static const struct of_device_id rtq6056_device_match[] = {
> > +     { .compatible = "richtek,rtq6056", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, rtq6056_device_match);
> > +
> > +static struct i2c_driver rtq6056_driver = {
> > +     .driver = {
> > +             .name = "rtq6056",
> > +             .of_match_table = rtq6056_device_match,
> > +     },
> > +     .probe_new = rtq6056_probe,
> > +     .remove = rtq6056_remove,
> > +     .shutdown = rtq6056_shutdown,
> > +};
> > +module_i2c_driver(rtq6056_driver);
> > +
> > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> > +MODULE_DESCRIPTION("Richtek RTQ6056 ADC Driver");
> > +MODULE_LICENSE("GPL v2");
>

Re: [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[ChiYuan Huang]

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月18日 週六 上午9:45寫道：
>
> On 17/06/2022 02:32, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add the documentation for Richtek RTQ6056.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> >  .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> > new file mode 100644
> > index 00000000..6b4e0e0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
> > +
> > +maintainers:
> > +  - ChiYuan Huang <cy_huang@richtek.com>
> > +
> > +description: |
> > +  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
> > +  interface, and the device provides full information for system by reading
> > +  out the loading current and power.
> > +
> > +  The device monitors both of the drops across sense resistor and the BUS
> > +  voltage, converts into the current in amperes, and power in watts through
> > +  internal analog-to-digital converter ADC. The programmable calibration,
> > +  adjustable conversion time, and averaging function are also built in for
> > +  more design flexibility.
> > +
> > +  Datasheet is available at
> > +  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: richtek,rtq6056
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +  richtek,shunt-resistor-uohm:
>
> Use standard properties, so "-micro-ohms". Drop the unit from
> description and drop the ref.
>
Use richtek.shunt-resistor-micro-ohms?
Or the prefix 'richtek,' can be removed?
> > +    description: Shunt IN+/IN- sensing node resistor in microohm.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#io-channel-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      rtq6056@40 {
>
> Generic node name, so probably "adc" (or something else appropriate).
>
Ack in next.
> > +        compatible = "richtek,rtq6056";
> > +        reg = <0x40>;
> > +        #io-channel-cells = <1>;
> > +      };
> > +    };
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[Krzysztof Kozlowski]

On 18/06/2022 08:50, ChiYuan Huang wrote:
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月18日 週六 上午9:45寫道：
>>
>> On 17/06/2022 02:32, cy_huang wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> Add the documentation for Richtek RTQ6056.
>>>
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>> ---
>>>  .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
>>>  1 file changed, 57 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
>>> new file mode 100644
>>> index 00000000..6b4e0e0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
>>> @@ -0,0 +1,57 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
>>> +
>>> +maintainers:
>>> +  - ChiYuan Huang <cy_huang@richtek.com>
>>> +
>>> +description: |
>>> +  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
>>> +  interface, and the device provides full information for system by reading
>>> +  out the loading current and power.
>>> +
>>> +  The device monitors both of the drops across sense resistor and the BUS
>>> +  voltage, converts into the current in amperes, and power in watts through
>>> +  internal analog-to-digital converter ADC. The programmable calibration,
>>> +  adjustable conversion time, and averaging function are also built in for
>>> +  more design flexibility.
>>> +
>>> +  Datasheet is available at
>>> +  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: richtek,rtq6056
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  "#io-channel-cells":
>>> +    const: 1
>>> +
>>> +  richtek,shunt-resistor-uohm:
>>
>> Use standard properties, so "-micro-ohms". Drop the unit from
>> description and drop the ref.
>>
> Use richtek.shunt-resistor-micro-ohms?

Yes.

> Or the prefix 'richtek,' can be removed?

No, the vendor prefix must stay.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[Jonathan Cameron]

On Sat, 18 Jun 2022 19:00:19 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 18/06/2022 08:50, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月18日 週六 上午9:45寫道：  
> >>
> >> On 17/06/2022 02:32, cy_huang wrote:  
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Add the documentation for Richtek RTQ6056.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>> ---
> >>>  .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
> >>>  1 file changed, 57 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> >>> new file mode 100644
> >>> index 00000000..6b4e0e0
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> >>> @@ -0,0 +1,57 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
> >>> +
> >>> +maintainers:
> >>> +  - ChiYuan Huang <cy_huang@richtek.com>
> >>> +
> >>> +description: |
> >>> +  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
> >>> +  interface, and the device provides full information for system by reading
> >>> +  out the loading current and power.
> >>> +
> >>> +  The device monitors both of the drops across sense resistor and the BUS
> >>> +  voltage, converts into the current in amperes, and power in watts through
> >>> +  internal analog-to-digital converter ADC. The programmable calibration,
> >>> +  adjustable conversion time, and averaging function are also built in for
> >>> +  more design flexibility.
> >>> +
> >>> +  Datasheet is available at
> >>> +  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: richtek,rtq6056
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  "#io-channel-cells":
> >>> +    const: 1
> >>> +
> >>> +  richtek,shunt-resistor-uohm:  
> >>
> >> Use standard properties, so "-micro-ohms". Drop the unit from
> >> description and drop the ref.
> >>  
> > Use richtek.shunt-resistor-micro-ohms?  
> 
> Yes.
> 
> > Or the prefix 'richtek,' can be removed?  
> 
> No, the vendor prefix must stay.

It's a standard generic binding used in a bunch of IIO and hwmon
drivers without vendor prefix.  Mind you I can't find a definition
in any of the top level binding files.  Probably wants to be in
adc.yaml for IIO.

Jonathan

 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[Jonathan Cameron]

On Sat, 18 Jun 2022 18:34:40 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 18 Jun 2022 19:00:19 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 18/06/2022 08:50, ChiYuan Huang wrote:  
> > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月18日 週六 上午9:45寫道：    
> > >>
> > >> On 17/06/2022 02:32, cy_huang wrote:    
> > >>> From: ChiYuan Huang <cy_huang@richtek.com>
> > >>>
> > >>> Add the documentation for Richtek RTQ6056.
> > >>>
> > >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > >>> ---
> > >>>  .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
> > >>>  1 file changed, 57 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> > >>> new file mode 100644
> > >>> index 00000000..6b4e0e0
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
> > >>> @@ -0,0 +1,57 @@
> > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
> > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>> +
> > >>> +title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
> > >>> +
> > >>> +maintainers:
> > >>> +  - ChiYuan Huang <cy_huang@richtek.com>
> > >>> +
> > >>> +description: |
> > >>> +  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
> > >>> +  interface, and the device provides full information for system by reading
> > >>> +  out the loading current and power.
> > >>> +
> > >>> +  The device monitors both of the drops across sense resistor and the BUS
> > >>> +  voltage, converts into the current in amperes, and power in watts through
> > >>> +  internal analog-to-digital converter ADC. The programmable calibration,
> > >>> +  adjustable conversion time, and averaging function are also built in for
> > >>> +  more design flexibility.
> > >>> +
> > >>> +  Datasheet is available at
> > >>> +  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
> > >>> +
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    const: richtek,rtq6056
> > >>> +
> > >>> +  reg:
> > >>> +    maxItems: 1
> > >>> +
> > >>> +  "#io-channel-cells":
> > >>> +    const: 1
> > >>> +
> > >>> +  richtek,shunt-resistor-uohm:    
> > >>
> > >> Use standard properties, so "-micro-ohms". Drop the unit from
> > >> description and drop the ref.
> > >>    
> > > Use richtek.shunt-resistor-micro-ohms?    
> > 
> > Yes.
> >   
> > > Or the prefix 'richtek,' can be removed?    
> > 
> > No, the vendor prefix must stay.  
> 
> It's a standard generic binding used in a bunch of IIO and hwmon
> drivers without vendor prefix.  Mind you I can't find a definition

by which I meant to say "bindings" not drivers :)

> in any of the top level binding files.  Probably wants to be in
> adc.yaml for IIO.
> 
> Jonathan
> 
>  
> > 
> > Best regards,
> > Krzysztof  
> 

Re: [PATCH 1/2] dt-bindings: iio: adc: Add rtq6056 adc support

[Krzysztof Kozlowski]

On 18/06/2022 19:34, Jonathan Cameron wrote:
> On Sat, 18 Jun 2022 19:00:19 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 18/06/2022 08:50, ChiYuan Huang wrote:
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 於 2022年6月18日 週六 上午9:45寫道：  
>>>>
>>>> On 17/06/2022 02:32, cy_huang wrote:  
>>>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>>>
>>>>> Add the documentation for Richtek RTQ6056.
>>>>>
>>>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>>>> ---
>>>>>  .../bindings/iio/adc/richtek,rtq6056-adc.yaml      | 57 ++++++++++++++++++++++
>>>>>  1 file changed, 57 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
>>>>> new file mode 100644
>>>>> index 00000000..6b4e0e0
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056-adc.yaml
>>>>> @@ -0,0 +1,57 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/iio/adc/richtek,rtq6056-adc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: RTQ6056 Bi-Directional Current and Power Monitor with 16-bit ADC
>>>>> +
>>>>> +maintainers:
>>>>> +  - ChiYuan Huang <cy_huang@richtek.com>
>>>>> +
>>>>> +description: |
>>>>> +  The RTQ6056 is a high accuracy current-sense monitor with I2C and SMBus
>>>>> +  interface, and the device provides full information for system by reading
>>>>> +  out the loading current and power.
>>>>> +
>>>>> +  The device monitors both of the drops across sense resistor and the BUS
>>>>> +  voltage, converts into the current in amperes, and power in watts through
>>>>> +  internal analog-to-digital converter ADC. The programmable calibration,
>>>>> +  adjustable conversion time, and averaging function are also built in for
>>>>> +  more design flexibility.
>>>>> +
>>>>> +  Datasheet is available at
>>>>> +  https://www.richtek.com/assets/product_file/RTQ6056/DSQ6056-00.pdf
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: richtek,rtq6056
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  "#io-channel-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  richtek,shunt-resistor-uohm:  
>>>>
>>>> Use standard properties, so "-micro-ohms". Drop the unit from
>>>> description and drop the ref.
>>>>  
>>> Use richtek.shunt-resistor-micro-ohms?  
>>
>> Yes.
>>
>>> Or the prefix 'richtek,' can be removed?  
>>
>> No, the vendor prefix must stay.
> 
> It's a standard generic binding used in a bunch of IIO and hwmon
> drivers without vendor prefix.  Mind you I can't find a definition
> in any of the top level binding files.  Probably wants to be in
> adc.yaml for IIO.

Indeed, thanks for correcting me. It should be just
"shunt-resistor-micro-ohms" then.


Best regards,
Krzysztof

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[Andy Shevchenko]

On Sat, Jun 18, 2022 at 5:16 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月18日 週六 凌晨1:08寫道：
> > On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote:

...

> > > +KernelVersion: 5.18.2
> >
> > Wrong version, this won't be part of a stable kernel.
> >
> From kernel.org, currently the stable kernel version is 5.18.5.
> Change to 5.18.5?

Nope. You need to use realistic kernel version, and as I said it can't
be a stable one.

...

> > But for sure you missed

> >   types.h
> >
> Ack in next. But for types.h, i2c.h already include device.h.
> And device.h already include types.h.
> Is it still needed to declare explicitly for types.h??

Yes. You have to include all headers you are a direct user of, except
the ones that are guaranteed to be included by others. The types.h is
not guaranteed to be included by listed above.

...

> > > +       struct device *dev;
> > > +       struct regmap *regmap;
> >
> > Swapping these two might give less code in the generated binary. Have
> > you run bloat-o-meter?
> >
> I never know about this tool.
> I'll check it before I submit the next revision.
> Thanks for the reminding.
>
> But from Jonathan's reply, I may remove 'struct regmap *regmap'.
> If all function need the 'regmap', a local variable 'regmap' need to
> be declared.
> To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.

It's fine, but you may experiment with bloat-o-meter even in that case
out of curiosity.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[Jonathan Cameron]

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

> Jonathan Cameron <Jonathan.Cameron@huawei.com> 於 2022年6月18日 週六 凌晨1:10寫道：
> >
> > On Fri, 17 Jun 2022 17:32:55 +0800
> > cy_huang <u0084500@gmail.com> wrote:
> >  
> > > From: ChiYuan Huang <cy_huang@richtek.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@richtek.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

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[ChiYuan Huang]

Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月19日 週日 下午6:38寫道：
>
> On Sat, Jun 18, 2022 at 5:16 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月18日 週六 凌晨1:08寫道：
> > > On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote:
>
> ...
>
> > > > +KernelVersion: 5.18.2
> > >
> > > Wrong version, this won't be part of a stable kernel.
> > >
> > From kernel.org, currently the stable kernel version is 5.18.5.
> > Change to 5.18.5?
>
> Nope. You need to use realistic kernel version, and as I said it can't
> be a stable one.
>
> ...
>
> > > But for sure you missed
>
> > >   types.h
> > >
> > Ack in next. But for types.h, i2c.h already include device.h.
> > And device.h already include types.h.
> > Is it still needed to declare explicitly for types.h??
>
> Yes. You have to include all headers you are a direct user of, except
> the ones that are guaranteed to be included by others. The types.h is
> not guaranteed to be included by listed above.
>
> ...
>
> > > > +       struct device *dev;
> > > > +       struct regmap *regmap;
> > >
> > > Swapping these two might give less code in the generated binary. Have
> > > you run bloat-o-meter?
> > >
> > I never know about this tool.
> > I'll check it before I submit the next revision.
> > Thanks for the reminding.
> >
> > But from Jonathan's reply, I may remove 'struct regmap *regmap'.
> > If all function need the 'regmap', a local variable 'regmap' need to
> > be declared.
> > To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.
>
> It's fine, but you may experiment with bloat-o-meter even in that case
> out of curiosity.
>
I tred to only swap these two line for *dev and *regmap.
Check the below two cases
1. bloat-o-meter with rtq6056 as the builtin
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Function                                     old     new   delta
Total: Before=24428680, After=24428680, chg +0.00%
2. size tool with rtq6056 as the kernel build
   text    data     bss     dec     hex filename
   5261    1155       0    6416    1910 drivers/iio/adc/rtq6056-adc.ko.old
   text    data     bss     dec     hex filename
   5261    1155       0    6416    1910 drivers/iio/adc/rtq6056-adc.ko

It's weird that there's no difference.

Do I misunderstand something?

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/2] iio: adc: Add rtq6056 support

[Andy Shevchenko]

On Fri, Jun 24, 2022 at 5:39 AM ChiYuan Huang <u0084500@gmail.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月19日 週日 下午6:38寫道：
> > On Sat, Jun 18, 2022 at 5:16 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> > > Andy Shevchenko <andy.shevchenko@gmail.com> 於 2022年6月18日 週六 凌晨1:08寫道：
> > > > On Fri, Jun 17, 2022 at 11:37 AM cy_huang <u0084500@gmail.com> wrote:

...

> > > > > +       struct device *dev;
> > > > > +       struct regmap *regmap;
> > > >
> > > > Swapping these two might give less code in the generated binary. Have
> > > > you run bloat-o-meter?
> > > >
> > > I never know about this tool.
> > > I'll check it before I submit the next revision.
> > > Thanks for the reminding.
> > >
> > > But from Jonathan's reply, I may remove 'struct regmap *regmap'.
> > > If all function need the 'regmap', a local variable 'regmap' need to
> > > be declared.
> > > To use struct regmap *regmap = dev_get_regmap(dev, NULL) is more effective.
> >
> > It's fine, but you may experiment with bloat-o-meter even in that case
> > out of curiosity.
> >
> I tred to only swap these two line for *dev and *regmap.
> Check the below two cases
> 1. bloat-o-meter with rtq6056 as the builtin
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> Function                                     old     new   delta
> Total: Before=24428680, After=24428680, chg +0.00%
> 2. size tool with rtq6056 as the kernel build
>    text    data     bss     dec     hex filename
>    5261    1155       0    6416    1910 drivers/iio/adc/rtq6056-adc.ko.old
>    text    data     bss     dec     hex filename
>    5261    1155       0    6416    1910 drivers/iio/adc/rtq6056-adc.ko
>
> It's weird that there's no difference.
>
> Do I misunderstand something?

Nope, it means that in _current_ code this makes no change. Feel free
to go with your variant if you prefer.

-- 
With Best Regards,
Andy Shevchenko