[PATCH v2 0/2] RTQ6056: Add compatible for the same chip family

[PATCH v2 0/2] RTQ6056: Add compatible for the same chip family

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

RTQ6053 and RTQ6059 are the same RTQ6056 family.
The differences are listed below
- RTQ6053
  Only change chip package type
- RTQ6059
  1. Enlarge the shunt voltage sensing range
  2. Shrink the pinout for VBUS sense pin
  3. Due to 1, the scale value is also changed

Since v2:
- Refine the description of 'compatible' property

ChiYuan Huang (2):
  dt-bindings: iio: adc: rtq6056: add support for the whole RTQ6056
    family
  iio: adc: rtq6056: Add support for the whole RTQ6056 family

 .../bindings/iio/adc/richtek,rtq6056.yaml     |   9 +-
 drivers/iio/adc/rtq6056.c                     | 264 +++++++++++++++++-
 2 files changed, 258 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: iio: adc: rtq6056: add support for the whole RTQ6056 family

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Add compatible support for RTQ6053 and RTQ6059.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
v2
- Refine the 'compatible' description with oneOf and listed items.
---
 .../devicetree/bindings/iio/adc/richtek,rtq6056.yaml     | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056.yaml b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056.yaml
index 88e008629ea8..af2c3a67f888 100644
--- a/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/richtek,rtq6056.yaml
@@ -25,7 +25,14 @@ description: |
 
 properties:
   compatible:
-    const: richtek,rtq6056
+    oneOf:
+      - enum:
+          - richtek,rtq6056
+          - richtek,rtq6059
+      - items:
+          - enum:
+              - richtek,rtq6053
+          - const: richtek,rtq6056
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: iio: adc: rtq6056: add support for the whole RTQ6056 family

[Krzysztof Kozlowski]

On 28/12/2023 12:29, cy_huang@richtek.com wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Add compatible support for RTQ6053 and RTQ6059.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

RTQ6053 and RTQ6059 are the same series of RTQ6056.

The respective differences with RTQ6056 are listed below
RTQ6053
- chip package type

RTQ6059
- Reduce the pinout for vbus sensing pin
- Some internal ADC scaling change

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
v2
- Remove rtq6053 in DT match table and make rtq6053 fallback compatible
  with rtq6056
---
 drivers/iio/adc/rtq6056.c | 264 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 250 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
index ad4cea6839b2..5587178cea83 100644
--- a/drivers/iio/adc/rtq6056.c
+++ b/drivers/iio/adc/rtq6056.c
@@ -39,6 +39,16 @@
 #define RTQ6056_DEFAULT_CONFIG	0x4127
 #define RTQ6056_CONT_ALLON	7
 
