[PATCH v2 0/2] iio: adc: ad7124: allow 16 channels

[PATCH v2 0/2] iio: adc: ad7124: allow 16 channels

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

AD7124-8 can have up to 16 pseudo-differential channels
enabled simultaneously and only 8 configurations. In this
scenario we cannot assign one configuration per channel,
some channels will have to share configurations like, ODR,
gain and filter parameters.

Allow the user to specify channels and configurations
separately in device-tree and assign, if needed, the same
configuration to multiple channels.

If two channels share the configuration changing the
sampling rate of one will change the sampling rate of the
other too.

Alexandru Tachici (2):
  iio: adc: ad7124: allow 16 channels
  dt-bindings: iio: adc: ad7124: add config nodes

 .../bindings/iio/adc/adi,ad7124.yaml          |  72 +++++--
 drivers/iio/adc/ad7124.c                      | 183 +++++++++++-------
 2 files changed, 166 insertions(+), 89 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] iio: adc: ad7124: allow 16 channels

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

AD7124-8 can have up to 16 pseudo-differential channels
enabled simultaneously and only 8 configurations. In this
scenario we cannot assign one configuration per channel,
some channels will have to share configurations like, ODR,
gain and filter parameters.

This patch allows the user to specify channels and configurations
separately in device-tree and assign, if needed, the same
configuration to multiple channels.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7124.c | 183 +++++++++++++++++++++++----------------
 1 file changed, 109 insertions(+), 74 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 766c73333604..0df88bea336f 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -86,6 +86,12 @@
 #define AD7124_SINC3_FILTER 2
 #define AD7124_SINC4_FILTER 0
 
+#define AD7124_CONF_ADDR_OFFSET	20
+#define AD7124_MAX_CONFIGS	8
+#define AD7124_MAX_CHANNELS	16
+
+#define AD7124_REG_NO 57
+
 enum ad7124_ids {
 	ID_AD7124_4,
 	ID_AD7124_8,
@@ -136,21 +142,28 @@ struct ad7124_chip_info {
 };
 
 struct ad7124_channel_config {
+	bool enable;
+	unsigned int nr;
 	enum ad7124_ref_sel refsel;
 	bool bipolar;
 	bool buf_positive;
 	bool buf_negative;
-	unsigned int ain;
 	unsigned int vref_mv;
 	unsigned int pga_bits;
 	unsigned int odr;
 	unsigned int filter_type;
 };
 
+struct ad7124_channel {
+	struct ad7124_channel_config *cfg;
+	unsigned int ain;
+};
+
 struct ad7124_state {
 	const struct ad7124_chip_info *chip_info;
 	struct ad_sigma_delta sd;
-	struct ad7124_channel_config *channel_config;
+	struct ad7124_channel channels[AD7124_MAX_CHANNELS];
+	struct ad7124_channel_config configs[AD7124_MAX_CONFIGS];
 	struct regulator *vref[4];
 	struct clk *mclk;
 	unsigned int adc_control;
@@ -243,8 +256,8 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
 	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
 	unsigned int val;
 
-	val = st->channel_config[channel].ain | AD7124_CHANNEL_EN(1) |
-	      AD7124_CHANNEL_SETUP(channel);
+	val = st->channels[channel].ain | AD7124_CHANNEL_EN(1) |
+	      AD7124_CHANNEL_SETUP(st->channels[channel].cfg->nr);
 
 	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(channel), 2, val);
 }
@@ -280,14 +293,13 @@ static int ad7124_set_channel_odr(struct ad7124_state *st,
 	else if (odr_sel_bits > 2047)
 		odr_sel_bits = 2047;
 
-	ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
+	ret = ad7124_spi_write_mask(st, AD7124_FILTER(st->channels[channel].cfg->nr),
 				    AD7124_FILTER_FS_MSK,
 				    AD7124_FILTER_FS(odr_sel_bits), 3);
 	if (ret < 0)
 		return ret;
 	/* fADC = fCLK / (FS[10:0] x 32) */
-	st->channel_config[channel].odr =
-		DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
+	st->channels[channel].cfg->odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
 
 	return 0;
 }
