[PATCH v2 1/2] dt-bindings: iio: addac: one-bit-adc-dac yaml documentation

[PATCH v2 1/2] dt-bindings: iio: addac: one-bit-adc-dac yaml documentation

[Cristian Pop]

This adds device tree bindings for the one-bit-adc-dac.

Signed-off-by: Cristian Pop <cristian.pop@analog.com>
V1->V2                                                                     
 - I am aware of the recommendation of rename/move this driver. Should we  
   consider "drivers/io/gpio.c"?                                           
 - Add .yaml file                                                          
 - Remove blank lines, remove unnecessary coma                             
 - Remove macros for channels                                              
 - Check if channel is input for write_raw                                 
 - Use labels instead of extend_name                                       
 - Fix channel indexing                                                    
 - Use "sizeof(*channels)" in devm_kcalloc()                               
 - Remove assignment: " indio_dev->dev.parent = &pdev->dev;"               
 - Remove "platform_set_drvdata"                                           
 - Remove "adi" from compatible string since is not ADI specific driver.
---
 .../bindings/iio/addac/one-bit-adc-dac.yaml   | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml

diff --git a/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml b/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
new file mode 100644
index 000000000000..dbed0f3b1ca4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2020 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/addac/one-bit-adc-dac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices one bit ADC DAC driver
+
+maintainers:
+  - Cristian Pop <cristian.pop@analog.com>
+
+description: |
+  One bit ADC DAC driver
+
+properties:
+  compatible:
+    enum:
+      - adi,one-bit-adc-dac
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  in-gpios:
+    description: Input GPIOs
+
+  out-gpios:
+    description: Output GPIOs
+
+required:
+  - compatible
+  - in-gpios
+  - out-gpios
+
+patternProperties:
+  "^channel@([0-9]|1[0-5])$":
+    type: object
+    description: |
+      Represents the external channels which are connected to the ADDAC.
+
+    properties:
+      reg:
+        maxItems: 1
+        description: |
+          The channel number.
+
+      label:
+        description: |
+          Unique name to identify which channel this is.
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    one-bit-adc-dac@0 {
+        compatible = "one-bit-adc-dac";
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        in-gpios = <&gpio 17 0>, <&gpio 27 0>;
+        out-gpios = <&gpio 23 0>, <&gpio 24 0>;
+
+        channel@0 {
+          reg = <0>;
+          label = "i_17";
+        };
+
+        channel@1 {
+          reg = <1>;
+          label = "i_27";
+        };
+
+        channel@2 {
+          reg = <2>;
+          label = "o_23";
+        };
+
+        channel@3 {
+          reg = <3>;
+          label = "o_24";
+        };
+    };
-- 
2.17.1

[PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Cristian Pop]

This allows remote reading and writing of the GPIOs. This is useful in
application that run on another PC, at system level, where multiple iio
devices and GPIO devices are integrated together.

Signed-off-by: Cristian Pop <cristian.pop@analog.com>
---
 drivers/iio/addac/Kconfig           |   8 +
 drivers/iio/addac/Makefile          |   1 +
 drivers/iio/addac/one-bit-adc-dac.c | 229 ++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+)
 create mode 100644 drivers/iio/addac/one-bit-adc-dac.c

diff --git a/drivers/iio/addac/Kconfig b/drivers/iio/addac/Kconfig
index 138492362f20..5f311f4a747e 100644
--- a/drivers/iio/addac/Kconfig
+++ b/drivers/iio/addac/Kconfig
@@ -17,4 +17,12 @@ config AD74413R
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad74413r.
 
+config ONE_BIT_ADC_DAC
+	tristate "ONE_BIT_ADC_DAC driver"
+	help
+	  Say yes here to build support for ONE_BIT_ADC_DAC driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called one-bit-adc-dac.
+
 endmenu
