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

[PATCH v3 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 v3:
- Add Reviewed-by tag in binding document patch
- Resotre the enum for control field.
- Put all the predefined datas/callbacks in dev_data.
- Remove the unused 'rtq6059_info'.
- Change 'default_conv_time' to 'default_conv_time_us'.
- Move the comment for default config above the dev_data setting line.

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                     | 270 ++++++++++++++++--
 2 files changed, 259 insertions(+), 20 deletions(-)


base-commit: 610a9b8f49fbcf1100716370d3b5f6f884a2835a
-- 
2.34.1

[PATCH v3 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v3
- Add Reviewed-by tag.

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

[PATCH v3 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>
---
v3
- Resotre the enum for control field.
- Put all the predefined datas/callbacks in dev_data.
- Remove the unused 'rtq6059_info'.
- Change 'default_conv_time' to 'default_conv_time_us'.
- Move the comment for default config above the dev_data setting line.

v2
- Remove rtq6053 in DT match table and make rtq6053 fallback compatible
  with rtq6056
---
 drivers/iio/adc/rtq6056.c | 270 +++++++++++++++++++++++++++++++++++---
 1 file changed, 251 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
index ad4cea6839b2..c87d78e161ce 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,
@@ -56,10 +66,28 @@ enum {
 	F_MAX_FIELDS
 };
 
+struct rtq6056_priv;
+
+struct richtek_dev_data {
+	bool fixed_samp_freq;
+	u8 vbus_offset;
+	int default_conv_time_us;
+	unsigned int default_config;
+	unsigned int calib_coefficient;
+	const int *avg_sample_list;
+	int avg_sample_list_length;
+	const struct reg_field *reg_fields;
+	const struct iio_chan_spec *channels;
+	int num_channels;
+	int (*read_scale)(struct iio_chan_spec const *ch, int *val, int *val2);
+	int (*set_average)(struct rtq6056_priv *priv, int val);
+};
+
 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 +102,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_VSHUNTCT] = REG_FIELD(RTQ6056_REG_CONFIG, 3, 6),
+	[F_VBUSCT] = REG_FIELD(RTQ6056_REG_CONFIG, 7, 10),
+	[F_AVG]	= 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 +187,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 +287,21 @@ 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:
+		regval >>= devdata->vbus_offset;
 		*val = regval;
-	else
+		break;
+	case RTQ6056_REG_POWER:
+		*val = regval;
+		break;
+	case RTQ6056_REG_SHUNTVOLT:
+	case RTQ6056_REG_CURRENT:
 		*val = sign_extend32(regval, 16);
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	return IIO_VAL_INT;
 }
@@ -199,6 +329,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 +400,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 +424,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_VBUSCT],
+				 RTQ6059_AVG_BASE + selector);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(priv->rm_fields[F_VSHUNTCT],
+				 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,12 +472,13 @@ 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:
-		return rtq6056_adc_read_scale(chan, val, val2);
+		return devdata->read_scale(chan, val, val2);
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = priv->avg_sample;
 		return IIO_VAL_INT;
@@ -313,6 +494,9 @@ 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:
 		*vals = rtq6056_samp_freq_list;
@@ -320,9 +504,9 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
 		*length = ARRAY_SIZE(rtq6056_samp_freq_list);
 		return IIO_AVAIL_LIST;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		*vals = rtq6056_avg_sample_list;
+		*vals = devdata->avg_sample_list;
+		*length = devdata->avg_sample_list_length;
 		*type = IIO_VAL_INT;
-		*length = ARRAY_SIZE(rtq6056_avg_sample_list);
 		return IIO_AVAIL_LIST;
 	default:
 		return -EINVAL;
@@ -334,6 +518,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 +527,13 @@ 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->fixed_samp_freq)
+			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);
+		ret = devdata->set_average(priv, val);
 		break;
 	default:
 		ret = -EINVAL;
@@ -374,6 +562,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 +571,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;
@@ -450,6 +639,7 @@ 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 +659,9 @@ static irqreturn_t rtq6056_buffer_trigger_handler(int irq, void *p)
 		if (ret)
 			goto out;
 