@@ -301,13 +313,13 @@ static int ad7124_set_channel_gain(struct ad7124_state *st,
 
 	res = ad7124_find_closest_match(ad7124_gain,
 					ARRAY_SIZE(ad7124_gain), gain);
-	ret = ad7124_spi_write_mask(st, AD7124_CONFIG(channel),
+	ret = ad7124_spi_write_mask(st, AD7124_CONFIG(st->channels[channel].cfg->nr),
 				    AD7124_CONFIG_PGA_MSK,
 				    AD7124_CONFIG_PGA(res), 2);
 	if (ret < 0)
 		return ret;
 
-	st->channel_config[channel].pga_bits = res;
+	st->channels[channel].cfg->pga_bits = res;
 
 	return 0;
 }
@@ -317,9 +329,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
 {
 	unsigned int fadc;
 
-	fadc = st->channel_config[channel].odr;
+	fadc = st->channels[channel].cfg->odr;
 
-	switch (st->channel_config[channel].filter_type) {
+	switch (st->channels[channel].cfg->filter_type) {
 	case AD7124_SINC3_FILTER:
 		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
 	case AD7124_SINC4_FILTER:
@@ -349,11 +361,11 @@ static int ad7124_set_3db_filter_freq(struct ad7124_state *st,
 		new_odr = sinc3_3db_odr;
 	}
 
-	if (st->channel_config[channel].filter_type != new_filter) {
+	if (st->channels[channel].cfg->filter_type != new_filter) {
 		int ret;
 
-		st->channel_config[channel].filter_type = new_filter;
-		ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
+		st->channels[channel].cfg->filter_type = new_filter;
+		ret = ad7124_spi_write_mask(st, AD7124_FILTER(st->channels[channel].cfg->nr),
 					    AD7124_FILTER_TYPE_MSK,
 					    AD7124_FILTER_TYPE_SEL(new_filter),
 					    3);
@@ -380,30 +392,30 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
 		/* After the conversion is performed, disable the channel */
 		ret = ad_sd_write_reg(&st->sd,
 				      AD7124_CHANNEL(chan->address), 2,
-				      st->channel_config[chan->address].ain |
+				      st->channels[chan->address].ain |
 				      AD7124_CHANNEL_EN(0));
 		if (ret < 0)
 			return ret;
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		idx = st->channel_config[chan->address].pga_bits;
-		*val = st->channel_config[chan->address].vref_mv;
-		if (st->channel_config[chan->address].bipolar)
+		idx = st->channels[chan->address].cfg->pga_bits;
+		*val = st->channels[chan->address].cfg->vref_mv;
+		if (st->channels[chan->address].cfg->bipolar)
 			*val2 = chan->scan_type.realbits - 1 + idx;
 		else
 			*val2 = chan->scan_type.realbits + idx;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		if (st->channel_config[chan->address].bipolar)
+		if (st->channels[chan->address].cfg->bipolar)
 			*val = -(1 << (chan->scan_type.realbits - 1));
 		else
 			*val = 0;
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = st->channel_config[chan->address].odr;
+		*val = st->channels[chan->address].cfg->odr;
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
@@ -431,12 +443,12 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
 		if (val != 0)
 			return -EINVAL;
 
-		if (st->channel_config[chan->address].bipolar)
+		if (st->channels[chan->address].cfg->bipolar)
 			full_scale = 1 << (chan->scan_type.realbits - 1);
 		else
 			full_scale = 1 << chan->scan_type.realbits;
 
-		vref = st->channel_config[chan->address].vref_mv * 1000000LL;
+		vref = st->channels[chan->address].cfg->vref_mv * 1000000LL;
 		res = DIV_ROUND_CLOSEST(vref, full_scale);
 		gain = DIV_ROUND_CLOSEST(res, val2);
 
@@ -550,7 +562,7 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
 static int ad7124_init_channel_vref(struct ad7124_state *st,
 				    unsigned int channel_number)
 {
-	unsigned int refsel = st->channel_config[channel_number].refsel;
+	unsigned int refsel = st->channels[channel_number].cfg->refsel;
 
 	switch (refsel) {
 	case AD7124_REFIN1:
@@ -562,13 +574,13 @@ static int ad7124_init_channel_vref(struct ad7124_state *st,
 				ad7124_ref_names[refsel]);
 			return PTR_ERR(st->vref[refsel]);
 		}
-		st->channel_config[channel_number].vref_mv =
+		st->channels[channel_number].cfg->vref_mv =
 			regulator_get_voltage(st->vref[refsel]);
 		/* Conversion from uV to mV */
-		st->channel_config[channel_number].vref_mv /= 1000;
+		st->channels[channel_number].cfg->vref_mv /= 1000;
 		break;
 	case AD7124_INT_REF:
-		st->channel_config[channel_number].vref_mv = 2500;
+		st->channels[channel_number].cfg->vref_mv = 2500;
 		st->adc_control &= ~AD7124_ADC_CTRL_REF_EN_MSK;
 		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
 		return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL,
@@ -587,14 +599,40 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 	struct ad7124_state *st = iio_priv(indio_dev);
 	struct device_node *child;
 	struct iio_chan_spec *chan;
-	struct ad7124_channel_config *chan_config;
-	unsigned int ain[2], channel = 0, tmp;
+	unsigned int ain[2], config_nr = 0, channel = 0, tmp;
 	int ret;
 
-	st->num_channels = of_get_available_child_count(np);
-	if (!st->num_channels) {
-		dev_err(indio_dev->dev.parent, "no channel children\n");
-		return -ENODEV;
+	/* parse configuration nodes */
+	for_each_available_child_of_node(np, child) {
+		ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
+		if (!ret) {
+			st->num_channels++;
+			continue;
+		}
+
+		if (ret == -EINVAL) {
+			ret = of_property_read_u32(child, "reg", &config_nr);
+			if (ret)
+				goto err;
+
+			config_nr -= AD7124_CONF_ADDR_OFFSET;
+			st->configs[config_nr].enable = true;
+			st->configs[config_nr].nr = config_nr;
+			st->configs[config_nr].bipolar = of_property_read_bool(child, "bipolar");
+
+			ret = of_property_read_u32(child, "adi,reference-select", &tmp);
+			if (ret)
+				st->configs[config_nr].refsel = AD7124_INT_REF;
+			else
+				st->configs[config_nr].refsel = tmp;
+
+			st->configs[config_nr].buf_positive =
+				of_property_read_bool(child, "adi,buffered-positive");
+			st->configs[config_nr].buf_negative =
+				of_property_read_bool(child, "adi,buffered-negative");
+		} else {
+			goto err;
+		}
 	}
 
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
@@ -602,46 +640,43 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 	if (!chan)
 		return -ENOMEM;
 
-	chan_config = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
-				   sizeof(*chan_config), GFP_KERNEL);
-	if (!chan_config)
-		return -ENOMEM;
-
 	indio_dev->channels = chan;
 	indio_dev->num_channels = st->num_channels;
-	st->channel_config = chan_config;
 
+	/* parse channel nodes */
 	for_each_available_child_of_node(np, child) {
-		ret = of_property_read_u32(child, "reg", &channel);
-		if (ret)
-			goto err;
-
-		ret = of_property_read_u32_array(child, "diff-channels",
-						 ain, 2);
-		if (ret)
-			goto err;
-
-		st->channel_config[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
-						  AD7124_CHANNEL_AINM(ain[1]);
-		st->channel_config[channel].bipolar =
-			of_property_read_bool(child, "bipolar");
-
-		ret = of_property_read_u32(child, "adi,reference-select", &tmp);
-		if (ret)
-			st->channel_config[channel].refsel = AD7124_INT_REF;
-		else
-			st->channel_config[channel].refsel = tmp;
-
-		st->channel_config[channel].buf_positive =
-			of_property_read_bool(child, "adi,buffered-positive");
-		st->channel_config[channel].buf_negative =
-			of_property_read_bool(child, "adi,buffered-negative");
-
-		chan[channel] = ad7124_channel_template;
-		chan[channel].address = channel;
-		chan[channel].scan_index = channel;
-		chan[channel].channel = ain[0];
-		chan[channel].channel2 = ain[1];
+		ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
+		if (!ret) {
+			ret = of_property_read_u32(child, "reg", &channel);
+			if (ret)
+				goto err;
+
+			ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
+			if (ret)
+				goto err;
+
+			st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
+						    AD7124_CHANNEL_AINM(ain[1]);
+
+			ret = of_property_read_u32(child, "adi,configuration", &config_nr);
+			if (ret)
+				goto err;
+
+			config_nr -= AD7124_CONF_ADDR_OFFSET;
+			if (!st->configs[config_nr].enable) {
+				dev_err(&st->sd.spi->dev, "Configuration %u not specified in DT.\n",
+					config_nr);
+				return -EINVAL;
+			}
+
+			st->channels[channel].cfg = &st->configs[config_nr];
+
+			chan[channel] = ad7124_channel_template;
+			chan[channel].address = channel;
+			chan[channel].scan_index = channel;
+			chan[channel].channel = ain[0];
+			chan[channel].channel2 = ain[1];
+		}
 	}
 
 	return 0;
@@ -678,7 +713,7 @@ static int ad7124_setup(struct ad7124_state *st)
 		return ret;
 
 	for (i = 0; i < st->num_channels; i++) {
-		val = st->channel_config[i].ain | AD7124_CHANNEL_SETUP(i);
+		val = st->channels[i].ain | AD7124_CHANNEL_SETUP(i);
 		ret = ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, val);
 		if (ret < 0)
 			return ret;
@@ -687,13 +722,13 @@ static int ad7124_setup(struct ad7124_state *st)
 		if (ret < 0)
 			return ret;
 
-		tmp = (st->channel_config[i].buf_positive << 1)  +
-			st->channel_config[i].buf_negative;
+		tmp = (st->channels[i].cfg->buf_positive << 1)  +
+			st->channels[i].cfg->buf_negative;
 
-		val = AD7124_CONFIG_BIPOLAR(st->channel_config[i].bipolar) |
-		      AD7124_CONFIG_REF_SEL(st->channel_config[i].refsel) |
+		val = AD7124_CONFIG_BIPOLAR(st->channels[i].cfg->bipolar) |
+		      AD7124_CONFIG_REF_SEL(st->channels[i].cfg->refsel) |
 		      AD7124_CONFIG_IN_BUFF(tmp);
-		ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(i), 2, val);
+		ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(st->channels[i].cfg->nr), 2, val);
 		if (ret < 0)
 			return ret;
 		/*
-- 
2.20.1

[PATCH v2 2/2] dt-bindings: iio: adc: ad7124: add config nodes

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Document use of configurations in device-tree bindings.

Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 .../bindings/iio/adc/adi,ad7124.yaml          | 72 +++++++++++++++----
 1 file changed, 57 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
index fb3d0dae9bae..330064461d0a 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
@@ -62,20 +62,19 @@ required:
   - interrupts
 
 patternProperties:
-  "^channel@([0-9]|1[0-5])$":
-    $ref: "adc.yaml"
+  "^config@(2[0-7])$":
     type: object
     description: |
-      Represents the external channels which are connected to the ADC.
+      Represents a channel configuration.
+      See Documentation/devicetree/bindings/iio/adc/adc.txt.
 
     properties:
       reg:
         description: |
-          The channel number. It can have up to 8 channels on ad7124-4
-          and 16 channels on ad7124-8, numbered from 0 to 15.
+          The config number. It can have up to 8 configuration.
         items:
-          minimum: 0
-          maximum: 15
+         minimum: 20
+         maximum: 27
 
       adi,reference-select:
         description: |
@@ -88,8 +87,6 @@ patternProperties:
         $ref: /schemas/types.yaml#/definitions/uint32
         enum: [0, 1, 3]
 
-      diff-channels: true
-
       bipolar: true
 
       adi,buffered-positive:
@@ -100,6 +97,35 @@ patternProperties:
         description: Enable buffered mode for negative input.
         type: boolean
 
+    additionalProperties: false
+
+  "^channel@([0-9]|1[0-5])$":
+    type: object
+    description: |
+      Represents the external channels which are connected to the ADC.
+      See Documentation/devicetree/bindings/iio/adc/adc.txt.
+
+    properties:
+      reg:
+        description: |
+          The channel number. It can have up to 8 channels on ad7124-4
+          and 16 channels on ad7124-8, numbered from 0 to 15.
+        items:
+         minimum: 0
+         maximum: 15
+
+      diff-channels: true
+
+      adi,configuration:
+        description: |
+          The devices has 8 configuration and ad7124-8 support up to 16 unipolar channels.
+          Each channel can be assigned one configuration. Some channels will be sharing the
+          same configuration.
+        allOf:
+          - $ref: /schemas/types.yaml#/definitions/uint32
+        minimum: 20
+        maximum: 27
+
     required:
       - reg
       - diff-channels
@@ -127,30 +153,46 @@ examples:
         #address-cells = <1>;
         #size-cells = <0>;
 
-        channel@0 {
-          reg = <0>;
-          diff-channels = <0 1>;
+        config@20 {
+          reg = <20>;
           adi,reference-select = <0>;
           adi,buffered-positive;
         };
 
-        channel@1 {
-          reg = <1>;
+        config@21 {
+          reg = <21>;
           bipolar;
-          diff-channels = <2 3>;
           adi,reference-select = <0>;
           adi,buffered-positive;
           adi,buffered-negative;
         };
 
+        config@22 {
+          reg = <22>;
+        };
+
+        channel@0 {
+          reg = <0>;
+          diff-channels = <0 1>;
+          adi,configuration = <20>;
+        };
+
+        channel@1 {
+          reg = <1>;
+          diff-channels = <2 3>;
+          adi,configuration = <21>;
+        };
+
         channel@2 {
           reg = <2>;
           diff-channels = <4 5>;
+          adi,configuration = <22>;
         };
 
         channel@3 {
           reg = <3>;
           diff-channels = <6 7>;
+          adi,configuration = <22>;
         };
       };
     };
-- 
2.20.1

Re: [PATCH v2 2/2] dt-bindings: iio: adc: ad7124: add config nodes

[Rob Herring]

On Thu, 04 Feb 2021 13:35:51 +0200, alexandru.tachici@analog.com wrote:
> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Document use of configurations in device-tree bindings.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7124.yaml          | 72 +++++++++++++++----
>  1 file changed, 57 insertions(+), 15 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml:76:10: [warning] wrong indentation: expected 10 but found 9 (indentation)
./Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml:114:10: [warning] wrong indentation: expected 10 but found 9 (indentation)

dtschema/dtc warnings/errors:

See https://patchwork.ozlabs.org/patch/1435965

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v2 2/2] dt-bindings: iio: adc: ad7124: add config nodes

[Jonathan Cameron]

On Thu, 4 Feb 2021 13:35:51 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Document use of configurations in device-tree bindings.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>

Ignoring discussing in my reply to the cover letter...

This is a breaking change as described.  We can't move properties
around without some sort of fullback for them being in the old
location.

> ---
>  .../bindings/iio/adc/adi,ad7124.yaml          | 72 +++++++++++++++----
>  1 file changed, 57 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> index fb3d0dae9bae..330064461d0a 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> @@ -62,20 +62,19 @@ required:
>    - interrupts
>  
>  patternProperties:
> -  "^channel@([0-9]|1[0-5])$":
> -    $ref: "adc.yaml"
> +  "^config@(2[0-7])$":
>      type: object
>      description: |
> -      Represents the external channels which are connected to the ADC.
> +      Represents a channel configuration.
> +      See Documentation/devicetree/bindings/iio/adc/adc.txt.

adc.yaml now.


>  
>      properties:
>        reg:
>          description: |
> -          The channel number. It can have up to 8 channels on ad7124-4
> -          and 16 channels on ad7124-8, numbered from 0 to 15.
> +          The config number. It can have up to 8 configuration.
>          items:
> -          minimum: 0
> -          maximum: 15
> +         minimum: 20
> +         maximum: 27

Number then 0-7 please rather than 20-27.

>  
>        adi,reference-select:
>          description: |
> @@ -88,8 +87,6 @@ patternProperties:
>          $ref: /schemas/types.yaml#/definitions/uint32
>          enum: [0, 1, 3]
>  
> -      diff-channels: true
> -
>        bipolar: true
>  
>        adi,buffered-positive:
> @@ -100,6 +97,35 @@ patternProperties:
>          description: Enable buffered mode for negative input.
>          type: boolean
>  
> +    additionalProperties: false
> +
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +    description: |
> +      Represents the external channels which are connected to the ADC.
> +      See Documentation/devicetree/bindings/iio/adc/adc.txt.
> +
> +    properties:
> +      reg:
> +        description: |
> +          The channel number. It can have up to 8 channels on ad7124-4
> +          and 16 channels on ad7124-8, numbered from 0 to 15.
> +        items:
> +         minimum: 0
> +         maximum: 15
> +
> +      diff-channels: true
> +
> +      adi,configuration:
> +        description: |
> +          The devices has 8 configuration and ad7124-8 support up to 16 unipolar channels.
> +          Each channel can be assigned one configuration. Some channels will be sharing the
> +          same configuration.
> +        allOf:
> +          - $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 20
> +        maximum: 27
> +
>      required:
>        - reg
>        - diff-channels
> @@ -127,30 +153,46 @@ examples:
>          #address-cells = <1>;
>          #size-cells = <0>;
>  
> -        channel@0 {
> -          reg = <0>;
> -          diff-channels = <0 1>;
> +        config@20 {
> +          reg = <20>;
>            adi,reference-select = <0>;
>            adi,buffered-positive;
>          };
>  
> -        channel@1 {
> -          reg = <1>;
> +        config@21 {
> +          reg = <21>;
>            bipolar;
> -          diff-channels = <2 3>;
>            adi,reference-select = <0>;
>            adi,buffered-positive;
>            adi,buffered-negative;
>          };
>  
> +        config@22 {
> +          reg = <22>;
> +        };
> +
> +        channel@0 {
> +          reg = <0>;
> +          diff-channels = <0 1>;
> +          adi,configuration = <20>;
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <2 3>;
> +          adi,configuration = <21>;
> +        };
> +
>          channel@2 {
>            reg = <2>;
>            diff-channels = <4 5>;
> +          adi,configuration = <22>;
>          };
>  
>          channel@3 {
>            reg = <3>;
>            diff-channels = <6 7>;
> +          adi,configuration = <22>;
>          };
>        };
>      };

Re: [PATCH v2 0/2] iio: adc: ad7124: allow 16 channels

[Jonathan Cameron]

On Thu, 4 Feb 2021 13:35:49 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> AD7124-8 can have up to 16 pseudo-differential channels
> enabled simultaneously and only 8 configurations. 

I'm curious.  Datasheet says up to 15 pseudo differential
channels.  I can't immediate see why though!

> In this
> scenario we cannot assign one configuration per channel,
> some channels will have to share configurations like, ODR,
> gain and filter parameters.
> 
> Allow the user to specify channels and configurations
> separately in device-tree and assign, if needed, the same
> configuration to multiple channels.
> 
> If two channels share the configuration changing the
> sampling rate of one will change the sampling rate of the
> other too.

I can see why you want to do this via device tree to keep
things simple, but it does sort of feel like it might be
something we 'should' be configuring from userspace.
This is unlike the whether the channels should be differential
or not in the first place (which is mostly down to what they
are wired up to).  It's a little bit of a mess as some things
like the reference are in the 8 config registers and we
already have those in the per channel description.

I'm trying to get my head around how we might do this in
the ideal case.

So what we'd want would be:
1) Userspace to not have to care about this restriction if
   whatever it requests is 'possible'.
2) If multiple channels have the same config, then we'd want
   to 'pack' them into the same configuration register.
3) Be as flexible as possible.

The problem is we don't have any way in the ABI of making
atomic adjustments to multiple parameters, but perhaps it
'could' be made to work as follows.

1) All settings go through a level of indirection - i.e. we
   cache them in the driver and only apply them as needed.
2) Sysfs raw reads work on a basis of LRU + packing where we
   can.  (or we just use one config and pay the cost of
   switching every time)
3) Buffered reads.   We check whether the requested
configuration of all channels is possible and set it up if
so.  Otherwise we return -EBUSY or similar to indicate
that what is currently requested can't be done.
Hence we make it atomic at the point of enable.

Most of the time, I'm going to assume that users either have
a carefully crafted optimum setup for their system in which
case they can read the datasheet to figure out what is possible
or they want to have a small number of groups of channels
where it makes sense to replicate configuration because they
are reading the 'same sort of thing'.

Perhaps just pushing all this to dt is fine, but I'm not that
keep on it if there is a reasonable way to avoid it.

Things like filter selections and calibration offsets shouldn't
be influenced by DT (by putting them in shared groups like
this series does).

These are weirdly flexible devices (what fun!) :)

Jonathan

> 
> Alexandru Tachici (2):
>   iio: adc: ad7124: allow 16 channels
>   dt-bindings: iio: adc: ad7124: add config nodes
> 
>  .../bindings/iio/adc/adi,ad7124.yaml          |  72 +++++--
>  drivers/iio/adc/ad7124.c                      | 183 +++++++++++-------
>  2 files changed, 166 insertions(+), 89 deletions(-)
> 

Re: [PATCH v2 1/2] iio: adc: ad7124: allow 16 channels

[Jonathan Cameron]

On Thu, 4 Feb 2021 13:35:50 +0200
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> AD7124-8 can have up to 16 pseudo-differential channels
> enabled simultaneously and only 8 configurations. In this
> scenario we cannot assign one configuration per channel,
> some channels will have to share configurations like, ODR,
> gain and filter parameters.
> 
> This patch allows the user to specify channels and configurations
> separately in device-tree and assign, if needed, the same
> configuration to multiple channels.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
A couple of minor comments, but the big question is whether this is a
good approach to take in general - discussion on that in reply
to the cover letter.

Jonathan

> ---
>  drivers/iio/adc/ad7124.c | 183 +++++++++++++++++++++++----------------
>  1 file changed, 109 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 766c73333604..0df88bea336f 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -86,6 +86,12 @@
>  #define AD7124_SINC3_FILTER 2
>  #define AD7124_SINC4_FILTER 0
>  
> +#define AD7124_CONF_ADDR_OFFSET	20
> +#define AD7124_MAX_CONFIGS	8
> +#define AD7124_MAX_CHANNELS	16
> +
> +#define AD7124_REG_NO 57

What's this for?

> +
>  enum ad7124_ids {
>  	ID_AD7124_4,
>  	ID_AD7124_8,
> @@ -136,21 +142,28 @@ struct ad7124_chip_info {
>  };
>  
>  struct ad7124_channel_config {
> +	bool enable;
> +	unsigned int nr;
>  	enum ad7124_ref_sel refsel;
>  	bool bipolar;
>  	bool buf_positive;
>  	bool buf_negative;
> -	unsigned int ain;
>  	unsigned int vref_mv;
>  	unsigned int pga_bits;
>  	unsigned int odr;
>  	unsigned int filter_type;
>  };
>  
> +struct ad7124_channel {
> +	struct ad7124_channel_config *cfg;
> +	unsigned int ain;
> +};
> +
>  struct ad7124_state {
>  	const struct ad7124_chip_info *chip_info;
>  	struct ad_sigma_delta sd;
> -	struct ad7124_channel_config *channel_config;
> +	struct ad7124_channel channels[AD7124_MAX_CHANNELS];
> +	struct ad7124_channel_config configs[AD7124_MAX_CONFIGS];
>  	struct regulator *vref[4];
>  	struct clk *mclk;
>  	unsigned int adc_control;
> @@ -243,8 +256,8 @@ static int ad7124_set_channel(struct ad_sigma_delta *sd, unsigned int channel)
>  	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
>  	unsigned int val;
>  
> -	val = st->channel_config[channel].ain | AD7124_CHANNEL_EN(1) |
> -	      AD7124_CHANNEL_SETUP(channel);
> +	val = st->channels[channel].ain | AD7124_CHANNEL_EN(1) |
> +	      AD7124_CHANNEL_SETUP(st->channels[channel].cfg->nr);
>  
>  	return ad_sd_write_reg(&st->sd, AD7124_CHANNEL(channel), 2, val);
>  }
> @@ -280,14 +293,13 @@ static int ad7124_set_channel_odr(struct ad7124_state *st,
>  	else if (odr_sel_bits > 2047)
>  		odr_sel_bits = 2047;
>  
> -	ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
> +	ret = ad7124_spi_write_mask(st, AD7124_FILTER(st->channels[channel].cfg->nr),
>  				    AD7124_FILTER_FS_MSK,
>  				    AD7124_FILTER_FS(odr_sel_bits), 3);
>  	if (ret < 0)
>  		return ret;
>  	/* fADC = fCLK / (FS[10:0] x 32) */
> -	st->channel_config[channel].odr =
> -		DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
> +	st->channels[channel].cfg->odr = DIV_ROUND_CLOSEST(fclk, odr_sel_bits * 32);
>  
>  	return 0;
>  }
> @@ -301,13 +313,13 @@ static int ad7124_set_channel_gain(struct ad7124_state *st,
>  
>  	res = ad7124_find_closest_match(ad7124_gain,
>  					ARRAY_SIZE(ad7124_gain), gain);
> -	ret = ad7124_spi_write_mask(st, AD7124_CONFIG(channel),
> +	ret = ad7124_spi_write_mask(st, AD7124_CONFIG(st->channels[channel].cfg->nr),
>  				    AD7124_CONFIG_PGA_MSK,
>  				    AD7124_CONFIG_PGA(res), 2);
>  	if (ret < 0)
>  		return ret;
>  
> -	st->channel_config[channel].pga_bits = res;
> +	st->channels[channel].cfg->pga_bits = res;
>  
>  	return 0;
>  }
> @@ -317,9 +329,9 @@ static int ad7124_get_3db_filter_freq(struct ad7124_state *st,
>  {
>  	unsigned int fadc;
>  
> -	fadc = st->channel_config[channel].odr;
> +	fadc = st->channels[channel].cfg->odr;
>  
> -	switch (st->channel_config[channel].filter_type) {
> +	switch (st->channels[channel].cfg->filter_type) {
>  	case AD7124_SINC3_FILTER:
>  		return DIV_ROUND_CLOSEST(fadc * 230, 1000);
>  	case AD7124_SINC4_FILTER:
> @@ -349,11 +361,11 @@ static int ad7124_set_3db_filter_freq(struct ad7124_state *st,
>  		new_odr = sinc3_3db_odr;
>  	}
>  
> -	if (st->channel_config[channel].filter_type != new_filter) {
> +	if (st->channels[channel].cfg->filter_type != new_filter) {
>  		int ret;
>  
> -		st->channel_config[channel].filter_type = new_filter;
> -		ret = ad7124_spi_write_mask(st, AD7124_FILTER(channel),
> +		st->channels[channel].cfg->filter_type = new_filter;
> +		ret = ad7124_spi_write_mask(st, AD7124_FILTER(st->channels[channel].cfg->nr),
>  					    AD7124_FILTER_TYPE_MSK,
>  					    AD7124_FILTER_TYPE_SEL(new_filter),
>  					    3);
> @@ -380,30 +392,30 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
>  		/* After the conversion is performed, disable the channel */
>  		ret = ad_sd_write_reg(&st->sd,
>  				      AD7124_CHANNEL(chan->address), 2,
> -				      st->channel_config[chan->address].ain |
> +				      st->channels[chan->address].ain |
>  				      AD7124_CHANNEL_EN(0));
>  		if (ret < 0)
>  			return ret;
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		idx = st->channel_config[chan->address].pga_bits;
> -		*val = st->channel_config[chan->address].vref_mv;
> -		if (st->channel_config[chan->address].bipolar)
> +		idx = st->channels[chan->address].cfg->pga_bits;
> +		*val = st->channels[chan->address].cfg->vref_mv;
> +		if (st->channels[chan->address].cfg->bipolar)
>  			*val2 = chan->scan_type.realbits - 1 + idx;
>  		else
>  			*val2 = chan->scan_type.realbits + idx;
>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_OFFSET:
> -		if (st->channel_config[chan->address].bipolar)
> +		if (st->channels[chan->address].cfg->bipolar)
>  			*val = -(1 << (chan->scan_type.realbits - 1));
>  		else
>  			*val = 0;
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*val = st->channel_config[chan->address].odr;
> +		*val = st->channels[chan->address].cfg->odr;
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> @@ -431,12 +443,12 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
>  		if (val != 0)
>  			return -EINVAL;
>  
> -		if (st->channel_config[chan->address].bipolar)
> +		if (st->channels[chan->address].cfg->bipolar)
>  			full_scale = 1 << (chan->scan_type.realbits - 1);
>  		else
>  			full_scale = 1 << chan->scan_type.realbits;
>  
> -		vref = st->channel_config[chan->address].vref_mv * 1000000LL;
> +		vref = st->channels[chan->address].cfg->vref_mv * 1000000LL;
>  		res = DIV_ROUND_CLOSEST(vref, full_scale);
>  		gain = DIV_ROUND_CLOSEST(res, val2);
>  
> @@ -550,7 +562,7 @@ static int ad7124_check_chip_id(struct ad7124_state *st)
>  static int ad7124_init_channel_vref(struct ad7124_state *st,
>  				    unsigned int channel_number)
>  {
> -	unsigned int refsel = st->channel_config[channel_number].refsel;
> +	unsigned int refsel = st->channels[channel_number].cfg->refsel;
>  
>  	switch (refsel) {
>  	case AD7124_REFIN1:
> @@ -562,13 +574,13 @@ static int ad7124_init_channel_vref(struct ad7124_state *st,
>  				ad7124_ref_names[refsel]);
>  			return PTR_ERR(st->vref[refsel]);
>  		}
> -		st->channel_config[channel_number].vref_mv =
> +		st->channels[channel_number].cfg->vref_mv =
>  			regulator_get_voltage(st->vref[refsel]);
>  		/* Conversion from uV to mV */
> -		st->channel_config[channel_number].vref_mv /= 1000;
> +		st->channels[channel_number].cfg->vref_mv /= 1000;
>  		break;
>  	case AD7124_INT_REF:
> -		st->channel_config[channel_number].vref_mv = 2500;
> +		st->channels[channel_number].cfg->vref_mv = 2500;
>  		st->adc_control &= ~AD7124_ADC_CTRL_REF_EN_MSK;
>  		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
>  		return ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL,
> @@ -587,14 +599,40 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
>  	struct ad7124_state *st = iio_priv(indio_dev);
>  	struct device_node *child;
>  	struct iio_chan_spec *chan;
> -	struct ad7124_channel_config *chan_config;
> -	unsigned int ain[2], channel = 0, tmp;
> +	unsigned int ain[2], config_nr = 0, channel = 0, tmp;
>  	int ret;
>  
> -	st->num_channels = of_get_available_child_count(np);
> -	if (!st->num_channels) {
> -		dev_err(indio_dev->dev.parent, "no channel children\n");
> -		return -ENODEV;
> +	/* parse configuration nodes */
> +	for_each_available_child_of_node(np, child) {
> +		ret = of_property_read_u32_array(child, "diff-channels", ain, 2);

Can we do something based on the child node name rather than a field potentially
within it?

> +		if (!ret) {
> +			st->num_channels++;
> +			continue;
> +		}
> +
> +		if (ret == -EINVAL) {
> +			ret = of_property_read_u32(child, "reg", &config_nr);
> +			if (ret)
> +				goto err;
> +
> +			config_nr -= AD7124_CONF_ADDR_OFFSET;
> +			st->configs[config_nr].enable = true;
> +			st->configs[config_nr].nr = config_nr;
> +			st->configs[config_nr].bipolar = of_property_read_bool(child, "bipolar");
> +
> +			ret = of_property_read_u32(child, "adi,reference-select", &tmp);
> +			if (ret)
> +				st->configs[config_nr].refsel = AD7124_INT_REF;
> +			else
> +				st->configs[config_nr].refsel = tmp;
> +
> +			st->configs[config_nr].buf_positive =
> +				of_property_read_bool(child, "adi,buffered-positive");
> +			st->configs[config_nr].buf_negative =
> +				of_property_read_bool(child, "adi,buffered-negative");
> +		} else {
> +			goto err;
> +		}
>  	}
>  
>  	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> @@ -602,46 +640,43 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
>  	if (!chan)
>  		return -ENOMEM;
>  
> -	chan_config = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> -				   sizeof(*chan_config), GFP_KERNEL);
> -	if (!chan_config)
> -		return -ENOMEM;
> -
>  	indio_dev->channels = chan;
>  	indio_dev->num_channels = st->num_channels;
> -	st->channel_config = chan_config;
>  
> +	/* parse channel nodes */
>  	for_each_available_child_of_node(np, child) {
> -		ret = of_property_read_u32(child, "reg", &channel);
> -		if (ret)
> -			goto err;
> -
> -		ret = of_property_read_u32_array(child, "diff-channels",
> -						 ain, 2);
> -		if (ret)
> -			goto err;
> -
> -		st->channel_config[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
> -						  AD7124_CHANNEL_AINM(ain[1]);
> -		st->channel_config[channel].bipolar =
> -			of_property_read_bool(child, "bipolar");
> -
> -		ret = of_property_read_u32(child, "adi,reference-select", &tmp);
> -		if (ret)
> -			st->channel_config[channel].refsel = AD7124_INT_REF;
> -		else
> -			st->channel_config[channel].refsel = tmp;
> -
> -		st->channel_config[channel].buf_positive =
> -			of_property_read_bool(child, "adi,buffered-positive");
> -		st->channel_config[channel].buf_negative =
> -			of_property_read_bool(child, "adi,buffered-negative");
> -
> -		chan[channel] = ad7124_channel_template;
> -		chan[channel].address = channel;
> -		chan[channel].scan_index = channel;
> -		chan[channel].channel = ain[0];
> -		chan[channel].channel2 = ain[1];
> +		ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
> +		if (!ret) {
> +			ret = of_property_read_u32(child, "reg", &channel);
> +			if (ret)
> +				goto err;
> +
> +			ret = of_property_read_u32_array(child, "diff-channels", ain, 2);
> +			if (ret)
> +				goto err;
> +
> +			st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
> +						    AD7124_CHANNEL_AINM(ain[1]);
> +
> +			ret = of_property_read_u32(child, "adi,configuration", &config_nr);
> +			if (ret)
> +				goto err;
> +
> +			config_nr -= AD7124_CONF_ADDR_OFFSET;
> +			if (!st->configs[config_nr].enable) {
> +				dev_err(&st->sd.spi->dev, "Configuration %u not specified in DT.\n",
> +					config_nr);
> +				return -EINVAL;
> +			}
> +
> +			st->channels[channel].cfg = &st->configs[config_nr];
> +
> +			chan[channel] = ad7124_channel_template;
> +			chan[channel].address = channel;
> +			chan[channel].scan_index = channel;
> +			chan[channel].channel = ain[0];
> +			chan[channel].channel2 = ain[1];
> +		}
>  	}
>  
>  	return 0;
> @@ -678,7 +713,7 @@ static int ad7124_setup(struct ad7124_state *st)
>  		return ret;
>  
>  	for (i = 0; i < st->num_channels; i++) {
> -		val = st->channel_config[i].ain | AD7124_CHANNEL_SETUP(i);
> +		val = st->channels[i].ain | AD7124_CHANNEL_SETUP(i);
>  		ret = ad_sd_write_reg(&st->sd, AD7124_CHANNEL(i), 2, val);
>  		if (ret < 0)
>  			return ret;
> @@ -687,13 +722,13 @@ static int ad7124_setup(struct ad7124_state *st)
>  		if (ret < 0)
>  			return ret;
>  
> -		tmp = (st->channel_config[i].buf_positive << 1)  +
> -			st->channel_config[i].buf_negative;
> +		tmp = (st->channels[i].cfg->buf_positive << 1)  +
> +			st->channels[i].cfg->buf_negative;
>  
> -		val = AD7124_CONFIG_BIPOLAR(st->channel_config[i].bipolar) |
> -		      AD7124_CONFIG_REF_SEL(st->channel_config[i].refsel) |
> +		val = AD7124_CONFIG_BIPOLAR(st->channels[i].cfg->bipolar) |
> +		      AD7124_CONFIG_REF_SEL(st->channels[i].cfg->refsel) |
>  		      AD7124_CONFIG_IN_BUFF(tmp);
> -		ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(i), 2, val);
> +		ret = ad_sd_write_reg(&st->sd, AD7124_CONFIG(st->channels[i].cfg->nr), 2, val);
>  		if (ret < 0)
>  			return ret;
>  		/*

Re: [PATCH v2 2/2] dt-bindings: iio: adc: ad7124: add config nodes

[Rob Herring]

On Sat, Feb 6, 2021 at 9:26 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 4 Feb 2021 13:35:51 +0200
> <alexandru.tachici@analog.com> wrote:
>
> > From: Alexandru Tachici <alexandru.tachici@analog.com>
> >
> > Document use of configurations in device-tree bindings.
> >
> > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
>
> Ignoring discussing in my reply to the cover letter...
>
> This is a breaking change as described.  We can't move properties
> around without some sort of fullback for them being in the old
> location.
>
> > ---
> >  .../bindings/iio/adc/adi,ad7124.yaml          | 72 +++++++++++++++----
> >  1 file changed, 57 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> > index fb3d0dae9bae..330064461d0a 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7124.yaml
> > @@ -62,20 +62,19 @@ required:
> >    - interrupts
> >
> >  patternProperties:
> > -  "^channel@([0-9]|1[0-5])$":
> > -    $ref: "adc.yaml"
> > +  "^config@(2[0-7])$":
> >      type: object
> >      description: |
> > -      Represents the external channels which are connected to the ADC.
> > +      Represents a channel configuration.
> > +      See Documentation/devicetree/bindings/iio/adc/adc.txt.
>
> adc.yaml now.
>
>
> >
> >      properties:
> >        reg:
> >          description: |
> > -          The channel number. It can have up to 8 channels on ad7124-4
> > -          and 16 channels on ad7124-8, numbered from 0 to 15.
> > +          The config number. It can have up to 8 configuration.
> >          items:
> > -          minimum: 0
> > -          maximum: 15
> > +         minimum: 20
> > +         maximum: 27
>
> Number then 0-7 please rather than 20-27.

That doesn't work. It would be creating 2 address spaces at one level
with channel@0 and config@0. The way to address this is add a
'configs' node with config@N children.

My question here though is where does 20-27 come from. I suspect it's
made up which isn't good either. Addresses should also be rooted in
something in the h/w.

Rob