+#define RTQ6059_DEFAULT_CONFIG	0x3C47
+#define RTQ6059_VBUS_LSB_OFFSET	3
+#define RTQ6059_AVG_BASE	8
+
+enum {
+	RICHTEK_DEV_RTQ6056 = 0,
+	RICHTEK_DEV_RTQ6059,
+	RICHTEK_DEV_MAX
+};
+
 enum {
 	RTQ6056_CH_VSHUNT = 0,
 	RTQ6056_CH_VBUS,
@@ -50,16 +60,29 @@ enum {
 enum {
 	F_OPMODE = 0,
 	F_VSHUNTCT,
+	F_SADC = F_VSHUNTCT,
 	F_VBUSCT,
+	F_BADC = F_VBUSCT,
 	F_AVG,
+	F_PGA = F_AVG,
 	F_RESET,
 	F_MAX_FIELDS
 };
 
+struct richtek_dev_data {
+	int dev_id;
+	int default_conv_time;
+	unsigned int default_config;
+	unsigned int calib_coefficient;
+	const struct reg_field *reg_fields;
+	const struct iio_chan_spec *channels;
+};
+
 struct rtq6056_priv {
 	struct device *dev;
 	struct regmap *regmap;
 	struct regmap_field *rm_fields[F_MAX_FIELDS];
+	const struct richtek_dev_data *devdata;
 	u32 shunt_resistor_uohm;
 	int vshuntct_us;
 	int vbusct_us;
@@ -74,6 +97,14 @@ static const struct reg_field rtq6056_reg_fields[F_MAX_FIELDS] = {
 	[F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15),
 };
 
+static const struct reg_field rtq6059_reg_fields[F_MAX_FIELDS] = {
+	[F_OPMODE] = REG_FIELD(RTQ6056_REG_CONFIG, 0, 2),
+	[F_SADC] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 6),
+	[F_BADC] = REG_FIELD(RTQ6056_REG_CONFIG, 7, 10),
+	[F_PGA]	= REG_FIELD(RTQ6056_REG_CONFIG, 11, 12),
+	[F_RESET] = REG_FIELD(RTQ6056_REG_CONFIG, 15, 15),
+};
+
 static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
 	{
 		.type = IIO_VOLTAGE,
@@ -151,10 +182,93 @@ static const struct iio_chan_spec rtq6056_channels[RTQ6056_MAX_CHANNEL + 1] = {
 	IIO_CHAN_SOFT_TIMESTAMP(RTQ6056_MAX_CHANNEL),
 };
 
+/*
+ * Difference between RTQ6056 and RTQ6059
+ * - Fixed sampling conversion time
+ * - Average sample numbers
+ * - Channel scale
+ * - calibration coefficient
+ */
+static const struct iio_chan_spec rtq6059_channels[RTQ6056_MAX_CHANNEL + 1] = {
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.address = RTQ6056_REG_SHUNTVOLT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = 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 = 1,
+		.address = RTQ6056_REG_BUSVOLT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = 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 = 2,
+		.address = RTQ6056_REG_POWER,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = 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 = 3,
+		.address = RTQ6056_REG_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available = 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,
 				    struct iio_chan_spec const *ch,
 				    int *val)
 {
+	const struct richtek_dev_data *devdata = priv->devdata;
 	struct device *dev = priv->dev;
 	unsigned int addr = ch->address;
 	unsigned int regval;
@@ -168,10 +282,18 @@ static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
 		return ret;
 
 	/* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
-	if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
+	switch (addr) {
+	case RTQ6056_REG_BUSVOLT:
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
+			regval >>= RTQ6059_VBUS_LSB_OFFSET;
+		fallthrough;
+	case RTQ6056_REG_POWER:
 		*val = regval;
-	else
+		break;
+	default:
 		*val = sign_extend32(regval, 16);
+		break;
+	}
 
 	return IIO_VAL_INT;
 }
@@ -199,6 +321,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
 	}
 }
 
+static int rtq6059_adc_read_scale(struct iio_chan_spec const *ch, int *val,
+				  int *val2)
+{
+	switch (ch->address) {
+	case RTQ6056_REG_SHUNTVOLT:
+		/* VSHUNT lsb  10uV */
+		*val = 10000;
+		*val2 = 1000000;
+		return IIO_VAL_FRACTIONAL;
+	case RTQ6056_REG_BUSVOLT:
+		/* VBUS lsb 4mV */
+		*val = 4;
+		return IIO_VAL_INT;
+	case RTQ6056_REG_POWER:
+		/* Power lsb 20mW */
+		*val = 20;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
 /*
  * Sample frequency for channel VSHUNT and VBUS. The indices correspond
  * with the bit value expected by the chip. And it can be found at
@@ -248,6 +392,10 @@ static const int rtq6056_avg_sample_list[] = {
 	1, 4, 16, 64, 128, 256, 512, 1024,
 };
 
+static const int rtq6059_avg_sample_list[] = {
+	1, 2, 4, 8, 16, 32, 64, 128,
+};
+
 static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
 {
 	unsigned int selector;
@@ -268,6 +416,30 @@ static int rtq6056_adc_set_average(struct rtq6056_priv *priv, int val)
 	return 0;
 }
 
+static int rtq6059_adc_set_average(struct rtq6056_priv *priv, int val)
+{
+	unsigned int selector;
+	int ret;
+
+	if (val > 128 || val < 1)
+		return -EINVAL;
+
+	/* The supported average sample is 2^x (x from 0 to 7) */
+	selector = fls(val) - 1;
+
+	ret = regmap_field_write(priv->rm_fields[F_BADC],
+				 RTQ6059_AVG_BASE + selector);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(priv->rm_fields[F_SADC],
+				 RTQ6059_AVG_BASE + selector);
+
+	priv->avg_sample = BIT(selector + 1);
+
+	return 0;
+}
+
 static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv,
 				       struct iio_chan_spec const *ch, int *val)
 {
@@ -292,11 +464,15 @@ static int rtq6056_adc_read_raw(struct iio_dev *indio_dev,
 				int *val2, long mask)
 {
 	struct rtq6056_priv *priv = iio_priv(indio_dev);
+	const struct richtek_dev_data *devdata = priv->devdata;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		return rtq6056_adc_read_channel(priv, chan, val);
 	case IIO_CHAN_INFO_SCALE:
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
+			return rtq6059_adc_read_scale(chan, val, val2);
+
 		return rtq6056_adc_read_scale(chan, val, val2);
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = priv->avg_sample;
@@ -313,16 +489,28 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
 				  const int **vals, int *type, int *length,
 				  long mask)
 {
+	struct rtq6056_priv *priv = iio_priv(indio_dev);
+	const struct richtek_dev_data *devdata = priv->devdata;
+
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
+			return -EINVAL;
+
 		*vals = rtq6056_samp_freq_list;
 		*type = IIO_VAL_INT;
 		*length = ARRAY_SIZE(rtq6056_samp_freq_list);
 		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		*vals = rtq6056_avg_sample_list;
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059) {
+			*vals = rtq6059_avg_sample_list;
+			*length = ARRAY_SIZE(rtq6059_avg_sample_list);
+		} else {
+			*vals = rtq6056_avg_sample_list;
+			*length = ARRAY_SIZE(rtq6056_avg_sample_list);
+		}
+
 		*type = IIO_VAL_INT;
-		*length = ARRAY_SIZE(rtq6056_avg_sample_list);
 		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
@@ -334,6 +522,7 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
 				 int val2, long mask)
 {
 	struct rtq6056_priv *priv = iio_priv(indio_dev);
+	const struct richtek_dev_data *devdata = priv->devdata;
 	int ret;
 
 	ret = iio_device_claim_direct_mode(indio_dev);
@@ -342,10 +531,16 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		ret = rtq6056_adc_set_samp_freq(priv, chan, val);
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
+			ret = -EINVAL;
+		else
+			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
 		break;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		ret = rtq6056_adc_set_average(priv, val);
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
+			ret = rtq6059_adc_set_average(priv, val);
+		else
+			ret = rtq6056_adc_set_average(priv, val);
 		break;
 	default:
 		ret = -EINVAL;
@@ -374,6 +569,7 @@ static int rtq6056_adc_read_label(struct iio_dev *indio_dev,
 static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
 				      int resistor_uohm)
 {
+	const struct richtek_dev_data *devdata = priv->devdata;
 	unsigned int calib_val;
 	int ret;
 
@@ -382,8 +578,8 @@ static int rtq6056_set_shunt_resistor(struct rtq6056_priv *priv,
 		return -EINVAL;
 	}
 
-	/* calibration = 5120000 / (Rshunt (uOhm) * current lsb (1mA)) */
-	calib_val = 5120000 / resistor_uohm;
+	/* calibration = coefficient / (Rshunt (uOhm) * current lsb (1mA)) */
+	calib_val = devdata->calib_coefficient / resistor_uohm;
 	ret = regmap_write(priv->regmap, RTQ6056_REG_CALIBRATION, calib_val);
 	if (ret)
 		return ret;
@@ -445,11 +641,16 @@ static const struct iio_info rtq6056_info = {
 	.read_label = rtq6056_adc_read_label,
 };
 
+static const struct iio_info rtq6059_info = {
+	.attrs = &rtq6056_attribute_group,
+};
+
 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);
+	const struct richtek_dev_data *devdata = priv->devdata;
 	struct device *dev = priv->dev;
 	struct {
 		u16 vals[RTQ6056_MAX_CHANNEL];
@@ -469,6 +670,10 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
 		if (ret)
 			goto out;
 
+		if (devdata->dev_id == RICHTEK_DEV_RTQ6059 &&
+		    addr == RTQ6056_REG_BUSVOLT)
+			raw >>= RTQ6059_VBUS_LSB_OFFSET;
+
 		data.vals[i++] = raw;
 	}
 
@@ -528,20 +733,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
 	struct rtq6056_priv *priv;
 	struct device *dev = &i2c->dev;
 	struct regmap *regmap;
+	const struct richtek_dev_data *devdata;
 	unsigned int vendor_id, shunt_resistor_uohm;
 	int ret;
 
 	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EOPNOTSUPP;
 
+	devdata = device_get_match_data(dev);
+	if (!devdata)
+		return dev_err_probe(dev, -EINVAL, "Invalid dev data\n");
+
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	priv = iio_priv(indio_dev);
 	priv->dev = dev;
-	priv->vshuntct_us = priv->vbusct_us = 1037;
+	priv->vshuntct_us = priv->vbusct_us = devdata->default_conv_time;
 	priv->avg_sample = 1;
+	priv->devdata = devdata;
 	i2c_set_clientdata(i2c, priv);
 
 	regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
@@ -556,20 +767,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
 		return dev_err_probe(dev, ret,
 				     "Failed to get manufacturer info\n");
 
+	/* For RTQ6059, this vendor id value is meaningless */
 	if (vendor_id != RTQ6056_VENDOR_ID)
 		return dev_err_probe(dev, -ENODEV,
 				     "Invalid vendor id 0x%04x\n", vendor_id);
 
 	ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
-					   rtq6056_reg_fields, F_MAX_FIELDS);
+					   devdata->reg_fields, F_MAX_FIELDS);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init regmap field\n");
 
 	/*
+	 * RTQ6053 & RTQ6056:
 	 * By default, configure average sample as 1, bus and shunt conversion
 	 * time as 1037 microsecond, and operating mode to all on.
+	 *
+	 * RTQ6059:
+	 * By default, configure average sample as 1, bus and shunt conversion
+	 * time as 532 microsecond, and operating mode to all on.
 	 */
-	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
+	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, devdata->default_config);
 	if (ret)
 		return dev_err_probe(dev, ret,
 				     "Failed to enable continuous sensing\n");
@@ -598,8 +815,8 @@ static int rtq6056_probe(struct i2c_client *i2c)
 
 	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->channels = devdata->channels;
+	indio_dev->num_channels = RTQ6056_MAX_CHANNEL + 1;
 	indio_dev->info = &rtq6056_info;
 
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
@@ -640,8 +857,27 @@ static int rtq6056_runtime_resume(struct device *dev)
 static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
 				 rtq6056_runtime_resume, NULL);
 
