Hi Jonathan,
The additional functionality it is a little bit unclear to me.
> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, January 16, 2022 1:52 PM
> To: Pop, Cristian <Cristian.Pop@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>");
> > +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...
>
