[RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[Cristian Pop]

Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)

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

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..8e2a8a09fedb
--- /dev/null
+++ b/drivers/iio/addac/one-bit-adc-dac.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices ONE_BIT_ADC_DAC
+ * Digital to Analog Converters driver
+ *
+ * Copyright 2019 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 {
+	struct platform_device  *pdev;
+	struct gpio_descs       *in_gpio_descs;
+	struct gpio_descs       *out_gpio_descs;
+};
+
+ #define ONE_BIT_ADC_DAC_CHANNEL(idx, direction)			\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = idx,						\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.output = direction,					\
+	}
+
+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);
+	int in_num_ch = 0, out_num_ch = 0;
+	int channel = chan->channel;
+
+	if (st->in_gpio_descs)
+		in_num_ch = st->in_gpio_descs->ndescs;
+
+	if (st->out_gpio_descs)
+		out_num_ch = st->out_gpio_descs->ndescs;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (channel < in_num_ch) {
+			*val = gpiod_get_value_cansleep(
+				st->in_gpio_descs->desc[channel]);
+		} else {
+			channel -= in_num_ch;
+			*val = gpiod_get_value_cansleep(
+				st->out_gpio_descs->desc[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 in_num_ch = 0, out_num_ch = 0;
+	int channel = chan->channel;
+
+	if (st->in_gpio_descs)
+		in_num_ch = st->in_gpio_descs->ndescs;
+
+	if (st->out_gpio_descs)
+		out_num_ch = st->out_gpio_descs->ndescs;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (channel < in_num_ch) {
+			gpiod_set_value_cansleep(
+				st->in_gpio_descs->desc[channel], val);
+		} else {
+			channel -= in_num_ch;
+			gpiod_set_value_cansleep(
+				st->out_gpio_descs->desc[channel], val);
+		}
+
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+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,
+};
+
+static int one_bit_adc_dac_set_ch(struct iio_dev *indio_dev,
+					struct iio_chan_spec *channels,
+					const char *propname,
+					int num_ch,
+					enum ch_direction direction,
+					int offset)
+{
+	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+	const char **gpio_names;
+	int ret, i;
+
+	if (num_ch <= 0)
+		return 0;
+
+	gpio_names = devm_kcalloc(indio_dev->dev.parent,
+					num_ch,
+					sizeof(char *),
+					GFP_KERNEL);
+	if (!gpio_names)
+		return -ENOMEM;
+
+	ret = device_property_read_string_array(&st->pdev->dev,
+					propname,
+					gpio_names,
+					num_ch);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < num_ch; i++) {
+		channels[i] = (struct iio_chan_spec)ONE_BIT_ADC_DAC_CHANNEL(i +
+							offset,
+							direction);
+		channels[i].extend_name = gpio_names[i];
+	}
+
+	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_HIGH);
+	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;
+
+	channels = devm_kcalloc(indio_dev->dev.parent, (in_num_ch + out_num_ch),
+				sizeof(struct iio_chan_spec), GFP_KERNEL);
+	if (!channels)
+		return -ENOMEM;
+
+	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[0],
+					"in-gpio-names", in_num_ch,
+					CH_IN, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[in_num_ch],
+					"out-gpio-names", out_num_ch,
+					CH_OUT, in_num_ch);
+	if (ret < 0)
+		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->dev.parent = &pdev->dev;
+	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 < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, indio_dev);
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
+}
+
+static const struct of_device_id one_bit_adc_dac_dt_match[] = {
+	{ .compatible = "adi,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("Analog Devices ONE_BIT_ADC_DAC");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[Lars-Peter Clausen]

On 7/16/20 9:27 AM, Cristian Pop wrote:
> Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)

Very sneaky way of introducing a iio-gpio-proxy driver to be able to 
access GPIOs through libiio ;). I'm not really a fan of the whole idea.

But either way I think this needs a better description of what 1-bit 
converters are and how they are used.