+static const struct richtek_dev_data rtq6056_devdata = {
+	.dev_id = RICHTEK_DEV_RTQ6056,
+	.default_conv_time = 1037,
+	.calib_coefficient = 5120000,
+	.default_config = RTQ6056_DEFAULT_CONFIG,
+	.reg_fields = rtq6056_reg_fields,
+	.channels = rtq6056_channels,
+};
+
+static const struct richtek_dev_data rtq6059_devdata = {
+	.dev_id = RICHTEK_DEV_RTQ6059,
+	.default_conv_time = 532,
+	.calib_coefficient = 40960000,
+	.default_config = RTQ6059_DEFAULT_CONFIG,
+	.reg_fields = rtq6059_reg_fields,
+	.channels = rtq6059_channels,
+};
+
 static const struct of_device_id rtq6056_device_match[] = {
-	{ .compatible = "richtek,rtq6056" },
+	{ .compatible = "richtek,rtq6056", .data = &rtq6056_devdata },
+	{ .compatible = "richtek,rtq6059", .data = &rtq6059_devdata },
 	{}
 };
 MODULE_DEVICE_TABLE(of, rtq6056_device_match);
-- 
2.34.1

Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[Jonathan Cameron]

On Thu, 28 Dec 2023 19:29:35 +0800
<cy_huang@richtek.com> wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> RTQ6053 and RTQ6059 are the same series of RTQ6056.
> 
> The respective differences with RTQ6056 are listed below
> RTQ6053
> - chip package type
> 
> RTQ6059
> - Reduce the pinout for vbus sensing pin
> - Some internal ADC scaling change
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
Hi ChiYuan,

Some comments inline, most focus on not mixing device specific features
across code and data.  You always want them to be fully specified by
the the device specific const struct directly not by code using an
ID from there.  It ends up more readable and more flexible to have it
all done via data or callbacks where things are a too complex for data.

Thanks,

Jonathan

> ---
> v2
> - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
>   with rtq6056
> ---
>  drivers/iio/adc/rtq6056.c | 264 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 250 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> index ad4cea6839b2..5587178cea83 100644
> --- a/drivers/iio/adc/rtq6056.c
> +++ b/drivers/iio/adc/rtq6056.c
> @@ -39,6 +39,16 @@
>  #define RTQ6056_DEFAULT_CONFIG	0x4127
>  #define RTQ6056_CONT_ALLON	7
>  
> +#define RTQ6059_DEFAULT_CONFIG	0x3C47
> +#define RTQ6059_VBUS_LSB_OFFSET	3
> +#define RTQ6059_AVG_BASE	8
> +
> +enum {
> +	RICHTEK_DEV_RTQ6056 = 0,
> +	RICHTEK_DEV_RTQ6059,
> +	RICHTEK_DEV_MAX
> +};
> +
>  enum {
>  	RTQ6056_CH_VSHUNT = 0,
>  	RTQ6056_CH_VBUS,
> @@ -50,16 +60,29 @@ enum {
>  enum {
>  	F_OPMODE = 0,
>  	F_VSHUNTCT,
> +	F_SADC = F_VSHUNTCT,

If the devices have different register fields, better to have different enums
for them as well as that should result in less confusing code.


>  	F_VBUSCT,
> +	F_BADC = F_VBUSCT,
>  	F_AVG,
> +	F_PGA = F_AVG,
>  	F_RESET,
>  	F_MAX_FIELDS
>  };
>  
> +struct richtek_dev_data {
> +	int dev_id;

It almost always turns out to be a bad idea to use a mixture of
'data' in a structure like this and a device id plus special casing int he
code.  Better to add more data to this structure or callbacks specific
to the individual devices types.  So I shouldn't see a dev_id field in
here at all.

> +	int default_conv_time;
> +	unsigned int default_config;
> +	unsigned int calib_coefficient;
> +	const struct reg_field *reg_fields;
> +	const struct iio_chan_spec *channels;
> +};

...

>  static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
>  				    struct iio_chan_spec const *ch,
>  				    int *val)
>  {
> +	const struct richtek_dev_data *devdata = priv->devdata;
>  	struct device *dev = priv->dev;
>  	unsigned int addr = ch->address;
>  	unsigned int regval;
> @@ -168,10 +282,18 @@ static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
>  		return ret;
>  
>  	/* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
> -	if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
> +	switch (addr) {
> +	case RTQ6056_REG_BUSVOLT:
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> +			regval >>= RTQ6059_VBUS_LSB_OFFSET;

Store the offset as for other case below and apply it unconditionally. If it is
zero then this is a noop.

> +		fallthrough;
> +	case RTQ6056_REG_POWER:
>  		*val = regval;
> -	else
> +		break;
> +	default:
>  		*val = sign_extend32(regval, 16);

Fallthrough stuff is harder to read so only use it when there is significant saving
in code.  here just repeat the sign_extend32() in both cases.

> +		break;
> +	}
>  
>  	return IIO_VAL_INT;
>  }
> @@ -199,6 +321,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
>  	}
>  }
>  

>  static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv,
>  				       struct iio_chan_spec const *ch, int *val)
>  {
> @@ -292,11 +464,15 @@ static int rtq6056_adc_read_raw(struct iio_dev *indio_dev,
>  				int *val2, long mask)
>  {
>  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> +	const struct richtek_dev_data *devdata = priv->devdata;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		return rtq6056_adc_read_channel(priv, chan, val);
>  	case IIO_CHAN_INFO_SCALE:
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> +			return rtq6059_adc_read_scale(chan, val, val2);

Provide a callback for this as for other examples below.

> +
>  		return rtq6056_adc_read_scale(chan, val, val2);
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		*val = priv->avg_sample;
> @@ -313,16 +489,28 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
>  				  const int **vals, int *type, int *length,
>  				  long mask)
>  {
> +	struct rtq6056_priv *priv = iio_priv(indio_dev);
> +	const struct richtek_dev_data *devdata = priv->devdata;
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> +			return -EINVAL;

Shouldn't need this protection as the channels won't have relevant
bitmap bit set and this will never be called.

> +
>  		*vals = rtq6056_samp_freq_list;
>  		*type = IIO_VAL_INT;
>  		*length = ARRAY_SIZE(rtq6056_samp_freq_list);
>  		return IIO_AVAIL_LIST;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		*vals = rtq6056_avg_sample_list;
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059) {
> +			*vals = rtq6059_avg_sample_list;
> +			*length = ARRAY_SIZE(rtq6059_avg_sample_list);
Store all this in devdata.

		*vals = devdata->avg_sample_list;
		*length = devdata->avg_sample_list_length;

> +		} else {
> +			*vals = rtq6056_avg_sample_list;
> +			*length = ARRAY_SIZE(rtq6056_avg_sample_list);
> +		}
> +
>  		*type = IIO_VAL_INT;
> -		*length = ARRAY_SIZE(rtq6056_avg_sample_list);
>  		return IIO_AVAIL_LIST;
>  	default:
>  		return -EINVAL;
> @@ -334,6 +522,7 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
>  				 int val2, long mask)
>  {
>  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> +	const struct richtek_dev_data *devdata = priv->devdata;
>  	int ret;
>  
>  	ret = iio_device_claim_direct_mode(indio_dev);
> @@ -342,10 +531,16 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> +			ret = -EINVAL;
> +		else
> +			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
>  		break;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		ret = rtq6056_adc_set_average(priv, val);
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> +			ret = rtq6059_adc_set_average(priv, val);
> +		else
> +			ret = rtq6056_adc_set_average(priv, val);

Provide a callback in devdata so this becomes something like
		ret = devdata->set_averate(priv, val);

>  		break;
>  	default:

> +static const struct iio_info rtq6059_info = {
> +	.attrs = &rtq6056_attribute_group,

This is odd. you don't provide an access functions so you won't be able to read
channels etc. It isn't used so I guess you should just get rid of it.

> +};
> +
>  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);
> +	const struct richtek_dev_data *devdata = priv->devdata;
>  	struct device *dev = priv->dev;
>  	struct {
>  		u16 vals[RTQ6056_MAX_CHANNEL];
> @@ -469,6 +670,10 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
>  		if (ret)
>  			goto out;
>  
> +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059 &&
> +		    addr == RTQ6056_REG_BUSVOLT)

Store an offset in the devdata->dev_id and this becomes something like.
		if (addr == RTQ6056_REG_BUS_VOLT)
			raw >>= devdata->vbus_offset;

> +			raw >>= RTQ6059_VBUS_LSB_OFFSET;
> +
>  		data.vals[i++] = raw;
>  	}
>  
> @@ -528,20 +733,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
>  	struct rtq6056_priv *priv;
>  	struct device *dev = &i2c->dev;
>  	struct regmap *regmap;
> +	const struct richtek_dev_data *devdata;
>  	unsigned int vendor_id, shunt_resistor_uohm;
>  	int ret;
>  
>  	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
>  		return -EOPNOTSUPP;
>  
> +	devdata = device_get_match_data(dev);
> +	if (!devdata)
> +		return dev_err_probe(dev, -EINVAL, "Invalid dev data\n");
> +
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	priv = iio_priv(indio_dev);
>  	priv->dev = dev;
> -	priv->vshuntct_us = priv->vbusct_us = 1037;
> +	priv->vshuntct_us = priv->vbusct_us = devdata->default_conv_time;

I'd keep a _us postfix for default_conv_time to make the units of that
self documenting as well.

>  	priv->avg_sample = 1;
> +	priv->devdata = devdata;
>  	i2c_set_clientdata(i2c, priv);
>  
>  	regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
> @@ -556,20 +767,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
>  		return dev_err_probe(dev, ret,
>  				     "Failed to get manufacturer info\n");
>  
> +	/* For RTQ6059, this vendor id value is meaningless */

If that is the case, why are you checking it?
I'd read that comment as meaning this will fail for RTQ6059

>  	if (vendor_id != RTQ6056_VENDOR_ID)
>  		return dev_err_probe(dev, -ENODEV,
>  				     "Invalid vendor id 0x%04x\n", vendor_id);
>  
>  	ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
> -					   rtq6056_reg_fields, F_MAX_FIELDS);
> +					   devdata->reg_fields, F_MAX_FIELDS);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to init regmap field\n");
>  
>  	/*
> +	 * RTQ6053 & RTQ6056:
>  	 * By default, configure average sample as 1, bus and shunt conversion
>  	 * time as 1037 microsecond, and operating mode to all on.
> +	 *
> +	 * RTQ6059:
> +	 * By default, configure average sample as 1, bus and shunt conversion
> +	 * time as 532 microsecond, and operating mode to all on.
Move this documentation to where devdata->default_config is set. 
It's device specific information, so put it in the device specific place not
the main code flow.

>  	 */
> -	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
> +	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, devdata->default_config);
>  	if (ret)
>  		return dev_err_probe(dev, ret,
>  				     "Failed to enable continuous sensing\n");
> @@ -598,8 +815,8 @@ static int rtq6056_probe(struct i2c_client *i2c)
>  
>  	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->channels = devdata->channels;
> +	indio_dev->num_channels = RTQ6056_MAX_CHANNEL + 1;