diff --git a/drivers/iio/addac/Makefile b/drivers/iio/addac/Makefile
index cfd4bbe64ad3..0a33f0706b55 100644
--- a/drivers/iio/addac/Makefile
+++ b/drivers/iio/addac/Makefile
@@ -5,3 +5,4 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AD74413R) += ad74413r.o
+obj-$(CONFIG_ONE_BIT_ADC_DAC) += one-bit-adc-dac.o
diff --git a/drivers/iio/addac/one-bit-adc-dac.c b/drivers/iio/addac/one-bit-adc-dac.c
new file mode 100644
index 000000000000..5680de594429
--- /dev/null
+++ b/drivers/iio/addac/one-bit-adc-dac.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+/*
+ * one-bit-adc-dac
+ *
+ * Copyright 2022 Analog Devices Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/iio/iio.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+
+enum ch_direction {
+	CH_IN,
+	CH_OUT,
+};
+
+struct one_bit_adc_dac_state {
+	int			in_num_ch;
+	int			out_num_ch;
+	struct platform_device  *pdev;
+	struct gpio_descs       *in_gpio_descs;
+	struct gpio_descs       *out_gpio_descs;
+	const char		**labels;
+};
+
+static int one_bit_adc_dac_read_raw(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, int *val, int *val2, long info)
+{
+	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+	struct gpio_descs *descs;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->output)
+			descs = st->out_gpio_descs;
+		else
+			descs = st->in_gpio_descs;
+		*val = gpiod_get_value_cansleep(descs->desc[chan->channel]);
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long info)
+{
+	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+	int channel = chan->channel;
+
+	if (!chan->output)
+		return 0;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		gpiod_set_value_cansleep(st->out_gpio_descs->desc[channel], val);
+
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int one_bit_adc_dac_read_label(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, char *label)
+{
+	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+	int ch;
+
+	if (chan->output)
+		ch = chan->channel + st->in_num_ch;
+	else
+		ch = chan->channel;
+
+	return sprintf(label, "%s\n", st->labels[ch]);
+}
+
+static const struct iio_info one_bit_adc_dac_info = {
+	.read_raw = &one_bit_adc_dac_read_raw,
+	.write_raw = &one_bit_adc_dac_write_raw,
+	.read_label = &one_bit_adc_dac_read_label,
+};
+
+static int one_bit_adc_dac_set_ch(struct iio_chan_spec *channels,
+					int num_ch,
+					enum ch_direction direction)
+{
+	int i;
+
+	for (i = 0; i < num_ch; i++) {
+		channels[i].type = IIO_VOLTAGE;
+		channels[i].indexed = 1;
+		channels[i].channel = i;
+		channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+		channels[i].output = direction;
+	}
+
+	return 0;
+}
+
+static int one_bit_adc_dac_set_channel_label(struct iio_dev *indio_dev,
+						struct iio_chan_spec *channels,
+						int num_channels)
+{
+	struct device *device = indio_dev->dev.parent;
+	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+	struct fwnode_handle *fwnode;
+	struct fwnode_handle *child;
+	struct iio_chan_spec *chan;
+	const char *label;
+	int crt_ch = 0, child_num, i = 0;
+
+	fwnode = dev_fwnode(device);
+	child_num = device_get_child_node_count(device);
+
+	st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num, GFP_KERNEL);
+	if (!st->labels)
+		return -ENOMEM;
+
+	i = child_num;
+	fwnode_for_each_child_node(fwnode, child) {
+		if (fwnode_property_read_u32(child, "reg", &crt_ch))
+			continue;
+
+		if (crt_ch >= num_channels)
+			continue;
+
+		if (fwnode_property_read_string(child, "label", &label))
+			continue;
+
+		chan = &channels[crt_ch];
+		st->labels[--i] = label;
+	}
+
+	return 0;
+}
+
+static int one_bit_adc_dac_parse_dt(struct iio_dev *indio_dev)
+{
+	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+	struct iio_chan_spec *channels;
+	int ret, in_num_ch = 0, out_num_ch = 0;
+
+	st->in_gpio_descs = devm_gpiod_get_array_optional(&st->pdev->dev, "in", GPIOD_IN);
+	if (IS_ERR(st->in_gpio_descs))
+		return PTR_ERR(st->in_gpio_descs);
+
+	if (st->in_gpio_descs)
+		in_num_ch = st->in_gpio_descs->ndescs;
+
+	st->out_gpio_descs = devm_gpiod_get_array_optional(&st->pdev->dev, "out", GPIOD_OUT_LOW);
+	if (IS_ERR(st->out_gpio_descs))
+		return PTR_ERR(st->out_gpio_descs);
+
+	if (st->out_gpio_descs)
+		out_num_ch = st->out_gpio_descs->ndescs;
+	st->in_num_ch = in_num_ch;
+	st->out_num_ch = out_num_ch;
+
+	channels = devm_kcalloc(indio_dev->dev.parent, in_num_ch + out_num_ch,
+				sizeof(*channels), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	ret = one_bit_adc_dac_set_ch(&channels[0], in_num_ch, CH_IN);
+	if (ret)
+		return ret;
+
+	ret = one_bit_adc_dac_set_ch(&channels[in_num_ch], out_num_ch, CH_OUT);
+	if (ret)
+		return ret;
+
+	ret = one_bit_adc_dac_set_channel_label(indio_dev, channels, in_num_ch + out_num_ch);
+	if (ret)
+		return ret;
+
+	indio_dev->channels = channels;
+	indio_dev->num_channels = in_num_ch + out_num_ch;
+
+	return 0;
+}
+
+static int one_bit_adc_dac_probe(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev;
+	struct one_bit_adc_dac_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->pdev = pdev;
+	indio_dev->name = "one-bit-adc-dac";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &one_bit_adc_dac_info;
+
+	ret = one_bit_adc_dac_parse_dt(indio_dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
+}
+
+static const struct of_device_id one_bit_adc_dac_dt_match[] = {
+	{ .compatible = "one-bit-adc-dac" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);
+
+static struct platform_driver one_bit_adc_dac_driver = {
+	.driver = {
+		.name = "one-bit-adc-dac",
+		.of_match_table = one_bit_adc_dac_dt_match,
+	},
+	.probe = one_bit_adc_dac_probe,
+};
+module_platform_driver(one_bit_adc_dac_driver);
+
+MODULE_AUTHOR("Cristian Pop <cristian.pop@analog.com>");
+MODULE_DESCRIPTION("One bit ADC DAC converter");
+MODULE_LICENSE("Dual BSD/GPL");
\ No newline at end of file
-- 
2.17.1

Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Andy Shevchenko]

On Wed, Jan 12, 2022 at 11:50 AM Cristian Pop <cristian.pop@analog.com> wrote:
>
> This allows remote reading and writing of the GPIOs. This is useful in
> application that run on another PC, at system level, where multiple iio
> devices and GPIO devices are integrated together.

Should it be called GPIO-IIO proxy or something like that?

...

> +/*

> + * one-bit-adc-dac
> + *

These two lines make no sense.

> + * Copyright 2022 Analog Devices Inc.
> + */

...

> +enum ch_direction {
> +       CH_IN,
> +       CH_OUT,
> +};

How is it different from the corresponding GPIO flag?

...

> +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> +                           struct iio_chan_spec const *chan,
> +                           int val,
> +                           int val2,
> +                           long info)

Can be compressed to a fewer LOCs.

...

> +       return sprintf(label, "%s\n", st->labels[ch]);

In a few releases the sysfs_emit() is present and must be used.

...

> +       fwnode = dev_fwnode(device);

No need. See below.

...

> +       child_num = device_get_child_node_count(device);

Error checks?

...

> +       st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num, GFP_KERNEL);

You should use devm_kcalloc() instead, it does slightly more than
simple multiplication.

> +       if (!st->labels)
> +               return -ENOMEM;

...

> +       fwnode_for_each_child_node(fwnode, child) {

device_for_each_...

> +               if (fwnode_property_read_u32(child, "reg", &crt_ch))
> +                       continue;
> +
> +               if (crt_ch >= num_channels)
> +                       continue;
> +
> +               if (fwnode_property_read_string(child, "label", &label))
> +                       continue;
> +
> +               chan = &channels[crt_ch];
> +               st->labels[--i] = label;
> +       }

...

> +MODULE_LICENSE("Dual BSD/GPL");
> \ No newline at end of file

Aiaiai.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Jonathan Cameron]

On Tue, 11 Jan 2022 13:59:19 +0200
Cristian Pop <cristian.pop@analog.com> wrote:

> This allows remote reading and writing of the GPIOs. This is useful in
> application that run on another PC, at system level, where multiple iio
> devices and GPIO devices are integrated together.
Hi Cristian,

So I fully understand why this can be useful, but it is a software only
construction so I'm not convinced that it makes sense to necessarily
configure it via device tree.   A better fit may well be configfs.
(note I always meant to add configfs controls for iio_hwmon but haven't
found the time to do it yet...)

As it currently stands, doing only polled / sysfs reads this driver doesn't
do enough to justify its existence. You could just do all this in userspace
using the existing gpio interfaces.  So to be useful I'd expect it to
at least do triggered buffer capture.

When we have talked about handling digital signals int he past we discussed
whether the IIO channel description would also benefit from tighter packing
than the current minimum of a byte per channel.  Perhaps we don't technically
'need' it here but if we do add it in future it will be hard to retrofit into
this driver without big userspace ABI changes (i.e. breaking all existing
users).

I've not repeated stuff Andy got it in his review (assuming
I remembered it was something Andy raised).

Conclusion:
1) Creation interface needs a rethink or strong argument why it belongs in DT.
2) Driver needs to do more to justify it's existence.

Jonathan

> 
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> ---
>  drivers/iio/addac/Kconfig           |   8 +
>  drivers/iio/addac/Makefile          |   1 +
>  drivers/iio/addac/one-bit-adc-dac.c | 229 ++++++++++++++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
> 
> diff --git a/drivers/iio/addac/Kconfig b/drivers/iio/addac/Kconfig
> index 138492362f20..5f311f4a747e 100644
> --- a/drivers/iio/addac/Kconfig
> +++ b/drivers/iio/addac/Kconfig
> @@ -17,4 +17,12 @@ config AD74413R
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad74413r.
>  
> +config ONE_BIT_ADC_DAC
> +	tristate "ONE_BIT_ADC_DAC driver"
> +	help
> +	  Say yes here to build support for ONE_BIT_ADC_DAC driver.

Needs a lot more detail here.  Though naming it to be explicitly about
GPIO to IIO binding would help.

> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called one-bit-adc-dac.
> +
>  endmenu
> diff --git a/drivers/iio/addac/Makefile b/drivers/iio/addac/Makefile
> index cfd4bbe64ad3..0a33f0706b55 100644
> --- a/drivers/iio/addac/Makefile
> +++ b/drivers/iio/addac/Makefile
> @@ -5,3 +5,4 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AD74413R) += ad74413r.o
> +obj-$(CONFIG_ONE_BIT_ADC_DAC) += one-bit-adc-dac.o
> diff --git a/drivers/iio/addac/one-bit-adc-dac.c b/drivers/iio/addac/one-bit-adc-dac.c
> new file mode 100644
> index 000000000000..5680de594429
> --- /dev/null
> +++ b/drivers/iio/addac/one-bit-adc-dac.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +/*
> + * one-bit-adc-dac

Improve description here as well.  It's really just a wrapper for
a gpio in an IIO channel, so just say that.  Fine to say the
representation is 1 bit ADC or DAC channels as well, but
I think the GPIO part will be more obvious to the casual reader.

> + *
> + * Copyright 2022 Analog Devices Inc.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/iio/iio.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +
> +enum ch_direction {
> +	CH_IN,
> +	CH_OUT,
> +};
> +
> +struct one_bit_adc_dac_state {
> +	int			in_num_ch;
> +	int			out_num_ch;
> +	struct platform_device  *pdev;

Not used after probe so don't keep it around.

> +	struct gpio_descs       *in_gpio_descs;
> +	struct gpio_descs       *out_gpio_descs;
> +	const char		**labels;
> +};
> +
> +static int one_bit_adc_dac_read_raw(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, int *val, int *val2, long info)
> +{
> +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +	struct gpio_descs *descs;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->output)
> +			descs = st->out_gpio_descs;
> +		else
> +			descs = st->in_gpio_descs;
> +		*val = gpiod_get_value_cansleep(descs->desc[chan->channel]);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val,
> +			    int val2,
> +			    long info)
> +{
> +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +	int channel = chan->channel;
> +
> +	if (!chan->output)
> +		return 0;

-EINVAL;  It's an error that should be reported to userspace..

> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		gpiod_set_value_cansleep(st->out_gpio_descs->desc[channel], val);
> +
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int one_bit_adc_dac_read_label(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, char *label)
> +{
> +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +	int ch;
> +
> +	if (chan->output)
> +		ch = chan->channel + st->in_num_ch;
> +	else
> +		ch = chan->channel;
> +
> +	return sprintf(label, "%s\n", st->labels[ch]);
> +}
> +
> +static const struct iio_info one_bit_adc_dac_info = {
> +	.read_raw = &one_bit_adc_dac_read_raw,
> +	.write_raw = &one_bit_adc_dac_write_raw,
> +	.read_label = &one_bit_adc_dac_read_label,
> +};
> +
> +static int one_bit_adc_dac_set_ch(struct iio_chan_spec *channels,
> +					int num_ch,
> +					enum ch_direction direction)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_ch; i++) {
> +		channels[i].type = IIO_VOLTAGE;
> +		channels[i].indexed = 1;
> +		channels[i].channel = i;
> +		channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +		channels[i].output = direction;
> +	}
> +
> +	return 0;
> +}
> +
> +static int one_bit_adc_dac_set_channel_label(struct iio_dev *indio_dev,
> +						struct iio_chan_spec *channels,
> +						int num_channels)
> +{
> +	struct device *device = indio_dev->dev.parent;
> +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_handle *child;
> +	struct iio_chan_spec *chan;
> +	const char *label;
> +	int crt_ch = 0, child_num, i = 0;
> +
> +	fwnode = dev_fwnode(device);
> +	child_num = device_get_child_node_count(device);
> +
> +	st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num, GFP_KERNEL);
> +	if (!st->labels)
> +		return -ENOMEM;
> +
> +	i = child_num;
> +	fwnode_for_each_child_node(fwnode, child) {
> +		if (fwnode_property_read_u32(child, "reg", &crt_ch))
> +			continue;
> +
> +		if (crt_ch >= num_channels)
> +			continue;
> +
> +		if (fwnode_property_read_string(child, "label", &label))
> +			continue;
> +
> +		chan = &channels[crt_ch];
? Not used.

> +		st->labels[--i] = label;

I've no idea how this works...  Should be looking for the chan->channel
value as that's what your read uses to index.

> +	}
> +
> +	return 0;
> +}
> +
> +static int one_bit_adc_dac_parse_dt(struct iio_dev *indio_dev)
> +{
> +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *channels;
> +	int ret, in_num_ch = 0, out_num_ch = 0;
> +
> +	st->in_gpio_descs = devm_gpiod_get_array_optional(&st->pdev->dev, "in", GPIOD_IN);
> +	if (IS_ERR(st->in_gpio_descs))
> +		return PTR_ERR(st->in_gpio_descs);
> +
> +	if (st->in_gpio_descs)
> +		in_num_ch = st->in_gpio_descs->ndescs;
> +
> +	st->out_gpio_descs = devm_gpiod_get_array_optional(&st->pdev->dev, "out", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->out_gpio_descs))
> +		return PTR_ERR(st->out_gpio_descs);
> +
> +	if (st->out_gpio_descs)
> +		out_num_ch = st->out_gpio_descs->ndescs;
> +	st->in_num_ch = in_num_ch;
> +	st->out_num_ch = out_num_ch;
> +
> +	channels = devm_kcalloc(indio_dev->dev.parent, in_num_ch + out_num_ch,
> +				sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	ret = one_bit_adc_dac_set_ch(&channels[0], in_num_ch, CH_IN);
> +	if (ret)
> +		return ret;
> +
> +	ret = one_bit_adc_dac_set_ch(&channels[in_num_ch], out_num_ch, CH_OUT);
> +	if (ret)
> +		return ret;
> +
> +	ret = one_bit_adc_dac_set_channel_label(indio_dev, channels, in_num_ch + out_num_ch);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = in_num_ch + out_num_ch;
> +
> +	return 0;
> +}
> +
> +static int one_bit_adc_dac_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct one_bit_adc_dac_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->pdev = pdev;
> +	indio_dev->name = "one-bit-adc-dac";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &one_bit_adc_dac_info;
> +
> +	ret = one_bit_adc_dac_parse_dt(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> +}
> +
> +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> +	{ .compatible = "one-bit-adc-dac" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);
> +
> +static struct platform_driver one_bit_adc_dac_driver = {
> +	.driver = {
> +		.name = "one-bit-adc-dac",
> +		.of_match_table = one_bit_adc_dac_dt_match,
> +	},
> +	.probe = one_bit_adc_dac_probe,
> +};
> +module_platform_driver(one_bit_adc_dac_driver);
> +
> +MODULE_AUTHOR("Cristian Pop <cristian.pop@analog.com>");
> +MODULE_DESCRIPTION("One bit ADC DAC converter");
> +MODULE_LICENSE("Dual BSD/GPL");
> \ No newline at end of file

Make sure to eyeball your patches before sending as this sort
of thing should be caught at that stage...

Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Andy Shevchenko]

On Mon, Jan 17, 2022 at 8:41 AM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 11 Jan 2022 13:59:19 +0200
> Cristian Pop <cristian.pop@analog.com> wrote:

> > +     st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num, GFP_KERNEL);
> > +     if (!st->labels)
> > +             return -ENOMEM;
> > +
> > +     i = child_num;
> > +     fwnode_for_each_child_node(fwnode, child) {
> > +             if (fwnode_property_read_u32(child, "reg", &crt_ch))
> > +                     continue;
> > +
> > +             if (crt_ch >= num_channels)
> > +                     continue;
> > +
> > +             if (fwnode_property_read_string(child, "label", &label))
> > +                     continue;
> > +
> > +             chan = &channels[crt_ch];
> ? Not used.
>
> > +             st->labels[--i] = label;

> I've no idea how this works...  Should be looking for the chan->channel
> value as that's what your read uses to index.

It's an implicit memcpy().

> > +     }
> > +
> > +     return 0;
> > +}



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Jonathan Cameron]

On Tue, 18 Jan 2022 19:06:20 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jan 17, 2022 at 8:41 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Tue, 11 Jan 2022 13:59:19 +0200
> > Cristian Pop <cristian.pop@analog.com> wrote:  
> 
> > > +     st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num, GFP_KERNEL);
> > > +     if (!st->labels)
> > > +             return -ENOMEM;
> > > +
> > > +     i = child_num;
> > > +     fwnode_for_each_child_node(fwnode, child) {
> > > +             if (fwnode_property_read_u32(child, "reg", &crt_ch))
> > > +                     continue;
> > > +
> > > +             if (crt_ch >= num_channels)
> > > +                     continue;
> > > +
> > > +             if (fwnode_property_read_string(child, "label", &label))
> > > +                     continue;
> > > +
> > > +             chan = &channels[crt_ch];  
> > ? Not used.
> >  
> > > +             st->labels[--i] = label;  
> 
> > I've no idea how this works...  Should be looking for the chan->channel
> > value as that's what your read uses to index.  
> 
> It's an implicit memcpy().

Not that.  What I don't follow is how it ends up in the right element of
st->labels[] seeing as i has nothing to do with reg which should
be the channel number.  Far as I can see it's setting the labels
in a random order.

Jonathan

> 
> > > +     }
> > > +
> > > +     return 0;
> > > +}  
> 
> 
> 

RE: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Sa, Nuno]

Hi Jonathan,

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 16, 2022 12:52 PM
> To: Pop, Cristian <Cristian.Pop@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org;
> robh+dt@kernel.org
> Subject: Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one
> bit ADC-DAC
> 
> [External]
> 
> On Tue, 11 Jan 2022 13:59:19 +0200
> Cristian Pop <cristian.pop@analog.com> wrote:
> 
> > This allows remote reading and writing of the GPIOs. This is useful in
> > application that run on another PC, at system level, where multiple
> iio
> > devices and GPIO devices are integrated together.
> Hi Cristian,
> 
> So I fully understand why this can be useful, but it is a software only
> construction so I'm not convinced that it makes sense to necessarily
> configure it via device tree.   A better fit may well be configfs.
> (note I always meant to add configfs controls for iio_hwmon but
> haven't
> found the time to do it yet...)
> 
> As it currently stands, doing only polled / sysfs reads this driver doesn't
> do enough to justify its existence. You could just do all this in
> userspace
> using the existing gpio interfaces.  So to be useful I'd expect it to
> at least do triggered buffer capture.
> 
> When we have talked about handling digital signals int he past we
> discussed
> whether the IIO channel description would also benefit from tighter
> packing
> than the current minimum of a byte per channel.  Perhaps we don't

That reminded me about this series [1] Alex tried to upstream. It's slightly
related and this was all about supporting buffered channels with less than
8 bits (IIRC). I didn't went through the all thread but I remember you had some
"questions" about the whole thing. That series is something that I definitely want
to revive at some point (might be that I just need to change things in ADI kernel)
to stop having these custom patches in our tree.

> technically
> 'need' it here but if we do add it in future it will be hard to retrofit into
> this driver without big userspace ABI changes (i.e. breaking all existing
> users).

I think this is the perfect opportunity to add support for it (either with the series
I mentioned or something else) if we ever really want to have this in IIO. 


[1]: https://lore.kernel.org/linux-iio/20200424051818.6408-1-alexandru.ardelean@analog.com/
- Nuno Sá
 

RE: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Pop, Cristian]

Hi Jonathan,

The additional functionality it is a little bit unclear to me.

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Sunday, January 16, 2022 1:52 PM
> To: Pop, Cristian <Cristian.Pop@analog.com>
> Cc: linux-iio@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org
> Subject: Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit
> ADC-DAC
> 
> [External]
> 
> On Tue, 11 Jan 2022 13:59:19 +0200
> Cristian Pop <cristian.pop@analog.com> wrote:
> 
> > This allows remote reading and writing of the GPIOs. This is useful in
> > application that run on another PC, at system level, where multiple
> > iio devices and GPIO devices are integrated together.
> Hi Cristian,
> 
> So I fully understand why this can be useful, but it is a software only
> construction so I'm not convinced that it makes sense to necessarily
> configure it via device tree.   A better fit may well be configfs.
> (note I always meant to add configfs controls for iio_hwmon but haven't
> found the time to do it yet...)
> 
> As it currently stands, doing only polled / sysfs reads this driver doesn't do
> enough to justify its existence. You could just do all this in userspace using
> the existing gpio interfaces.  So to be useful I'd expect it to at least do
> triggered buffer capture.

What do you mean by triggered buffer capture? Should I save previous GPIO
states into a buffer? What should I do with them?

A useful use case that I see:
- Is to register a function callback (in kernel space, maybe user space that
  should be used by a different driver), and called in the interrupt handler
  when the state of an input GPIO changes.
- Output to a GPIO from a buffer using a clock, obtaining a PWM like signal.

> 
> When we have talked about handling digital signals int he past we discussed
> whether the IIO channel description would also benefit from tighter packing
> than the current minimum of a byte per channel.  Perhaps we don't
> technically 'need' it here but if we do add it in future it will be hard to retrofit
> into this driver without big userspace ABI changes (i.e. breaking all existing
> users).
> 
> I've not repeated stuff Andy got it in his review (assuming I remembered it
> was something Andy raised).
> 
> Conclusion:
> 1) Creation interface needs a rethink or strong argument why it belongs in
> DT.
> 2) Driver needs to do more to justify it's existence.
> 
> Jonathan
> 
> >
> > Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> > ---
> >  drivers/iio/addac/Kconfig           |   8 +
> >  drivers/iio/addac/Makefile          |   1 +
> >  drivers/iio/addac/one-bit-adc-dac.c | 229
> > ++++++++++++++++++++++++++++
> >  3 files changed, 238 insertions(+)
> >  create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
> >
> > diff --git a/drivers/iio/addac/Kconfig b/drivers/iio/addac/Kconfig
> > index 138492362f20..5f311f4a747e 100644
> > --- a/drivers/iio/addac/Kconfig
> > +++ b/drivers/iio/addac/Kconfig
> > @@ -17,4 +17,12 @@ config AD74413R
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called ad74413r.
> >
> > +config ONE_BIT_ADC_DAC
> > +	tristate "ONE_BIT_ADC_DAC driver"
> > +	help
> > +	  Say yes here to build support for ONE_BIT_ADC_DAC driver.
> 
> Needs a lot more detail here.  Though naming it to be explicitly about GPIO to
> IIO binding would help.
> 
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called one-bit-adc-dac.
> > +
> >  endmenu
> > diff --git a/drivers/iio/addac/Makefile b/drivers/iio/addac/Makefile
> > index cfd4bbe64ad3..0a33f0706b55 100644
> > --- a/drivers/iio/addac/Makefile
> > +++ b/drivers/iio/addac/Makefile
> > @@ -5,3 +5,4 @@
> >
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_AD74413R) += ad74413r.o
> > +obj-$(CONFIG_ONE_BIT_ADC_DAC) += one-bit-adc-dac.o
> > diff --git a/drivers/iio/addac/one-bit-adc-dac.c
> > b/drivers/iio/addac/one-bit-adc-dac.c
> > new file mode 100644
> > index 000000000000..5680de594429
> > --- /dev/null
> > +++ b/drivers/iio/addac/one-bit-adc-dac.c
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > +/*
> > + * one-bit-adc-dac
> 
> Improve description here as well.  It's really just a wrapper for a gpio in an IIO
> channel, so just say that.  Fine to say the representation is 1 bit ADC or DAC
> channels as well, but I think the GPIO part will be more obvious to the casual
> reader.
> 
> > + *
> > + * Copyright 2022 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/gpio/consumer.h>
> > +
> > +enum ch_direction {
> > +	CH_IN,
> > +	CH_OUT,
> > +};
> > +
> > +struct one_bit_adc_dac_state {
> > +	int			in_num_ch;
> > +	int			out_num_ch;
> > +	struct platform_device  *pdev;
> 
> Not used after probe so don't keep it around.
> 
> > +	struct gpio_descs       *in_gpio_descs;
> > +	struct gpio_descs       *out_gpio_descs;
> > +	const char		**labels;
> > +};
> > +
> > +static int one_bit_adc_dac_read_raw(struct iio_dev *indio_dev,
> > +	const struct iio_chan_spec *chan, int *val, int *val2, long info) {
> > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > +	struct gpio_descs *descs;
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (chan->output)
> > +			descs = st->out_gpio_descs;
> > +		else
> > +			descs = st->in_gpio_descs;
> > +		*val = gpiod_get_value_cansleep(descs->desc[chan-
> >channel]);
> > +
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int val,
> > +			    int val2,
> > +			    long info)
> > +{
> > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > +	int channel = chan->channel;
> > +
> > +	if (!chan->output)
> > +		return 0;
> 
> -EINVAL;  It's an error that should be reported to userspace..
> 
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		gpiod_set_value_cansleep(st->out_gpio_descs-
> >desc[channel], val);
> > +
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int one_bit_adc_dac_read_label(struct iio_dev *indio_dev,
> > +	const struct iio_chan_spec *chan, char *label) {
> > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > +	int ch;
> > +
> > +	if (chan->output)
> > +		ch = chan->channel + st->in_num_ch;
> > +	else
> > +		ch = chan->channel;
> > +
> > +	return sprintf(label, "%s\n", st->labels[ch]); }
> > +
> > +static const struct iio_info one_bit_adc_dac_info = {
> > +	.read_raw = &one_bit_adc_dac_read_raw,
> > +	.write_raw = &one_bit_adc_dac_write_raw,
> > +	.read_label = &one_bit_adc_dac_read_label, };
> > +
> > +static int one_bit_adc_dac_set_ch(struct iio_chan_spec *channels,
> > +					int num_ch,
> > +					enum ch_direction direction)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < num_ch; i++) {
> > +		channels[i].type = IIO_VOLTAGE;
> > +		channels[i].indexed = 1;
> > +		channels[i].channel = i;
> > +		channels[i].info_mask_separate =
> BIT(IIO_CHAN_INFO_RAW);
> > +		channels[i].output = direction;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int one_bit_adc_dac_set_channel_label(struct iio_dev *indio_dev,
> > +						struct iio_chan_spec
> *channels,
> > +						int num_channels)
> > +{
> > +	struct device *device = indio_dev->dev.parent;
> > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > +	struct fwnode_handle *fwnode;
> > +	struct fwnode_handle *child;
> > +	struct iio_chan_spec *chan;
> > +	const char *label;
> > +	int crt_ch = 0, child_num, i = 0;
> > +
> > +	fwnode = dev_fwnode(device);
> > +	child_num = device_get_child_node_count(device);
> > +
> > +	st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num,
> GFP_KERNEL);
> > +	if (!st->labels)
> > +		return -ENOMEM;
> > +
> > +	i = child_num;
> > +	fwnode_for_each_child_node(fwnode, child) {
> > +		if (fwnode_property_read_u32(child, "reg", &crt_ch))
> > +			continue;
> > +
> > +		if (crt_ch >= num_channels)
> > +			continue;
> > +
> > +		if (fwnode_property_read_string(child, "label", &label))
> > +			continue;
> > +
> > +		chan = &channels[crt_ch];
> ? Not used.
> 
> > +		st->labels[--i] = label;
> 
> I've no idea how this works...  Should be looking for the chan->channel value
> as that's what your read uses to index.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int one_bit_adc_dac_parse_dt(struct iio_dev *indio_dev) {
> > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > +	struct iio_chan_spec *channels;
> > +	int ret, in_num_ch = 0, out_num_ch = 0;
> > +
> > +	st->in_gpio_descs = devm_gpiod_get_array_optional(&st->pdev-
> >dev, "in", GPIOD_IN);
> > +	if (IS_ERR(st->in_gpio_descs))
> > +		return PTR_ERR(st->in_gpio_descs);
> > +
> > +	if (st->in_gpio_descs)
> > +		in_num_ch = st->in_gpio_descs->ndescs;
> > +
> > +	st->out_gpio_descs = devm_gpiod_get_array_optional(&st->pdev-
> >dev, "out", GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->out_gpio_descs))
> > +		return PTR_ERR(st->out_gpio_descs);
> > +
> > +	if (st->out_gpio_descs)
> > +		out_num_ch = st->out_gpio_descs->ndescs;
> > +	st->in_num_ch = in_num_ch;
> > +	st->out_num_ch = out_num_ch;
> > +
> > +	channels = devm_kcalloc(indio_dev->dev.parent, in_num_ch +
> out_num_ch,
> > +				sizeof(*channels), GFP_KERNEL);
> > +	if (!channels)
> > +		return -ENOMEM;
> > +
> > +	ret = one_bit_adc_dac_set_ch(&channels[0], in_num_ch, CH_IN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = one_bit_adc_dac_set_ch(&channels[in_num_ch],
> out_num_ch, CH_OUT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = one_bit_adc_dac_set_channel_label(indio_dev, channels,
> in_num_ch + out_num_ch);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->channels = channels;
> > +	indio_dev->num_channels = in_num_ch + out_num_ch;
> > +
> > +	return 0;
> > +}
> > +
> > +static int one_bit_adc_dac_probe(struct platform_device *pdev) {
> > +	struct iio_dev *indio_dev;
> > +	struct one_bit_adc_dac_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->pdev = pdev;
> > +	indio_dev->name = "one-bit-adc-dac";
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &one_bit_adc_dac_info;
> > +
> > +	ret = one_bit_adc_dac_parse_dt(indio_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(indio_dev->dev.parent,
> indio_dev); }
> > +
> > +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> > +	{ .compatible = "one-bit-adc-dac" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);
> > +
> > +static struct platform_driver one_bit_adc_dac_driver = {
> > +	.driver = {
> > +		.name = "one-bit-adc-dac",
> > +		.of_match_table = one_bit_adc_dac_dt_match,
> > +	},
> > +	.probe = one_bit_adc_dac_probe,
> > +};
> > +module_platform_driver(one_bit_adc_dac_driver);
> > +
> > +MODULE_AUTHOR("Cristian Pop <cristian.pop@analog.com>");
> > +MODULE_DESCRIPTION("One bit ADC DAC converter");
> MODULE_LICENSE("Dual
> > +BSD/GPL");
> > \ No newline at end of file
> 
> Make sure to eyeball your patches before sending as this sort of thing should
> be caught at that stage...
> 

Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit ADC-DAC

[Jonathan Cameron]

On Fri, 21 Jan 2022 10:24:18 +0000
"Pop, Cristian" <Cristian.Pop@analog.com> wrote:

> Hi Jonathan,
> 
> The additional functionality it is a little bit unclear to me.
> 
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, January 16, 2022 1:52 PM
> > To: Pop, Cristian <Cristian.Pop@analog.com>
> > Cc: linux-iio@vger.kernel.org; linux-gpio@vger.kernel.org; linux-
> > kernel@vger.kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org
> > Subject: Re: [PATCH v2 2/2] one-bit-adc-dac: Add initial version of one bit
> > ADC-DAC
> > 
> > [External]
> > 
> > On Tue, 11 Jan 2022 13:59:19 +0200
> > Cristian Pop <cristian.pop@analog.com> wrote:
> >   
> > > This allows remote reading and writing of the GPIOs. This is useful in
> > > application that run on another PC, at system level, where multiple
> > > iio devices and GPIO devices are integrated together.  
> > Hi Cristian,
> > 
> > So I fully understand why this can be useful, but it is a software only
> > construction so I'm not convinced that it makes sense to necessarily
> > configure it via device tree.   A better fit may well be configfs.
> > (note I always meant to add configfs controls for iio_hwmon but haven't
> > found the time to do it yet...)
> > 
> > As it currently stands, doing only polled / sysfs reads this driver doesn't do
> > enough to justify its existence. You could just do all this in userspace using
> > the existing gpio interfaces.  So to be useful I'd expect it to at least do
> > triggered buffer capture.  
> 
> What do you mean by triggered buffer capture? Should I save previous GPIO
> states into a buffer? What should I do with them?

As in using IIO triggers in conjunction with a Kfifo. So that you can do
in kernel capture of a series of samples triggered at the same time as
captures from ADCs.  Then it's easy to line the data up afterwards in
userspace.

Same in the output direction.  This is only really interesting if it is being
done in sync with something else - otherwise you could just use the usespace
gpio interfaces directly.

> 
> A useful use case that I see:
> - Is to register a function callback (in kernel space, maybe user space that
>   should be used by a different driver), and called in the interrupt handler
>   when the state of an input GPIO changes.

Ok. But if you can just do that directly using the gpio interaces as it will
require a bit of custom driver code anyway.

> - Output to a GPIO from a buffer using a clock, obtaining a PWM like signal.
Again, why is the IIO bit useful?  It's only useful if you are doing it
in sync with other IIO stuff and I don't just mean using the IIO in kernel
interfaces for accessing a channel in a polled mode (effectively call read_raw
/write_raw because whereever you can do that you can just the gpio directly.

Jonathan
> 
> > 
> > When we have talked about handling digital signals int he past we discussed
> > whether the IIO channel description would also benefit from tighter packing
> > than the current minimum of a byte per channel.  Perhaps we don't
> > technically 'need' it here but if we do add it in future it will be hard to retrofit
> > into this driver without big userspace ABI changes (i.e. breaking all existing
> > users).
> > 
> > I've not repeated stuff Andy got it in his review (assuming I remembered it
> > was something Andy raised).
> > 
> > Conclusion:
> > 1) Creation interface needs a rethink or strong argument why it belongs in
> > DT.
> > 2) Driver needs to do more to justify it's existence.
> > 
> > Jonathan
> >   
> > >
> > > Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> > > ---
> > >  drivers/iio/addac/Kconfig           |   8 +
> > >  drivers/iio/addac/Makefile          |   1 +
> > >  drivers/iio/addac/one-bit-adc-dac.c | 229
> > > ++++++++++++++++++++++++++++
> > >  3 files changed, 238 insertions(+)
> > >  create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
> > >
> > > diff --git a/drivers/iio/addac/Kconfig b/drivers/iio/addac/Kconfig
> > > index 138492362f20..5f311f4a747e 100644
> > > --- a/drivers/iio/addac/Kconfig
> > > +++ b/drivers/iio/addac/Kconfig
> > > @@ -17,4 +17,12 @@ config AD74413R
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called ad74413r.
> > >
> > > +config ONE_BIT_ADC_DAC
> > > +	tristate "ONE_BIT_ADC_DAC driver"
> > > +	help
> > > +	  Say yes here to build support for ONE_BIT_ADC_DAC driver.  
> > 
> > Needs a lot more detail here.  Though naming it to be explicitly about GPIO to
> > IIO binding would help.
> >   
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called one-bit-adc-dac.
> > > +
> > >  endmenu
> > > diff --git a/drivers/iio/addac/Makefile b/drivers/iio/addac/Makefile
> > > index cfd4bbe64ad3..0a33f0706b55 100644
> > > --- a/drivers/iio/addac/Makefile
> > > +++ b/drivers/iio/addac/Makefile
> > > @@ -5,3 +5,4 @@
> > >
> > >  # When adding new entries keep the list in alphabetical order
> > >  obj-$(CONFIG_AD74413R) += ad74413r.o
> > > +obj-$(CONFIG_ONE_BIT_ADC_DAC) += one-bit-adc-dac.o
> > > diff --git a/drivers/iio/addac/one-bit-adc-dac.c
> > > b/drivers/iio/addac/one-bit-adc-dac.c
> > > new file mode 100644
> > > index 000000000000..5680de594429
> > > --- /dev/null
> > > +++ b/drivers/iio/addac/one-bit-adc-dac.c
> > > @@ -0,0 +1,229 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > > +/*
> > > + * one-bit-adc-dac  
> > 
> > Improve description here as well.  It's really just a wrapper for a gpio in an IIO
> > channel, so just say that.  Fine to say the representation is 1 bit ADC or DAC
> > channels as well, but I think the GPIO part will be more obvious to the casual
> > reader.
> >   
> > > + *
> > > + * Copyright 2022 Analog Devices Inc.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/gpio/consumer.h>
> > > +
> > > +enum ch_direction {
> > > +	CH_IN,
> > > +	CH_OUT,
> > > +};
> > > +
> > > +struct one_bit_adc_dac_state {
> > > +	int			in_num_ch;
> > > +	int			out_num_ch;
> > > +	struct platform_device  *pdev;  
> > 
> > Not used after probe so don't keep it around.
> >   
> > > +	struct gpio_descs       *in_gpio_descs;
> > > +	struct gpio_descs       *out_gpio_descs;
> > > +	const char		**labels;
> > > +};
> > > +
> > > +static int one_bit_adc_dac_read_raw(struct iio_dev *indio_dev,
> > > +	const struct iio_chan_spec *chan, int *val, int *val2, long info) {
> > > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > > +	struct gpio_descs *descs;
> > > +
> > > +	switch (info) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		if (chan->output)
> > > +			descs = st->out_gpio_descs;
> > > +		else
> > > +			descs = st->in_gpio_descs;
> > > +		*val = gpiod_get_value_cansleep(descs->desc[chan-
> > >channel]);
> > > +
> > > +		return IIO_VAL_INT;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> > > +			    struct iio_chan_spec const *chan,
> > > +			    int val,
> > > +			    int val2,
> > > +			    long info)
> > > +{
> > > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > > +	int channel = chan->channel;
> > > +
> > > +	if (!chan->output)
> > > +		return 0;  
> > 
> > -EINVAL;  It's an error that should be reported to userspace..
> >   
> > > +
> > > +	switch (info) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		gpiod_set_value_cansleep(st->out_gpio_descs-
> > >desc[channel], val);
> > > +
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int one_bit_adc_dac_read_label(struct iio_dev *indio_dev,
> > > +	const struct iio_chan_spec *chan, char *label) {
> > > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > > +	int ch;
> > > +
> > > +	if (chan->output)
> > > +		ch = chan->channel + st->in_num_ch;
> > > +	else
> > > +		ch = chan->channel;
> > > +
> > > +	return sprintf(label, "%s\n", st->labels[ch]); }
> > > +
> > > +static const struct iio_info one_bit_adc_dac_info = {
> > > +	.read_raw = &one_bit_adc_dac_read_raw,
> > > +	.write_raw = &one_bit_adc_dac_write_raw,
> > > +	.read_label = &one_bit_adc_dac_read_label, };
> > > +
> > > +static int one_bit_adc_dac_set_ch(struct iio_chan_spec *channels,
> > > +					int num_ch,
> > > +					enum ch_direction direction)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < num_ch; i++) {
> > > +		channels[i].type = IIO_VOLTAGE;
> > > +		channels[i].indexed = 1;
> > > +		channels[i].channel = i;
> > > +		channels[i].info_mask_separate =  
> > BIT(IIO_CHAN_INFO_RAW);  
> > > +		channels[i].output = direction;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int one_bit_adc_dac_set_channel_label(struct iio_dev *indio_dev,
> > > +						struct iio_chan_spec  
> > *channels,  
> > > +						int num_channels)
> > > +{
> > > +	struct device *device = indio_dev->dev.parent;
> > > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > > +	struct fwnode_handle *fwnode;
> > > +	struct fwnode_handle *child;
> > > +	struct iio_chan_spec *chan;
> > > +	const char *label;
> > > +	int crt_ch = 0, child_num, i = 0;
> > > +
> > > +	fwnode = dev_fwnode(device);
> > > +	child_num = device_get_child_node_count(device);
> > > +
> > > +	st->labels = devm_kzalloc(device, sizeof(*st->labels) * child_num,  
> > GFP_KERNEL);  
> > > +	if (!st->labels)
> > > +		return -ENOMEM;
> > > +
> > > +	i = child_num;
> > > +	fwnode_for_each_child_node(fwnode, child) {
> > > +		if (fwnode_property_read_u32(child, "reg", &crt_ch))
> > > +			continue;
> > > +
> > > +		if (crt_ch >= num_channels)
> > > +			continue;
> > > +
> > > +		if (fwnode_property_read_string(child, "label", &label))
> > > +			continue;
> > > +
> > > +		chan = &channels[crt_ch];  
> > ? Not used.
> >   
> > > +		st->labels[--i] = label;  
> > 
> > I've no idea how this works...  Should be looking for the chan->channel value
> > as that's what your read uses to index.
> >   
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int one_bit_adc_dac_parse_dt(struct iio_dev *indio_dev) {
> > > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > > +	struct iio_chan_spec *channels;
> > > +	int ret, in_num_ch = 0, out_num_ch = 0;
> > > +
> > > +	st->in_gpio_descs = devm_gpiod_get_array_optional(&st->pdev-
> > >dev, "in", GPIOD_IN);
> > > +	if (IS_ERR(st->in_gpio_descs))
> > > +		return PTR_ERR(st->in_gpio_descs);
> > > +
> > > +	if (st->in_gpio_descs)
> > > +		in_num_ch = st->in_gpio_descs->ndescs;
> > > +
> > > +	st->out_gpio_descs = devm_gpiod_get_array_optional(&st->pdev-
> > >dev, "out", GPIOD_OUT_LOW);
> > > +	if (IS_ERR(st->out_gpio_descs))
> > > +		return PTR_ERR(st->out_gpio_descs);
> > > +
> > > +	if (st->out_gpio_descs)
> > > +		out_num_ch = st->out_gpio_descs->ndescs;
> > > +	st->in_num_ch = in_num_ch;
> > > +	st->out_num_ch = out_num_ch;
> > > +
> > > +	channels = devm_kcalloc(indio_dev->dev.parent, in_num_ch +  
> > out_num_ch,  
> > > +				sizeof(*channels), GFP_KERNEL);
> > > +	if (!channels)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = one_bit_adc_dac_set_ch(&channels[0], in_num_ch, CH_IN);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = one_bit_adc_dac_set_ch(&channels[in_num_ch],  
> > out_num_ch, CH_OUT);  
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = one_bit_adc_dac_set_channel_label(indio_dev, channels,  
> > in_num_ch + out_num_ch);  
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	indio_dev->channels = channels;
> > > +	indio_dev->num_channels = in_num_ch + out_num_ch;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int one_bit_adc_dac_probe(struct platform_device *pdev) {
> > > +	struct iio_dev *indio_dev;
> > > +	struct one_bit_adc_dac_state *st;
> > > +	int ret;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	st = iio_priv(indio_dev);
> > > +	st->pdev = pdev;
> > > +	indio_dev->name = "one-bit-adc-dac";
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +	indio_dev->info = &one_bit_adc_dac_info;
> > > +
> > > +	ret = one_bit_adc_dac_parse_dt(indio_dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return devm_iio_device_register(indio_dev->dev.parent,  
> > indio_dev); }  
> > > +
> > > +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> > > +	{ .compatible = "one-bit-adc-dac" },
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);
> > > +
> > > +static struct platform_driver one_bit_adc_dac_driver = {
> > > +	.driver = {
> > > +		.name = "one-bit-adc-dac",
> > > +		.of_match_table = one_bit_adc_dac_dt_match,
> > > +	},
> > > +	.probe = one_bit_adc_dac_probe,
> > > +};
> > > +module_platform_driver(one_bit_adc_dac_driver);
> > > +
> > > +MODULE_AUTHOR("Cristian Pop <cristian.pop@analog.com>");
> > > +MODULE_DESCRIPTION("One bit ADC DAC converter");  
> > MODULE_LICENSE("Dual  
> > > +BSD/GPL");
> > > \ No newline at end of file  
> > 
> > Make sure to eyeball your patches before sending as this sort of thing should
> > be caught at that stage...
> >   
> 

Re: [PATCH v2 1/2] dt-bindings: iio: addac: one-bit-adc-dac yaml documentation

[Rob Herring]

On Tue, 11 Jan 2022 13:59:18 +0200, Cristian Pop wrote:
> This adds device tree bindings for the one-bit-adc-dac.
> 
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> V1->V2
>  - I am aware of the recommendation of rename/move this driver. Should we
>    consider "drivers/io/gpio.c"?
>  - Add .yaml file
>  - Remove blank lines, remove unnecessary coma
>  - Remove macros for channels
>  - Check if channel is input for write_raw
>  - Use labels instead of extend_name
>  - Fix channel indexing
>  - Use "sizeof(*channels)" in devm_kcalloc()
>  - Remove assignment: " indio_dev->dev.parent = &pdev->dev;"
>  - Remove "platform_set_drvdata"
>  - Remove "adi" from compatible string since is not ADI specific driver.
> ---
>  .../bindings/iio/addac/one-bit-adc-dac.yaml   | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.example.dts:19.27-47.11: Warning (unit_address_vs_reg): /example-0/one-bit-adc-dac@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.example.dt.yaml:0:0: /example-0/one-bit-adc-dac@0: failed to match any schema with compatible: ['one-bit-adc-dac']

doc reference errors (make refcheckdocs):

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

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 1/2] dt-bindings: iio: addac: one-bit-adc-dac yaml documentation

[Rob Herring]

On Tue, Jan 11, 2022 at 01:59:18PM +0200, Cristian Pop wrote:
> This adds device tree bindings for the one-bit-adc-dac.

I have no idea what a one-bit-adc-dac is. Please describe or provide a 
reference to what this h/w looks like.

> 
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> V1->V2                                                                     

This belongs below the '---'

>  - I am aware of the recommendation of rename/move this driver. Should we  
>    consider "drivers/io/gpio.c"?                                           
>  - Add .yaml file                                                          
>  - Remove blank lines, remove unnecessary coma                             
>  - Remove macros for channels                                              
>  - Check if channel is input for write_raw                                 
>  - Use labels instead of extend_name                                       
>  - Fix channel indexing                                                    
>  - Use "sizeof(*channels)" in devm_kcalloc()                               
>  - Remove assignment: " indio_dev->dev.parent = &pdev->dev;"               
>  - Remove "platform_set_drvdata"                                           
>  - Remove "adi" from compatible string since is not ADI specific driver.
> ---
>  .../bindings/iio/addac/one-bit-adc-dac.yaml   | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml b/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> new file mode 100644
> index 000000000000..dbed0f3b1ca4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2020 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/addac/one-bit-adc-dac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices one bit ADC DAC driver
> +
> +maintainers:
> +  - Cristian Pop <cristian.pop@analog.com>
> +
> +description: |
> +  One bit ADC DAC driver
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,one-bit-adc-dac
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  in-gpios:
> +    description: Input GPIOs
> +
> +  out-gpios:
> +    description: Output GPIOs

No constraints on how many GPIOs?

> +
> +required:
> +  - compatible
> +  - in-gpios
> +  - out-gpios
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +    description: |
> +      Represents the external channels which are connected to the ADDAC.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +        description: |
> +          The channel number.
> +
> +      label:
> +        description: |
> +          Unique name to identify which channel this is.
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    one-bit-adc-dac@0 {
> +        compatible = "one-bit-adc-dac";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        in-gpios = <&gpio 17 0>, <&gpio 27 0>;
> +        out-gpios = <&gpio 23 0>, <&gpio 24 0>;
> +
> +        channel@0 {
> +          reg = <0>;

What does '0' correspond to?

> +          label = "i_17";

Why is this needed? 'label' is supposed to correspond to physical 
labelling of ports. IOW, for identification by humans looking at the 
device.

This all looks duplicated from information in in-gpios and out-gpios.

> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          label = "i_27";
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          label = "o_23";
> +        };
> +
> +        channel@3 {
> +          reg = <3>;
> +          label = "o_24";
> +        };
> +    };
> -- 
> 2.17.1
> 
> 

Re: [PATCH v2 1/2] dt-bindings: iio: addac: one-bit-adc-dac yaml documentation

[Jonathan Cameron]

On Tue, 11 Jan 2022 13:59:18 +0200
Cristian Pop <cristian.pop@analog.com> wrote:

> This adds device tree bindings for the one-bit-adc-dac.

This series really needs a cover letter where you describe in
general terms what the aim is etc.

> 
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> V1->V2                                                                     
>  - I am aware of the recommendation of rename/move this driver. Should we  
>    consider "drivers/io/gpio.c"? 

Probably keep with the naming of the hwmon iio bridge and go with
gpio_iio.c to indicate bridge from gpio to iio.

I'll put more general comments in patch 2 review, but I'm very doubtful
that setting this up via dt is giong to be the way forward.
Shall we say, the iio_hwmon bindings have always been controversial and
we'd probably not get away with them today...

Reason being it's policy not wiring and reflects internal Linux subsystem
constructs, not generic things applicable to all operating systems.

                                          
>  - Add .yaml file                                                          
>  - Remove blank lines, remove unnecessary coma                             
>  - Remove macros for channels                                              
>  - Check if channel is input for write_raw                                 
>  - Use labels instead of extend_name                                       
>  - Fix channel indexing                                                    
>  - Use "sizeof(*channels)" in devm_kcalloc()                               
>  - Remove assignment: " indio_dev->dev.parent = &pdev->dev;"               
>  - Remove "platform_set_drvdata"                               
Not in this patch so shouldn't be in this description.            
>  - Remove "adi" from compatible string since is not ADI specific driver.
> ---

Version log here for stuff in this patch.  Fine to have the log in the
cover letter if the changes tend to go across multiple patches (renames etc).

>  .../bindings/iio/addac/one-bit-adc-dac.yaml   | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml b/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> new file mode 100644
> index 000000000000..dbed0f3b1ca4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/addac/one-bit-adc-dac.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2020 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/addac/one-bit-adc-dac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices one bit ADC DAC driver
> +
> +maintainers:
> +  - Cristian Pop <cristian.pop@analog.com>
> +
> +description: |
> +  One bit ADC DAC driver
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,one-bit-adc-dac
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  in-gpios:
> +    description: Input GPIOs
> +
> +  out-gpios:
> +    description: Output GPIOs
> +
> +required:
> +  - compatible
> +  - in-gpios
> +  - out-gpios
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-5])$":
> +    type: object
> +    description: |
> +      Represents the external channels which are connected to the ADDAC.
> +
> +    properties:
> +      reg:
> +        maxItems: 1
> +        description: |
> +          The channel number.
> +
> +      label:
> +        description: |
> +          Unique name to identify which channel this is.
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    one-bit-adc-dac@0 {
> +        compatible = "one-bit-adc-dac";
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        in-gpios = <&gpio 17 0>, <&gpio 27 0>;
> +        out-gpios = <&gpio 23 0>, <&gpio 24 0>;
> +
> +        channel@0 {
> +          reg = <0>;
> +          label = "i_17";
> +        };
> +
> +        channel@1 {
> +          reg = <1>;
> +          label = "i_27";
> +        };
> +
> +        channel@2 {
> +          reg = <2>;
> +          label = "o_23";
> +        };
> +
> +        channel@3 {
> +          reg = <3>;
> +          label = "o_24";
> +        };
> +    };