>
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> ---
>   drivers/iio/addac/one-bit-adc-dac.c | 229 ++++++++++++++++++++++++++++
>   1 file changed, 229 insertions(+)
>   create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
>
> 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..8e2a8a09fedb
> --- /dev/null
> +++ b/drivers/iio/addac/one-bit-adc-dac.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices ONE_BIT_ADC_DAC
> + * Digital to Analog Converters driver
> + *
> + * Copyright 2019 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 {
> +	struct platform_device  *pdev;
> +	struct gpio_descs       *in_gpio_descs;
> +	struct gpio_descs       *out_gpio_descs;
> +};
> +
> + #define ONE_BIT_ADC_DAC_CHANNEL(idx, direction)			\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = idx,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.output = direction,					\
> +	}
> +
> +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);
> +	int in_num_ch = 0, out_num_ch = 0;
> +	int channel = chan->channel;
> +
> +	if (st->in_gpio_descs)
> +		in_num_ch = st->in_gpio_descs->ndescs;
> +
> +	if (st->out_gpio_descs)
> +		out_num_ch = st->out_gpio_descs->ndescs;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (channel < in_num_ch) {
> +			*val = gpiod_get_value_cansleep(
> +				st->in_gpio_descs->desc[channel]);
> +		} else {
> +			channel -= in_num_ch;
> +			*val = gpiod_get_value_cansleep(
> +				st->out_gpio_descs->desc[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 in_num_ch = 0, out_num_ch = 0;
> +	int channel = chan->channel;
> +
> +	if (st->in_gpio_descs)
> +		in_num_ch = st->in_gpio_descs->ndescs;
> +
> +	if (st->out_gpio_descs)
> +		out_num_ch = st->out_gpio_descs->ndescs;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (channel < in_num_ch) {
> +			gpiod_set_value_cansleep(
> +				st->in_gpio_descs->desc[channel], val);

How can we set a value on an input GPIO?

> +		} else {
> +			channel -= in_num_ch;
> +			gpiod_set_value_cansleep(
> +				st->out_gpio_descs->desc[channel], val);
> +		}
> +
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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,
> +};
> +
> +static int one_bit_adc_dac_set_ch(struct iio_dev *indio_dev,
> +					struct iio_chan_spec *channels,
> +					const char *propname,
> +					int num_ch,
> +					enum ch_direction direction,
> +					int offset)
> +{
> +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +	const char **gpio_names;
> +	int ret, i;
> +
> +	if (num_ch <= 0)
> +		return 0;
> +
> +	gpio_names = devm_kcalloc(indio_dev->dev.parent,
> +					num_ch,
> +					sizeof(char *),
sizeof(*gpio_names). It might be better to use normal kcalloc, kfree 
here since you only use it in this function.
> +					GFP_KERNEL);
> +	if (!gpio_names)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_string_array(&st->pdev->dev,
> +					propname,
> +					gpio_names,
> +					num_ch);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < num_ch; i++) {
> +		channels[i] = (struct iio_chan_spec)ONE_BIT_ADC_DAC_CHANNEL(i +
> +							offset,
> +							direction);
> +		channels[i].extend_name = gpio_names[i];
I think we want to avoid using extend_name in new drivers because it 
makes for a very clumsy ABI. We should add a label property like we have 
for the device for channels to have a symbolic name of the channel.
> +	}
> +
> +	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_HIGH);
> +	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;
> +
> +	channels = devm_kcalloc(indio_dev->dev.parent, (in_num_ch + out_num_ch),
> +				sizeof(struct iio_chan_spec), GFP_KERNEL);

sizeof(*channels) to avoid accidentally using the wrong type.

> +	if (!channels)
> +		return -ENOMEM;
> +
> +	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[0],
> +					"in-gpio-names", in_num_ch,
> +					CH_IN, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[in_num_ch],
> +					"out-gpio-names", out_num_ch,
> +					CH_OUT, in_num_ch);
> +	if (ret < 0)
> +		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->dev.parent = &pdev->dev;
parent assignment should not be needed thanks to Alex's work.
> +	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 < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, indio_dev);

There does not seem to be a matching get_drvdata() anywhere so this is 
not needed.

> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> +}
> +
> +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> +	{ .compatible = "adi,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("Analog Devices ONE_BIT_ADC_DAC");
> +MODULE_LICENSE("GPL v2");

Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[Jonathan Cameron]

On Thu, 16 Jul 2020 11:25:36 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 7/16/20 9:27 AM, Cristian Pop wrote:
> > Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)  
> 
> Very sneaky way of introducing a iio-gpio-proxy driver to be able to 
> access GPIOs through libiio ;). I'm not really a fan of the whole idea.
> 
> But either way I think this needs a better description of what 1-bit 
> converters are and how they are used.
I'll second that.  If we want to do this, I'd much rather seeing as
an explicit gpio to IIO bridge driver. 

If there is a comparator on an ADC pin, then the analog characteristics
of that need describing.  There might be some argument in favour if that
was done and hence we had scale etc provided for the channel.

Given this is really just putting a new interface on gpios please
cc the gpio maintainer / list for future versions.