I'd embed the number of channels in devdata as well then the ARRAY_SIZE() code
that is obviously correct can still be used (just with different things being
counted depending on which channels are being used). The gain in readability
is worth the tiny bit of extra code and data.

>  	indio_dev->info = &rtq6056_info;
>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> @@ -640,8 +857,27 @@ static int rtq6056_runtime_resume(struct device *dev)
>  static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
>  				 rtq6056_runtime_resume, NULL);
>  
> +static const struct richtek_dev_data rtq6056_devdata = {
> +	.dev_id = RICHTEK_DEV_RTQ6056,
> +	.default_conv_time = 1037,
> +	.calib_coefficient = 5120000,
> +	.default_config = RTQ6056_DEFAULT_CONFIG,
> +	.reg_fields = rtq6056_reg_fields,
> +	.channels = rtq6056_channels,
> +};
> +
> +static const struct richtek_dev_data rtq6059_devdata = {
> +	.dev_id = RICHTEK_DEV_RTQ6059,
As mentioned above, this mix of data and a dev_id is not a good design pattern.
It tends to end up as insufficiently flexible as support for more devices is
added to a driver - plus it scatters the device type specific stuff all through
the driver rather than having it all in one place.

> +	.default_conv_time = 532,
> +	.calib_coefficient = 40960000,
> +	.default_config = RTQ6059_DEFAULT_CONFIG,
> +	.reg_fields = rtq6059_reg_fields,
> +	.channels = rtq6059_channels,
> +};
> +
>  static const struct of_device_id rtq6056_device_match[] = {
> -	{ .compatible = "richtek,rtq6056" },
> +	{ .compatible = "richtek,rtq6056", .data = &rtq6056_devdata },
> +	{ .compatible = "richtek,rtq6059", .data = &rtq6059_devdata },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, rtq6056_device_match);

Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[ChiYuan Huang]

Hi, Johathan:

Most comments are good and will be fixed in next revision.

Still one comment I cannot make sure.

Please see the comment that's below yours.
	
