> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Monday, July 20, 2020 4:52 PM
> To: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Cc: Pop, Cristian <Cristian.Pop@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>");
> > > +MODULE_DESCRIPTION("Analog Devices ONE_BIT_ADC_DAC");
> > > +MODULE_LICENSE("GPL v2");
> >
> >