Thanks,

Jonathan


> 
> >
> > Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> > ---
> >   drivers/iio/addac/one-bit-adc-dac.c | 229 ++++++++++++++++++++++++++++
> >   1 file changed, 229 insertions(+)
> >   create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
> >
> > 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..8e2a8a09fedb
> > --- /dev/null
> > +++ b/drivers/iio/addac/one-bit-adc-dac.c
> > @@ -0,0 +1,229 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices ONE_BIT_ADC_DAC
> > + * Digital to Analog Converters driver
> > + *
> > + * Copyright 2019 Analog Devices Inc.

Probably update to 2020 or 2019-2020

> > + */
> > +
> > +#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 {
> > +	struct platform_device  *pdev;
> > +	struct gpio_descs       *in_gpio_descs;
> > +	struct gpio_descs       *out_gpio_descs;
> > +};
> > +
> > + #define ONE_BIT_ADC_DAC_CHANNEL(idx, direction)			\
> > +	{								\
> > +		.type = IIO_VOLTAGE,					\
> > +		.indexed = 1,						\
> > +		.channel = idx,						\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +		.output = direction,					\
> > +	}

Macro only used in one place, I'd not bother with the macro.

> > +
> > +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);
> > +	int in_num_ch = 0, out_num_ch = 0;
> > +	int channel = chan->channel;
> > +
> > +	if (st->in_gpio_descs)
> > +		in_num_ch = st->in_gpio_descs->ndescs;
> > +
> > +	if (st->out_gpio_descs)
> > +		out_num_ch = st->out_gpio_descs->ndescs;
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (channel < in_num_ch) {
> > +			*val = gpiod_get_value_cansleep(
> > +				st->in_gpio_descs->desc[channel]);
> > +		} else {
> > +			channel -= in_num_ch;
> > +			*val = gpiod_get_value_cansleep(
> > +				st->out_gpio_descs->desc[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 in_num_ch = 0, out_num_ch = 0;
> > +	int channel = chan->channel;
> > +
> > +	if (st->in_gpio_descs)
> > +		in_num_ch = st->in_gpio_descs->ndescs;
> > +
> > +	if (st->out_gpio_descs)
> > +		out_num_ch = st->out_gpio_descs->ndescs;
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (channel < in_num_ch) {
> > +			gpiod_set_value_cansleep(
> > +				st->in_gpio_descs->desc[channel], val);  
> 
> How can we set a value on an input GPIO?
> 
> > +		} else {
> > +			channel -= in_num_ch;
> > +			gpiod_set_value_cansleep(
> > +				st->out_gpio_descs->desc[channel], val);
> > +		}
> > +
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +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,
> > +};
> > +
> > +static int one_bit_adc_dac_set_ch(struct iio_dev *indio_dev,
> > +					struct iio_chan_spec *channels,
> > +					const char *propname,
> > +					int num_ch,
> > +					enum ch_direction direction,
> > +					int offset)
> > +{
> > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > +	const char **gpio_names;
> > +	int ret, i;
> > +
> > +	if (num_ch <= 0)
> > +		return 0;
> > +
> > +	gpio_names = devm_kcalloc(indio_dev->dev.parent,
> > +					num_ch,
> > +					sizeof(char *),  
> sizeof(*gpio_names). It might be better to use normal kcalloc, kfree 
> here since you only use it in this function.

Definitely.

> > +					GFP_KERNEL);
> > +	if (!gpio_names)
> > +		return -ENOMEM;
> > +
> > +	ret = device_property_read_string_array(&st->pdev->dev,
> > +					propname,
> > +					gpio_names,
> > +					num_ch);
Take advantage of the new longer acceptable line length (100 chars)
to make some of these more readable.

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for (i = 0; i < num_ch; i++) {
> > +		channels[i] = (struct iio_chan_spec)ONE_BIT_ADC_DAC_CHANNEL(i +
> > +							offset,
> > +							direction);
> > +		channels[i].extend_name = gpio_names[i];  
> I think we want to avoid using extend_name in new drivers because it 
> makes for a very clumsy ABI. We should add a label property like we have 
> for the device for channels to have a symbolic name of the channel.

Agreed. It keeps getting talked about, but no patches yet IIRC.

I'd expect separate indexing for input and output channels.
The in / out distinguishes them.

in_voltage0_raw
out_voltage0_raw etc


> > +	}
> > +
> > +	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_HIGH);
> > +	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;
> > +
> > +	channels = devm_kcalloc(indio_dev->dev.parent, (in_num_ch + out_num_ch),
> > +				sizeof(struct iio_chan_spec), GFP_KERNEL);  
> 
> sizeof(*channels) to avoid accidentally using the wrong type.
> 
> > +	if (!channels)
> > +		return -ENOMEM;
> > +
> > +	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[0],
> > +					"in-gpio-names", in_num_ch,
> > +					CH_IN, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[in_num_ch],
> > +					"out-gpio-names", out_num_ch,
> > +					CH_OUT, in_num_ch);
> > +	if (ret < 0)
> > +		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->dev.parent = &pdev->dev;  
> parent assignment should not be needed thanks to Alex's work.
> > +	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 < 0)
> > +		return ret;
> > +
> > +	platform_set_drvdata(pdev, indio_dev);  
> 
> There does not seem to be a matching get_drvdata() anywhere so this is 
> not needed.
> 
> > +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > +}
> > +
> > +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> > +	{ .compatible = "adi,one-bit-adc-dac" },