On Sat, Dec 30, 2023 at 12:03:47PM +0000, Jonathan Cameron wrote:
> On Thu, 28 Dec 2023 19:29:35 +0800
> <cy_huang@richtek.com> wrote:
> 
> > From: ChiYuan Huang <cy_huang@richtek.com>
> > 
> > RTQ6053 and RTQ6059 are the same series of RTQ6056.
> > 
> > The respective differences with RTQ6056 are listed below
> > RTQ6053
> > - chip package type
> > 
> > RTQ6059
> > - Reduce the pinout for vbus sensing pin
> > - Some internal ADC scaling change
> > 
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> Hi ChiYuan,
> 
> Some comments inline, most focus on not mixing device specific features
> across code and data.  You always want them to be fully specified by
> the the device specific const struct directly not by code using an
> ID from there.  It ends up more readable and more flexible to have it
> all done via data or callbacks where things are a too complex for data.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > v2
> > - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
> >   with rtq6056
> > ---
> >  drivers/iio/adc/rtq6056.c | 264 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 250 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> > index ad4cea6839b2..5587178cea83 100644
> > --- a/drivers/iio/adc/rtq6056.c
> > +++ b/drivers/iio/adc/rtq6056.c
> > @@ -39,6 +39,16 @@
> >  #define RTQ6056_DEFAULT_CONFIG	0x4127
> >  #define RTQ6056_CONT_ALLON	7
> >  
> > +#define RTQ6059_DEFAULT_CONFIG	0x3C47
> > +#define RTQ6059_VBUS_LSB_OFFSET	3
> > +#define RTQ6059_AVG_BASE	8
> > +
> > +enum {
> > +	RICHTEK_DEV_RTQ6056 = 0,
> > +	RICHTEK_DEV_RTQ6059,
> > +	RICHTEK_DEV_MAX
> > +};
> > +
> >  enum {
> >  	RTQ6056_CH_VSHUNT = 0,
> >  	RTQ6056_CH_VBUS,
> > @@ -50,16 +60,29 @@ enum {
> >  enum {
> >  	F_OPMODE = 0,
> >  	F_VSHUNTCT,
> > +	F_SADC = F_VSHUNTCT,
> 
> If the devices have different register fields, better to have different enums
> for them as well as that should result in less confusing code.
> 
Actually, this is all the same register, just the control naming difference.
If not to define the new eum, I can remain to use the same field to handle rtq6059 part.
> 
> >  	F_VBUSCT,
> > +	F_BADC = F_VBUSCT,
> >  	F_AVG,
> > +	F_PGA = F_AVG,
> >  	F_RESET,
> >  	F_MAX_FIELDS
> >  };
> >  
> > +struct richtek_dev_data {
> > +	int dev_id;
> 
> It almost always turns out to be a bad idea to use a mixture of
> 'data' in a structure like this and a device id plus special casing int he
> code.  Better to add more data to this structure or callbacks specific
> to the individual devices types.  So I shouldn't see a dev_id field in
> here at all.
> 
> > +	int default_conv_time;
> > +	unsigned int default_config;
> > +	unsigned int calib_coefficient;
> > +	const struct reg_field *reg_fields;
> > +	const struct iio_chan_spec *channels;
> > +};
> 
> ...
> 
> >  static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
> >  				    struct iio_chan_spec const *ch,
> >  				    int *val)
> >  {
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  	struct device *dev = priv->dev;
> >  	unsigned int addr = ch->address;
> >  	unsigned int regval;
> > @@ -168,10 +282,18 @@ static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
> >  		return ret;
> >  
> >  	/* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
> > -	if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
> > +	switch (addr) {
> > +	case RTQ6056_REG_BUSVOLT:
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			regval >>= RTQ6059_VBUS_LSB_OFFSET;
> 
> Store the offset as for other case below and apply it unconditionally. If it is
> zero then this is a noop.
> 
> > +		fallthrough;
> > +	case RTQ6056_REG_POWER:
> >  		*val = regval;
> > -	else
> > +		break;
> > +	default:
> >  		*val = sign_extend32(regval, 16);
> 
> Fallthrough stuff is harder to read so only use it when there is significant saving
> in code.  here just repeat the sign_extend32() in both cases.
> 
> > +		break;
> > +	}
> >  
> >  	return IIO_VAL_INT;
> >  }
> > @@ -199,6 +321,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
> >  	}
> >  }
> >  
> 
> >  static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv,
> >  				       struct iio_chan_spec const *ch, int *val)
> >  {
> > @@ -292,11 +464,15 @@ static int rtq6056_adc_read_raw(struct iio_dev *indio_dev,
> >  				int *val2, long mask)
> >  {
> >  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> >  		return rtq6056_adc_read_channel(priv, chan, val);
> >  	case IIO_CHAN_INFO_SCALE:
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			return rtq6059_adc_read_scale(chan, val, val2);
> 
> Provide a callback for this as for other examples below.
> 
> > +
> >  		return rtq6056_adc_read_scale(chan, val, val2);
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >  		*val = priv->avg_sample;
> > @@ -313,16 +489,28 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
> >  				  const int **vals, int *type, int *length,
> >  				  long mask)
> >  {
> > +	struct rtq6056_priv *priv = iio_priv(indio_dev);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> > +
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			return -EINVAL;
> 
> Shouldn't need this protection as the channels won't have relevant
> bitmap bit set and this will never be called.
> 
> > +
> >  		*vals = rtq6056_samp_freq_list;
> >  		*type = IIO_VAL_INT;
> >  		*length = ARRAY_SIZE(rtq6056_samp_freq_list);
> >  		return IIO_AVAIL_LIST;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		*vals = rtq6056_avg_sample_list;
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059) {
> > +			*vals = rtq6059_avg_sample_list;
> > +			*length = ARRAY_SIZE(rtq6059_avg_sample_list);
> Store all this in devdata.
> 
> 		*vals = devdata->avg_sample_list;
> 		*length = devdata->avg_sample_list_length;
> 
> > +		} else {
> > +			*vals = rtq6056_avg_sample_list;
> > +			*length = ARRAY_SIZE(rtq6056_avg_sample_list);
> > +		}
> > +
> >  		*type = IIO_VAL_INT;
> > -		*length = ARRAY_SIZE(rtq6056_avg_sample_list);
> >  		return IIO_AVAIL_LIST;
> >  	default:
> >  		return -EINVAL;
> > @@ -334,6 +522,7 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
> >  				 int val2, long mask)
> >  {
> >  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  	int ret;
> >  
> >  	ret = iio_device_claim_direct_mode(indio_dev);
> > @@ -342,10 +531,16 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> > -		ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			ret = -EINVAL;
> > +		else
> > +			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> >  		break;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		ret = rtq6056_adc_set_average(priv, val);
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			ret = rtq6059_adc_set_average(priv, val);
> > +		else
> > +			ret = rtq6056_adc_set_average(priv, val);
> 
> Provide a callback in devdata so this becomes something like
> 		ret = devdata->set_averate(priv, val);
> 
> >  		break;
> >  	default:
> 
> > +static const struct iio_info rtq6059_info = {
> > +	.attrs = &rtq6056_attribute_group,
> 
> This is odd. you don't provide an access functions so you won't be able to read
> channels etc. It isn't used so I guess you should just get rid of it.
> 
> > +};
> > +
> >  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);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  	struct device *dev = priv->dev;
> >  	struct {
> >  		u16 vals[RTQ6056_MAX_CHANNEL];
> > @@ -469,6 +670,10 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
> >  		if (ret)
> >  			goto out;
> >  
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059 &&
> > +		    addr == RTQ6056_REG_BUSVOLT)
> 
> Store an offset in the devdata->dev_id and this becomes something like.
> 		if (addr == RTQ6056_REG_BUS_VOLT)
> 			raw >>= devdata->vbus_offset;
> 
> > +			raw >>= RTQ6059_VBUS_LSB_OFFSET;
> > +
> >  		data.vals[i++] = raw;
> >  	}
> >  
> > @@ -528,20 +733,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
> >  	struct rtq6056_priv *priv;
> >  	struct device *dev = &i2c->dev;
> >  	struct regmap *regmap;
> > +	const struct richtek_dev_data *devdata;
> >  	unsigned int vendor_id, shunt_resistor_uohm;
> >  	int ret;
> >  
> >  	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> >  		return -EOPNOTSUPP;
> >  
> > +	devdata = device_get_match_data(dev);
> > +	if (!devdata)
> > +		return dev_err_probe(dev, -EINVAL, "Invalid dev data\n");
> > +
> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> >  	priv = iio_priv(indio_dev);
> >  	priv->dev = dev;
> > -	priv->vshuntct_us = priv->vbusct_us = 1037;
> > +	priv->vshuntct_us = priv->vbusct_us = devdata->default_conv_time;
> 
> I'd keep a _us postfix for default_conv_time to make the units of that
> self documenting as well.
> 
> >  	priv->avg_sample = 1;
> > +	priv->devdata = devdata;
> >  	i2c_set_clientdata(i2c, priv);
> >  
> >  	regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
> > @@ -556,20 +767,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
> >  		return dev_err_probe(dev, ret,
> >  				     "Failed to get manufacturer info\n");
> >  
> > +	/* For RTQ6059, this vendor id value is meaningless */
> 
> If that is the case, why are you checking it?
> I'd read that comment as meaning this will fail for RTQ6059
> 
> >  	if (vendor_id != RTQ6056_VENDOR_ID)
> >  		return dev_err_probe(dev, -ENODEV,
> >  				     "Invalid vendor id 0x%04x\n", vendor_id);
> >  
> >  	ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
> > -					   rtq6056_reg_fields, F_MAX_FIELDS);
> > +					   devdata->reg_fields, F_MAX_FIELDS);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "Failed to init regmap field\n");
> >  
> >  	/*
> > +	 * RTQ6053 & RTQ6056:
> >  	 * By default, configure average sample as 1, bus and shunt conversion
> >  	 * time as 1037 microsecond, and operating mode to all on.
> > +	 *
> > +	 * RTQ6059:
> > +	 * By default, configure average sample as 1, bus and shunt conversion
> > +	 * time as 532 microsecond, and operating mode to all on.
> Move this documentation to where devdata->default_config is set. 
> It's device specific information, so put it in the device specific place not
> the main code flow.
> 
> >  	 */
> > -	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
> > +	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, devdata->default_config);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret,
> >  				     "Failed to enable continuous sensing\n");
> > @@ -598,8 +815,8 @@ static int rtq6056_probe(struct i2c_client *i2c)
> >  
> >  	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->channels = devdata->channels;
> > +	indio_dev->num_channels = RTQ6056_MAX_CHANNEL + 1;
> 
> I'd embed the number of channels in devdata as well then the ARRAY_SIZE() code
> that is obviously correct can still be used (just with different things being
> counted depending on which channels are being used). The gain in readability
> is worth the tiny bit of extra code and data.
> 
> >  	indio_dev->info = &rtq6056_info;
> >  
> >  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > @@ -640,8 +857,27 @@ static int rtq6056_runtime_resume(struct device *dev)
> >  static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
> >  				 rtq6056_runtime_resume, NULL);
> >  
> > +static const struct richtek_dev_data rtq6056_devdata = {
> > +	.dev_id = RICHTEK_DEV_RTQ6056,
> > +	.default_conv_time = 1037,
> > +	.calib_coefficient = 5120000,
> > +	.default_config = RTQ6056_DEFAULT_CONFIG,
> > +	.reg_fields = rtq6056_reg_fields,
> > +	.channels = rtq6056_channels,
> > +};
> > +
> > +static const struct richtek_dev_data rtq6059_devdata = {
> > +	.dev_id = RICHTEK_DEV_RTQ6059,
> As mentioned above, this mix of data and a dev_id is not a good design pattern.
> It tends to end up as insufficiently flexible as support for more devices is
> added to a driver - plus it scatters the device type specific stuff all through
> the driver rather than having it all in one place.
> 
> > +	.default_conv_time = 532,
> > +	.calib_coefficient = 40960000,
> > +	.default_config = RTQ6059_DEFAULT_CONFIG,
> > +	.reg_fields = rtq6059_reg_fields,
> > +	.channels = rtq6059_channels,
> > +};
> > +
> >  static const struct of_device_id rtq6056_device_match[] = {
> > -	{ .compatible = "richtek,rtq6056" },
> > +	{ .compatible = "richtek,rtq6056", .data = &rtq6056_devdata },
> > +	{ .compatible = "richtek,rtq6059", .data = &rtq6059_devdata },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, rtq6056_device_match);
> 

Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[Jonathan Cameron]