+		if (addr == RTQ6056_REG_BUSVOLT)
+			raw >>= devdata->vbus_offset;
+
 		data.vals[i++] = raw;
 	}
 
@@ -528,20 +721,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_us;
 	priv->avg_sample = 1;
+	priv->devdata = devdata;
 	i2c_set_clientdata(i2c, priv);
 
 	regmap = devm_regmap_init_i2c(i2c, &rtq6056_regmap_config);
@@ -561,15 +760,11 @@ static int rtq6056_probe(struct i2c_client *i2c)
 				     "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");
 
-	/*
-	 * By default, configure average sample as 1, bus and shunt conversion
-	 * time as 1037 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 +793,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 +835,45 @@ 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 = {
+	.default_conv_time_us = 1037,
+	.calib_coefficient = 5120000,
+	/*
+	 * By default, configure average sample as 1, bus and shunt conversion
+	 * time as 1037 microsecond, and operating mode to all on.
+	 */
+	.default_config = RTQ6056_DEFAULT_CONFIG,
+	.avg_sample_list = rtq6056_avg_sample_list,
+	.avg_sample_list_length = ARRAY_SIZE(rtq6056_avg_sample_list),
+	.reg_fields = rtq6056_reg_fields,
+	.channels = rtq6056_channels,
+	.num_channels = ARRAY_SIZE(rtq6056_channels),
+	.read_scale = rtq6056_adc_read_scale,
+	.set_average = rtq6056_adc_set_average,
+};
+
+static const struct richtek_dev_data rtq6059_devdata = {
+	.fixed_samp_freq = true,
+	.vbus_offset = RTQ6059_VBUS_LSB_OFFSET,
+	.default_conv_time_us = 532,
+	.calib_coefficient = 40960000,
+	/*
+	 * By default, configure average sample as 1, bus and shunt conversion
+	 * time as 532 microsecond, and operating mode to all on.
+	 */
+	.default_config = RTQ6059_DEFAULT_CONFIG,
+	.avg_sample_list = rtq6059_avg_sample_list,
+	.avg_sample_list_length = ARRAY_SIZE(rtq6059_avg_sample_list),
+	.reg_fields = rtq6059_reg_fields,
+	.channels = rtq6059_channels,
+	.num_channels = ARRAY_SIZE(rtq6059_channels),
+	.read_scale = rtq6059_adc_read_scale,
+	.set_average = rtq6059_adc_set_average,
+};
+
 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 v3 2/2] iio: adc: rtq6056: Add support for the whole RTQ6056 family

[Jonathan Cameron]

On Tue, 2 Jan 2024 19:28:22 +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>

I replied to your question on v2. As a general rule don't send a new version
out quite so quickly.  Better to let all questions be answered (or a week or so
to pass) before sending a new version.  That way it saves on review time.

Anyhow, some follow up comments and one or two things I didn't notice before.

Jonathan

> ---
> v3
> - Resotre the enum for control field.
> - Put all the predefined datas/callbacks in dev_data.
> - Remove the unused 'rtq6059_info'.
> - Change 'default_conv_time' to 'default_conv_time_us'.
> - Move the comment for default config above the dev_data setting line.
> 
> v2
> - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
>   with rtq6056
> ---
>  drivers/iio/adc/rtq6056.c | 270 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 251 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> index ad4cea6839b2..c87d78e161ce 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
> +};
> +

No longer used so drop this enum.

>  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 +287,21 @@ 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:
> +		regval >>= devdata->vbus_offset;
>  		*val = regval;
> -	else
> +		break;
Can just return directly here
		return IIO_VAL_INT;
> +	case RTQ6056_REG_POWER:
> +		*val = regval;
> +		break;
		return IIO_VAL_INT;
> +	case RTQ6056_REG_SHUNTVOLT:
> +	case RTQ6056_REG_CURRENT:
>  		*val = sign_extend32(regval, 16);
> +		break;
		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	return IIO_VAL_INT;
And drop this.
>  }
> @@ -199,6 +329,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
>  	}
>  }
>  

> @@ -334,6 +518,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 +527,13 @@ 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->fixed_samp_freq)
> +			ret = -EINVAL;