This is definitely not ADI specific.  It also currently looks
like a policy thing rather than truely defined by the wiring.
Hence I'd kind of expect to see it instantiated via configfs
rather than a dt binding.

Note this would definitely need a dt binding doc if done this way.
My gut feeling is that as it stands, it would go nowhere.



> > +	{},
> > +};
> > +
> > +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("Analog Devices ONE_BIT_ADC_DAC");
> > +MODULE_LICENSE("GPL v2");  
> 
> 

Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[Andy Shevchenko]

On Thu, Jul 16, 2020 at 10:27 AM Cristian Pop <cristian.pop@analog.com> wrote:
>
> Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)

...

> +struct one_bit_adc_dac_state {

> +       struct platform_device  *pdev;

Wouldn't 'struct device *dev;' be enough?

> +       struct gpio_descs       *in_gpio_descs;
> +       struct gpio_descs       *out_gpio_descs;
> +};

...

> +{
> +       struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> +       int in_num_ch = 0, out_num_ch = 0;
> +       int channel = chan->channel;

> +       if (st->in_gpio_descs)
> +               in_num_ch = st->in_gpio_descs->ndescs;
> +
> +       if (st->out_gpio_descs)
> +               out_num_ch = st->out_gpio_descs->ndescs;

I'm wondering if you need these conditionals. Is it even possible you
have ndescs > 0 and no descriptors?

> +       switch (info) {
> +       case IIO_CHAN_INFO_RAW:
> +               if (channel < in_num_ch) {

> +                       *val = gpiod_get_value_cansleep(
> +                               st->in_gpio_descs->desc[channel]);

Strange indentation...

> +               } else {
> +                       channel -= in_num_ch;

> +                       *val = gpiod_get_value_cansleep(
> +                               st->out_gpio_descs->desc[channel]);

Ditto.

> +               }
> +               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)
> +{

Same comments as per previous function above.

> +}

...

> +       ret = device_property_read_string_array(&st->pdev->dev,
> +                                       propname,
> +                                       gpio_names,
> +                                       num_ch);

> +       if (ret < 0)

if (ret)

> +               return ret;

...

> +       channels = devm_kcalloc(indio_dev->dev.parent, (in_num_ch + out_num_ch),

Extra parentheses.

> +                               sizeof(struct iio_chan_spec), GFP_KERNEL);

sizeof(*channels) ?

> +       if (!channels)
> +               return -ENOMEM;

...

> +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> +       { .compatible = "adi,one-bit-adc-dac" },

> +       {},

No comma here!

> +};

> +

Unnecessary blank line.

> +MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);

...

> +

Ditto.

> +module_platform_driver(one_bit_adc_dac_driver);

-- 
With Best Regards,
Andy Shevchenko

