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@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...
>