Slight preference here for following form to make it easy to see nothing is done
in error paths beyond breaking out of the switch statement.

		if (devdata->fixed_samp_freq) {
			ret = -EINVAL;
			break;
		}

		ret = rtq....
		break;

> +		else
> +			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
>  		break;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> -		ret = rtq6056_adc_set_average(priv, val);
> +		ret = devdata->set_average(priv, val);
>  		break;
>  	default:
>  		ret = -EINVAL;
;
> @@ -598,8 +793,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;

You have added devdata->num_channels but seem to have forgotten to use it.

>  	indio_dev->info = &rtq6056_info;
>  
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> @@ -640,8 +835,45 @@ static int rtq6056_runtime_resume(struct device *dev)
>  static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
>  				 rtq6056_runtime_resume, NULL);
>  

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

[ChiYuan Huang]

On Tue, Jan 02, 2024 at 07:53:40PM +0000, Jonathan Cameron wrote:
> On Tue, 2 Jan 2024 19:28:22 +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>
> 
> I replied to your question on v2. As a general rule don't send a new version
> out quite so quickly.  Better to let all questions be answered (or a week or so
> to pass) before sending a new version.  That way it saves on review time.
OK. I'll send the new one after all the answers were got.
> 
> Anyhow, some follow up comments and one or two things I didn't notice before.
>
All wii be fixed in v4. 
> Jonathan
> 
> > ---
> > v3
> > - Resotre the enum for control field.
> > - Put all the predefined datas/callbacks in dev_data.
> > - Remove the unused 'rtq6059_info'.
> > - Change 'default_conv_time' to 'default_conv_time_us'.
> > - Move the comment for default config above the dev_data setting line.
> > 
> > v2
> > - Remove rtq6053 in DT match table and make rtq6053 fallback compatible
> >   with rtq6056
> > ---
> >  drivers/iio/adc/rtq6056.c | 270 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 251 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
> > index ad4cea6839b2..c87d78e161ce 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
> > +};
> > +
> 
> No longer used so drop this enum.
> 
> >  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 +287,21 @@ 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:
> > +		regval >>= devdata->vbus_offset;
> >  		*val = regval;
> > -	else
> > +		break;
> Can just return directly here
> 		return IIO_VAL_INT;
> > +	case RTQ6056_REG_POWER:
> > +		*val = regval;
> > +		break;
> 		return IIO_VAL_INT;
> > +	case RTQ6056_REG_SHUNTVOLT:
> > +	case RTQ6056_REG_CURRENT:
> >  		*val = sign_extend32(regval, 16);
> > +		break;
> 		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> >  
> >  	return IIO_VAL_INT;
> And drop this.
> >  }
> > @@ -199,6 +329,28 @@ static int rtq6056_adc_read_scale(struct iio_chan_spec const *ch, int *val,
> >  	}
> >  }
> >  
> 
> > @@ -334,6 +518,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 +527,13 @@ 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->fixed_samp_freq)
> > +			ret = -EINVAL;
> 
> Slight preference here for following form to make it easy to see nothing is done
> in error paths beyond breaking out of the switch statement.
> 
> 		if (devdata->fixed_samp_freq) {
> 			ret = -EINVAL;
> 			break;
> 		}
> 
> 		ret = rtq....
> 		break;
> 
> > +		else
> > +			ret = rtq6056_adc_set_samp_freq(priv, chan, val);
> >  		break;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > -		ret = rtq6056_adc_set_average(priv, val);
> > +		ret = devdata->set_average(priv, val);
> >  		break;
> >  	default:
> >  		ret = -EINVAL;
> ;
> > @@ -598,8 +793,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;
> 
> You have added devdata->num_channels but seem to have forgotten to use it.
> 
> >  	indio_dev->info = &rtq6056_info;
> >  
> >  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > @@ -640,8 +835,45 @@ static int rtq6056_runtime_resume(struct device *dev)
> >  static DEFINE_RUNTIME_DEV_PM_OPS(rtq6056_pm_ops, rtq6056_runtime_suspend,
> >  				 rtq6056_runtime_resume, NULL);
> >