RE: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[Pop, Cristian]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Monday, July 20, 2020 4:52 PM
> To: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Pop, Cristian <Cristian.Pop@analog.com>; linux-iio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC,
> DAC
> 
> [External]
> 
> On Thu, 16 Jul 2020 11:25:36 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
> > On 7/16/20 9:27 AM, Cristian Pop wrote:
> > > Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)
> >
> > Very sneaky way of introducing a iio-gpio-proxy driver to be able to
> > access GPIOs through libiio ;). I'm not really a fan of the whole idea.
> >
> > But either way I think this needs a better description of what 1-bit
> > converters are and how they are used.
> I'll second that.  If we want to do this, I'd much rather seeing as an explicit gpio
> to IIO bridge driver.
> 
> If there is a comparator on an ADC pin, then the analog characteristics of that
> need describing.  There might be some argument in favour if that was done
> and hence we had scale etc provided for the channel.
> 
> Given this is really just putting a new interface on gpios please cc the gpio
> maintainer / list for future versions.
> 
> Thanks,
> 
> Jonathan
> 
> 
> >
> > >
> > > Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> > > ---
> > >   drivers/iio/addac/one-bit-adc-dac.c | 229
> ++++++++++++++++++++++++++++
> > >   1 file changed, 229 insertions(+)
> > >   create mode 100644 drivers/iio/addac/one-bit-adc-dac.c
> > >
> > > 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..8e2a8a09fedb
> > > --- /dev/null
> > > +++ b/drivers/iio/addac/one-bit-adc-dac.c
> > > @@ -0,0 +1,229 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices ONE_BIT_ADC_DAC
> > > + * Digital to Analog Converters driver
> > > + *
> > > + * Copyright 2019 Analog Devices Inc.
> 
> Probably update to 2020 or 2019-2020
> 
> > > + */
> > > +
> > > +#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 {
> > > +	struct platform_device  *pdev;
> > > +	struct gpio_descs       *in_gpio_descs;
> > > +	struct gpio_descs       *out_gpio_descs;
> > > +};
> > > +
> > > + #define ONE_BIT_ADC_DAC_CHANNEL(idx, direction)
> 	\
> > > +	{								\
> > > +		.type = IIO_VOLTAGE,					\
> > > +		.indexed = 1,						\
> > > +		.channel = idx,						\
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> 	\
> > > +		.output = direction,					\
> > > +	}
> 
> Macro only used in one place, I'd not bother with the macro.
> 
> > > +
> > > +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);
> > > +	int in_num_ch = 0, out_num_ch = 0;
> > > +	int channel = chan->channel;
> > > +
> > > +	if (st->in_gpio_descs)
> > > +		in_num_ch = st->in_gpio_descs->ndescs;
> > > +
> > > +	if (st->out_gpio_descs)
> > > +		out_num_ch = st->out_gpio_descs->ndescs;
> > > +
> > > +	switch (info) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		if (channel < in_num_ch) {
> > > +			*val = gpiod_get_value_cansleep(
> > > +				st->in_gpio_descs->desc[channel]);
> > > +		} else {
> > > +			channel -= in_num_ch;
> > > +			*val = gpiod_get_value_cansleep(
> > > +				st->out_gpio_descs->desc[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 in_num_ch = 0, out_num_ch = 0;
> > > +	int channel = chan->channel;
> > > +
> > > +	if (st->in_gpio_descs)
> > > +		in_num_ch = st->in_gpio_descs->ndescs;
> > > +
> > > +	if (st->out_gpio_descs)
> > > +		out_num_ch = st->out_gpio_descs->ndescs;
> > > +
> > > +	switch (info) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		if (channel < in_num_ch) {
> > > +			gpiod_set_value_cansleep(
> > > +				st->in_gpio_descs->desc[channel], val);
> >
> > How can we set a value on an input GPIO?
> >
> > > +		} else {
> > > +			channel -= in_num_ch;
> > > +			gpiod_set_value_cansleep(
> > > +				st->out_gpio_descs->desc[channel], val);
> > > +		}
> > > +
> > > +		return 0;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +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, };
> > > +
> > > +static int one_bit_adc_dac_set_ch(struct iio_dev *indio_dev,
> > > +					struct iio_chan_spec *channels,
> > > +					const char *propname,
> > > +					int num_ch,
> > > +					enum ch_direction direction,
> > > +					int offset)
> > > +{
> > > +	struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> > > +	const char **gpio_names;
> > > +	int ret, i;
> > > +
> > > +	if (num_ch <= 0)
> > > +		return 0;
> > > +
> > > +	gpio_names = devm_kcalloc(indio_dev->dev.parent,
> > > +					num_ch,
> > > +					sizeof(char *),
> > sizeof(*gpio_names). It might be better to use normal kcalloc, kfree
> > here since you only use it in this function.
> 
> Definitely.
> 
> > > +					GFP_KERNEL);
> > > +	if (!gpio_names)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = device_property_read_string_array(&st->pdev->dev,
> > > +					propname,
> > > +					gpio_names,
> > > +					num_ch);
> Take advantage of the new longer acceptable line length (100 chars) to make
> some of these more readable.
> 
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	for (i = 0; i < num_ch; i++) {
> > > +		channels[i] = (struct
> iio_chan_spec)ONE_BIT_ADC_DAC_CHANNEL(i +
> > > +							offset,
> > > +							direction);
> > > +		channels[i].extend_name = gpio_names[i];
> > I think we want to avoid using extend_name in new drivers because it
> > makes for a very clumsy ABI. We should add a label property like we
> > have for the device for channels to have a symbolic name of the channel.
The current dts looks like this:
one-bit-adc-dac@0 {
				in-gpios = <&gpio 17 0>, <&gpio 27 0>;
				in-gpio-names = "i_17", "i_27";
				out-gpios = <&gpio 23 0>, <&gpio 24 0>;
				out-gpio-names = "o_23", "o_24";
			};
Resulting in channels:
in_voltage0_i_17_raw
in_voltage1_i_27_raw
out_voltage2_o_23_raw
out_voltage3_o_24_raw
If we want to lose extend_name, please provide an example for using labels.
How the dts should look like, how do I use it in the driver?
> 
> Agreed. It keeps getting talked about, but no patches yet IIRC.
> 
> I'd expect separate indexing for input and output channels.
> The in / out distinguishes them.
> 
> in_voltage0_raw
> out_voltage0_raw etc
> 
> 
> > > +	}
> > > +
> > > +	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_HIGH);
> > > +	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;
> > > +
> > > +	channels = devm_kcalloc(indio_dev->dev.parent, (in_num_ch +
> out_num_ch),
> > > +				sizeof(struct iio_chan_spec), GFP_KERNEL);
> >
> > sizeof(*channels) to avoid accidentally using the wrong type.
> >
> > > +	if (!channels)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[0],
> > > +					"in-gpio-names", in_num_ch,
> > > +					CH_IN, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = one_bit_adc_dac_set_ch(indio_dev, &channels[in_num_ch],
> > > +					"out-gpio-names", out_num_ch,
> > > +					CH_OUT, in_num_ch);
> > > +	if (ret < 0)
> > > +		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->dev.parent = &pdev->dev;
> > parent assignment should not be needed thanks to Alex's work.
> > > +	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 < 0)
> > > +		return ret;
> > > +
> > > +	platform_set_drvdata(pdev, indio_dev);
> >
> > There does not seem to be a matching get_drvdata() anywhere so this is
> > not needed.
> >
> > > +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > > +}
> > > +
> > > +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> > > +	{ .compatible = "adi,one-bit-adc-dac" },
> 
> This is definitely not ADI specific.  It also currently looks like a policy thing
> rather than truely defined by the wiring.
> Hence I'd kind of expect to see it instantiated via configfs rather than a dt
> binding.
> 
> Note this would definitely need a dt binding doc if done this way.
> My gut feeling is that as it stands, it would go nowhere.
> 
> 
> 
> > > +	{},
> > > +};
> > > +
> > > +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("Analog Devices ONE_BIT_ADC_DAC");
> > > +MODULE_LICENSE("GPL v2");
> >
> >

Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

