Messages in this thread | | | Subject | Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC | From | Lars-Peter Clausen <> | Date | Thu, 16 Jul 2020 11:25:36 +0200 |
| |
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");
| |