On Tue, 2 Jan 2024 16:30:42 +0800
ChiYuan Huang <cy_huang@richtek.com> wrote:

> Hi, Johathan:
> 
> Most comments are good and will be fixed in next revision.
> 
> Still one comment I cannot make sure.
> 
> Please see the comment that's below yours.
> 	
Hi ChiYuan,

It's good practice to crop away all the parts where the discussion is finished.
Makes it easier for people to find the bit you want discussion to continue on!

I've done so in this reply.

...
> > > +
> > >  enum {
> > >  	RTQ6056_CH_VSHUNT = 0,
> > >  	RTQ6056_CH_VBUS,
> > > @@ -50,16 +60,29 @@ enum {
> > >  enum {
> > >  	F_OPMODE = 0,
> > >  	F_VSHUNTCT,
> > > +	F_SADC = F_VSHUNTCT,  
> > 
> > If the devices have different register fields, better to have different enums
> > for them as well as that should result in less confusing code.
> >   
> Actually, this is all the same register, just the control naming difference.
> If not to define the new eum, I can remain to use the same field to handle rtq6059 part.

If the bits in the register control the same thing across both parts then
add a comment alongside the enum to make that clear. 

Given the naming that seems very unlikely.  PGA and AVG would eman
very different things to me for starters (oversampling vs a programmble
gain amplifier on the front end)

> >   
> > >  	F_VBUSCT,
> > > +	F_BADC = F_VBUSCT,
> > >  	F_AVG,
> > > +	F_PGA = F_AVG,
> > >  	F_RESET,
> > >  	F_MAX_FIELDS
> > >  };

Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[ChiYuan Huang]

On Tue, Jan 02, 2024 at 07:36:42PM +0000, Jonathan Cameron wrote:
> On Tue, 2 Jan 2024 16:30:42 +0800
> ChiYuan Huang <cy_huang@richtek.com> wrote:
> 
> > Hi, Johathan:
> > 
> > Most comments are good and will be fixed in next revision.
> > 
> > Still one comment I cannot make sure.
> > 
> > Please see the comment that's below yours.
> > 	
> Hi ChiYuan,
> 
> It's good practice to crop away all the parts where the discussion is finished.
> Makes it easier for people to find the bit you want discussion to continue on!
> 
> I've done so in this reply.
> 
> ...
> > > > +
> > > >  enum {
> > > >  	RTQ6056_CH_VSHUNT = 0,
> > > >  	RTQ6056_CH_VBUS,
> > > > @@ -50,16 +60,29 @@ enum {
> > > >  enum {
> > > >  	F_OPMODE = 0,
> > > >  	F_VSHUNTCT,
> > > > +	F_SADC = F_VSHUNTCT,  
> > > 
> > > If the devices have different register fields, better to have different enums
> > > for them as well as that should result in less confusing code.
> > >   
> > Actually, this is all the same register, just the control naming difference.
> > If not to define the new eum, I can remain to use the same field to handle rtq6059 part.
> 
> If the bits in the register control the same thing across both parts then
> add a comment alongside the enum to make that clear. 
> 
> Given the naming that seems very unlikely.  PGA and AVG would eman
> very different things to me for starters (oversampling vs a programmble
> gain amplifier on the front end)
> 
I'm also thinking how to write this difference like as comments or a seperate enum.
But if to define a new enum, many function about the regfield controls must be seperated
for 6056 and 6059.
> > >   
> > > >  	F_VBUSCT,
> > > > +	F_BADC = F_VBUSCT,
> > > >  	F_AVG,
> > > > +	F_PGA = F_AVG,
> > > >  	F_RESET,
> > > >  	F_MAX_FIELDS
> > > >  };

What if to keep the original coding, just to rename the different part like as below
F_SADC -> F_RTQ6059_SDAC
F_BADC -> F_RTQ6059_BADC
F_PGA -> F_RTQ6059_PGA

At least, the nameing already shows the difference between 6056 and 6059.
Only these three parts are different, others are the same like as F_OPMODE, F_RESET.

Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[Jonathan Cameron]

On Wed, 3 Jan 2024 09:04:52 +0800
ChiYuan Huang <cy_huang@richtek.com> wrote:

> On Tue, Jan 02, 2024 at 07:36:42PM +0000, Jonathan Cameron wrote:
> > On Tue, 2 Jan 2024 16:30:42 +0800
> > ChiYuan Huang <cy_huang@richtek.com> wrote:
> >   
> > > Hi, Johathan:
> > > 
> > > Most comments are good and will be fixed in next revision.
> > > 
> > > Still one comment I cannot make sure.
> > > 
> > > Please see the comment that's below yours.
> > > 	  
> > Hi ChiYuan,
> > 
> > It's good practice to crop away all the parts where the discussion is finished.
> > Makes it easier for people to find the bit you want discussion to continue on!
> > 
> > I've done so in this reply.
> > 
> > ...  
> > > > > +
> > > > >  enum {
> > > > >  	RTQ6056_CH_VSHUNT = 0,
> > > > >  	RTQ6056_CH_VBUS,
> > > > > @@ -50,16 +60,29 @@ enum {
> > > > >  enum {
> > > > >  	F_OPMODE = 0,
> > > > >  	F_VSHUNTCT,
> > > > > +	F_SADC = F_VSHUNTCT,    
> > > > 
> > > > If the devices have different register fields, better to have different enums
> > > > for them as well as that should result in less confusing code.
> > > >     
> > > Actually, this is all the same register, just the control naming difference.
> > > If not to define the new eum, I can remain to use the same field to handle rtq6059 part.  
> > 
> > If the bits in the register control the same thing across both parts then
> > add a comment alongside the enum to make that clear. 
> > 
> > Given the naming that seems very unlikely.  PGA and AVG would eman
> > very different things to me for starters (oversampling vs a programmble
> > gain amplifier on the front end)
> >   
> I'm also thinking how to write this difference like as comments or a seperate enum.
> But if to define a new enum, many function about the regfield controls must be seperated
> for 6056 and 6059.
> > > >     
> > > > >  	F_VBUSCT,
> > > > > +	F_BADC = F_VBUSCT,
> > > > >  	F_AVG,
> > > > > +	F_PGA = F_AVG,
> > > > >  	F_RESET,
> > > > >  	F_MAX_FIELDS
> > > > >  };  
> 
> What if to keep the original coding, just to rename the different part like as below
> F_SADC -> F_RTQ6059_SDAC
> F_BADC -> F_RTQ6059_BADC
> F_PGA -> F_RTQ6059_PGA

That works well. It makes it clear they are different across the parts.
So agreed this is the best option.
> 
> At least, the nameing already shows the difference between 6056 and 6059.
> Only these three parts are different, others are the same like as F_OPMODE, F_RESET.
> 
> 

Re: [PATCH v2 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[ChiYuan Huang]

On Sat, Dec 30, 2023 at 12:03:47PM +0000, Jonathan Cameron wrote:
> On Thu, 28 Dec 2023 19:29:35 +0800
> <cy_huang@richtek.com> wrote:
> 
> > From: ChiYuan Huang <cy_huang@richtek.com>
> > 
> > RTQ6053 and RTQ6059 are the same series of RTQ6056.
> > 
> > The respective differences with RTQ6056 are listed below
> > RTQ6053
> > - chip package type
> > 
> > RTQ6059
> > - Reduce the pinout for vbus sensing pin
> > - Some internal ADC scaling change
> > 
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> Hi ChiYuan,
> 
> Some comments inline, most focus on not mixing device specific features
> across code and data.  You always want them to be fully specified by
> the the device specific const struct directly not by code using an
> ID from there.  It ends up more readable and more flexible to have it
> all done via data or callbacks where things are a too complex for data.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > v2
> > - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
> >   with rtq6056
> > ---
> >  drivers/iio/adc/rtq6056.c | 264 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 250 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> > index ad4cea6839b2..5587178cea83 100644
> > --- a/drivers/iio/adc/rtq6056.c
> > +++ b/drivers/iio/adc/rtq6056.c
> > @@ -39,6 +39,16 @@
> >  #define RTQ6056_DEFAULT_CONFIG	0x4127
> >  #define RTQ6056_CONT_ALLON	7
> >  
> > +#define RTQ6059_DEFAULT_CONFIG	0x3C47
> > +#define RTQ6059_VBUS_LSB_OFFSET	3
> > +#define RTQ6059_AVG_BASE	8
> > +
> > +enum {
> > +	RICHTEK_DEV_RTQ6056 = 0,
> > +	RICHTEK_DEV_RTQ6059,
> > +	RICHTEK_DEV_MAX
> > +};
> > +
> >  enum {
> >  	RTQ6056_CH_VSHUNT = 0,
> >  	RTQ6056_CH_VBUS,
> > @@ -50,16 +60,29 @@ enum {
> >  enum {
> >  	F_OPMODE = 0,
> >  	F_VSHUNTCT,
> > +	F_SADC = F_VSHUNTCT,
> 
> If the devices have different register fields, better to have different enums
> for them as well as that should result in less confusing code.
> 
> 
> >  	F_VBUSCT,
> > +	F_BADC = F_VBUSCT,
> >  	F_AVG,
> > +	F_PGA = F_AVG,
> >  	F_RESET,
> >  	F_MAX_FIELDS
> >  };
> >  
> > +struct richtek_dev_data {
> > +	int dev_id;
> 
> It almost always turns out to be a bad idea to use a mixture of
> 'data' in a structure like this and a device id plus special casing int he
> code.  Better to add more data to this structure or callbacks specific
> to the individual devices types.  So I shouldn't see a dev_id field in
> here at all.
> 
> > +	int default_conv_time;
> > +	unsigned int default_config;
> > +	unsigned int calib_coefficient;
> > +	const struct reg_field *reg_fields;
> > +	const struct iio_chan_spec *channels;
> > +};
> 
> ...
> 
> >  static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
> >  				    struct iio_chan_spec const *ch,
> >  				    int *val)
> >  {
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  	struct device *dev = priv->dev;
> >  	unsigned int addr = ch->address;
> >  	unsigned int regval;
> > @@ -168,10 +282,18 @@ static int rtq6056_adc_read_channel(struct rtq6056_priv *priv,
> >  		return ret;
> >  
> >  	/* Power and VBUS is unsigned 16-bit, others are signed 16-bit */
> > -	if (addr == RTQ6056_REG_BUSVOLT || addr == RTQ6056_REG_POWER)
> > +	switch (addr) {
> > +	case RTQ6056_REG_BUSVOLT:
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			regval >>= RTQ6059_VBUS_LSB_OFFSET;
> 
> Store the offset as for other case below and apply it unconditionally. If it is
> zero then this is a noop.
> 
> > +		fallthrough;
> > +	case RTQ6056_REG_POWER:
> >  		*val = regval;
> > -	else
> > +		break;
> > +	default:
> >  		*val = sign_extend32(regval, 16);
> 
> Fallthrough stuff is harder to read so only use it when there is significant saving
> in code.  here just repeat the sign_extend32() in both cases.
> 
> > +		break;
> > +	}
> >  
> >  	return IIO_VAL_INT;
> >  }
> > @@ -199,6 +321,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
> >  	}
> >  }
> >  
> 
> >  static int rtq6056_adc_get_sample_freq(struct rtq6056_priv *priv,
> >  				       struct iio_chan_spec const *ch, int *val)
> >  {
> > @@ -292,11 +464,15 @@ static int rtq6056_adc_read_raw(struct iio_dev *indio_dev,
> >  				int *val2, long mask)
> >  {
> >  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> >  		return rtq6056_adc_read_channel(priv, chan, val);
> >  	case IIO_CHAN_INFO_SCALE:
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			return rtq6059_adc_read_scale(chan, val, val2);
> 
> Provide a callback for this as for other examples below.
> 
> > +
> >  		return rtq6056_adc_read_scale(chan, val, val2);
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >  		*val = priv->avg_sample;
> > @@ -313,16 +489,28 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
> >  				  const int **vals, int *type, int *length,
> >  				  long mask)
> >  {
> > +	struct rtq6056_priv *priv = iio_priv(indio_dev);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> > +
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			return -EINVAL;
> 
> Shouldn't need this protection as the channels won't have relevant
> bitmap bit set and this will never be called.
> 
> > +
> >  		*vals = rtq6056_samp_freq_list;
> >  		*type = IIO_VAL_INT;
> >  		*length = ARRAY_SIZE(rtq6056_samp_freq_list);
> >  		return IIO_AVAIL_LIST;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		*vals = rtq6056_avg_sample_list;
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059) {
> > +			*vals = rtq6059_avg_sample_list;
> > +			*length = ARRAY_SIZE(rtq6059_avg_sample_list);
> Store all this in devdata.
> 
> 		*vals = devdata->avg_sample_list;
> 		*length = devdata->avg_sample_list_length;
> 
> > +		} else {
> > +			*vals = rtq6056_avg_sample_list;
> > +			*length = ARRAY_SIZE(rtq6056_avg_sample_list);
> > +		}
> > +
> >  		*type = IIO_VAL_INT;
> > -		*length = ARRAY_SIZE(rtq6056_avg_sample_list);
> >  		return IIO_AVAIL_LIST;
> >  	default:
> >  		return -EINVAL;
> > @@ -334,6 +522,7 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
> >  				 int val2, long mask)
> >  {
> >  	struct rtq6056_priv *priv = iio_priv(indio_dev);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  	int ret;
> >  
> >  	ret = iio_device_claim_direct_mode(indio_dev);
> > @@ -342,10 +531,16 @@ static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SAMP_FREQ:
> > -		ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			ret = -EINVAL;
> > +		else
> > +			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> >  		break;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		ret = rtq6056_adc_set_average(priv, val);
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059)
> > +			ret = rtq6059_adc_set_average(priv, val);
> > +		else
> > +			ret = rtq6056_adc_set_average(priv, val);
> 
> Provide a callback in devdata so this becomes something like
> 		ret = devdata->set_averate(priv, val);
> 
> >  		break;
> >  	default:
> 
> > +static const struct iio_info rtq6059_info = {
> > +	.attrs = &rtq6056_attribute_group,
> 
> This is odd. you don't provide an access functions so you won't be able to read
> channels etc. It isn't used so I guess you should just get rid of it.
> 
> > +};
> > +
> >  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);
> > +	const struct richtek_dev_data *devdata = priv->devdata;
> >  	struct device *dev = priv->dev;
> >  	struct {
> >  		u16 vals[RTQ6056_MAX_CHANNEL];
> > @@ -469,6 +670,10 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
> >  		if (ret)
> >  			goto out;
> >  
> > +		if (devdata->dev_id == RICHTEK_DEV_RTQ6059 &&
> > +		    addr == RTQ6056_REG_BUSVOLT)
> 
> Store an offset in the devdata->dev_id and this becomes something like.
> 		if (addr == RTQ6056_REG_BUS_VOLT)
> 			raw >>= devdata->vbus_offset;
> 
> > +			raw >>= RTQ6059_VBUS_LSB_OFFSET;
> > +
> >  		data.vals[i++] = raw;
> >  	}
> >  
> > @@ -528,20 +733,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
> >  	struct rtq6056_priv *priv;
> >  	struct device *dev = &i2c->dev;
> >  	struct regmap *regmap;
> > +	const struct richtek_dev_data *devdata;
> >  	unsigned int vendor_id, shunt_resistor_uohm;
> >  	int ret;
> >  
> >  	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> >  		return -EOPNOTSUPP;
> >  
> > +	devdata = device_get_match_data(dev);
> > +	if (!devdata)
> > +		return dev_err_probe(dev, -EINVAL, "Invalid dev data\n");
> > +
> >  	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> >  	priv = iio_priv(indio_dev);
> >  	priv->dev = dev;
> > -	priv->vshuntct_us = priv->vbusct_us = 1037;
> > +	priv->vshuntct_us = priv->vbusct_us = devdata->default_conv_time;
> 
> I'd keep a _us postfix for default_conv_time to make the units of that
> self documenting as well.
> 
> >  	priv->avg_sample = 1;
> > +	priv->devdata = devdata;
> >  	i2c_set_clientdata(i2c, priv);
> >  
> >  	regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
> > @@ -556,20 +767,26 @@ static int rtq6056_probe(struct i2c_client *i2c)
> >  		return dev_err_probe(dev, ret,
> >  				     "Failed to get manufacturer info\n");
> >  
> > +	/* For RTQ6059, this vendor id value is meaningless */
> 
> If that is the case, why are you checking it?
> I'd read that comment as meaning this will fail for RTQ6059
>
This is my misunderstanding. That's due to the draft datasheet not said this register
exist.  After I contact the designer, that's all the same with RTQ6056.

So I'll remove the comment line. The check is still needed. RTQ6059 won't fail in this check.
> >  	if (vendor_id != RTQ6056_VENDOR_ID)
> >  		return dev_err_probe(dev, -ENODEV,
> >  				     "Invalid vendor id 0x%04x\n", vendor_id);
> >  
> >  	ret = devm_regmap_field_bulk_alloc(dev, regmap, priv->rm_fields,
> > -					   rtq6056_reg_fields, F_MAX_FIELDS);
> > +					   devdata->reg_fields, F_MAX_FIELDS);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "Failed to init regmap field\n");
> >  
> >  	/*
> > +	 * RTQ6053 & RTQ6056:
> >  	 * By default, configure average sample as 1, bus and shunt conversion
> >  	 * time as 1037 microsecond, and operating mode to all on.
> > +	 *
> > +	 * RTQ6059:
> > +	 * By default, configure average sample as 1, bus and shunt conversion
> > +	 * time as 532 microsecond, and operating mode to all on.
> Move this documentation to where devdata->default_config is set. 
> It's device specific information, so put it in the device specific place not
> the main code flow.
> 
> >  	 */
> > -	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, RTQ6056_DEFAULT_CONFIG);
> > +	ret = regmap_write(regmap, RTQ6056_REG_CONFIG, devdata->default_config);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret,
> >  				     "Failed to enable continuous sensing\n");
> > @@ -598,8 +815,8 @@ static int rtq6056_probe(struct i2c_client *i2c)
> >  
> >  	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->channels = devdata->channels;
> > +	indio_dev->num_channels = RTQ6056_MAX_CHANNEL + 1;
> 
> I'd embed the number of channels in devdata as well then the ARRAY_SIZE() code
> that is obviously correct can still be used (just with different things being
> counted depending on which channels are being used). The gain in readability
> is worth the tiny bit of extra code and data.
> 
> >  	indio_dev->info = &rtq6056_info;
> >  
> >  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > @@ -640,8 +857,27 @@ static int rtq6056_runtime_resume(struct device *dev)
> >  static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
> >  				 rtq6056_runtime_resume, NULL);
> >  
> > +static const struct richtek_dev_data rtq6056_devdata = {
> > +	.dev_id = RICHTEK_DEV_RTQ6056,
> > +	.default_conv_time = 1037,
> > +	.calib_coefficient = 5120000,
> > +	.default_config = RTQ6056_DEFAULT_CONFIG,
> > +	.reg_fields = rtq6056_reg_fields,
> > +	.channels = rtq6056_channels,
> > +};
> > +
> > +static const struct richtek_dev_data rtq6059_devdata = {
> > +	.dev_id = RICHTEK_DEV_RTQ6059,
> As mentioned above, this mix of data and a dev_id is not a good design pattern.
> It tends to end up as insufficiently flexible as support for more devices is
> added to a driver - plus it scatters the device type specific stuff all through
> the driver rather than having it all in one place.
> 
> > +	.default_conv_time = 532,
> > +	.calib_coefficient = 40960000,
> > +	.default_config = RTQ6059_DEFAULT_CONFIG,
> > +	.reg_fields = rtq6059_reg_fields,
> > +	.channels = rtq6059_channels,
> > +};
> > +
> >  static const struct of_device_id rtq6056_device_match[] = {
> > -	{ .compatible = "richtek,rtq6056" },
> > +	{ .compatible = "richtek,rtq6056", .data = &rtq6056_devdata },
> > +	{ .compatible = "richtek,rtq6059", .data = &rtq6059_devdata },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, rtq6056_device_match);
>