[Jonathan Cameron]

..
> >   
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	for (i = 0; i < num_ch; i++) {
> > > > +		channels[i] = (struct  
> > iio_chan_spec)ONE_BIT_ADC_DAC_CHANNEL(i +  
> > > > +							offset,
> > > > +							direction);
> > > > +		channels[i].extend_name = gpio_names[i];  
> > > I think we want to avoid using extend_name in new drivers because it
> > > makes for a very clumsy ABI. We should add a label property like we
> > > have for the device for channels to have a symbolic name of the channel.  
> The current dts looks like this:
> one-bit-adc-dac@0 {
> 				in-gpios = <&gpio 17 0>, <&gpio 27 0>;
> 				in-gpio-names = "i_17", "i_27";
> 				out-gpios = <&gpio 23 0>, <&gpio 24 0>;
> 				out-gpio-names = "o_23", "o_24";
> 			};
> Resulting in channels:
> in_voltage0_i_17_raw
> in_voltage1_i_27_raw
> out_voltage2_o_23_raw
> out_voltage3_o_24_raw
> If we want to lose extend_name, please provide an example for using labels.
> How the dts should look like, how do I use it in the driver?

Doesn't exist yet :)  Proposal was to add support for a label attribute
to the iio core.

If we were going to do a generic binding it would have to be part of the
existing channel bindings. e.g.
Documentation/devicetree/bindings/iio/adc/adc.txt

But here it might be fine to use the gpio-names to fill one in.
Alternative would be to use a binding where the gpio was specified
as part of the per channel child nodes rather than as a single entry.