Re: [PATCH 2/2] iio: spi-dac: Add driver for SPI shift register DACs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2023-12-13 at 14:24 +0100, Mike Looijmans wrote:
> Hi Nuno,
> 
> Thanks for reviewing,
> 
> See below...
> 
> On 13-12-2023 11:37, Nuno Sá wrote:
> > Hi Mike,
> > 
> > Some comments from me...
> > 
> > On Wed, 2023-12-13 at 10:09 +0100, Mike Looijmans wrote:
> > > Add a driver for generic serial shift register DACs like TI DAC714.
> > > 
> > > Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
> > > 
> > > ---
> > > 
> > >   drivers/iio/dac/Kconfig   |  11 ++
> > >   drivers/iio/dac/Makefile  |   1 +
> > >   drivers/iio/dac/spi-dac.c | 212 ++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 224 insertions(+)
> > >   create mode 100644 drivers/iio/dac/spi-dac.c
> > > 
> > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > > index 93b8be183de6..bb35d901ee57 100644
> > > --- a/drivers/iio/dac/Kconfig
> > > +++ b/drivers/iio/dac/Kconfig
> > > @@ -410,6 +410,17 @@ config MCP4922
> > >            To compile this driver as a module, choose M here: the module
> > >            will be called mcp4922.
> > >   
> > > +config SPI_DAC
> > > +       tristate "SPI shift register DAC driver"
> > > +       depends on SPI
> > > +       help
> > > +         Driver for an array of shift-register DACs, like the TI DAC714.
> > > +         The driver shifts the DAC values into the registers in a SPI
> > > +         transfer, then optionally toggles a GPIO to latch the values.
> > > +
> > > +         To compile this driver as a module, choose M here: the module
> > > +         will be called spi-dac.
> > > +
> > >   config STM32_DAC
> > >          tristate "STMicroelectronics STM32 DAC"
> > >          depends on (ARCH_STM32 && OF) || COMPILE_TEST
> > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > > index 5b2bac900d5a..33748799b0f0 100644
> > > --- a/drivers/iio/dac/Makefile
> > > +++ b/drivers/iio/dac/Makefile
> > > @@ -45,6 +45,7 @@ obj-$(CONFIG_MCP4728) += mcp4728.o
> > >   obj-$(CONFIG_MCP4922) += mcp4922.o
> > >   obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
> > >   obj-$(CONFIG_STM32_DAC) += stm32-dac.o
> > > +obj-$(CONFIG_SPI_DAC) += spi-dac.o
> > >   obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
> > >   obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o
> > >   obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o
> > > diff --git a/drivers/iio/dac/spi-dac.c b/drivers/iio/dac/spi-dac.c
> > > new file mode 100644
> > > index 000000000000..0c0113d51604
> > > --- /dev/null
> > > +++ b/drivers/iio/dac/spi-dac.c
> > > @@ -0,0 +1,212 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * SPI generic shift register Serial input Digital-to-Analog Converter
> > > + * For example, TI DAC714
> > > + *
> > > + * Copyright 2023 Topic Embedded Systems
> > > + */
> > > +#include <linux/delay.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/iio/iio.h>
> > > +
> > > +struct spidac {
> > > +       struct spi_device *spi;
> > > +       struct gpio_desc *loaddacs;
> > > +       u8 *data; /* SPI buffer */
> > > +       u32 data_size;
> > > +       /* Protect the data buffer and update sequence */
> > > +       struct mutex lock;
> > > +};
> > > +
> > > +static int spidac_cmd_single(struct spidac *priv,
> > > +                            const struct iio_chan_spec *chan, int val)
> > > +{
> > > +       u8 *data = priv->data + chan->address;
> > > +       unsigned int bytes = chan->scan_type.storagebits >> 3;
> > the '3' seems a bit "magical". Is the intent diving by 8? I would say so and if
> > it
> > is, doing the explicit division would be more readable IMO.
> 
> Divide by 8 is indeed the intention (bits to bytes). But... if 
> storagebits cannot be 24 for example, it'll have to be stored elsewhere 
> anyway.
> 
> 
> > 
> > > +       int ret;
> > > +       unsigned int i;
> > > +
> > > +       /* Write big-endian value into data */
> > > +       data += bytes - 1;
> > > +       for (i = 0; i < bytes; i++, val >>= 8, data--)
> > > +               *data = val & 0xff;
> > This not optimal... You allow someone to put in any 'bits_per_channel' from FW.
> > In
> > theory, one could set, let's say 64bits but then you only allow an integer value.
> > So,
> > we need to make things more sane.
> 
> I think limiting to 32 bit is sensible enough (a 64-bit DAC would be 
> moving individual electrons around or so), but that should indeed be 
> made explicit.
> 
> 
> > With some rework, I think we can also make use of put_unalignedX()...
> 
> Agree. Might want to make endianness a configuration option as well 
> (haven't seen a little-endian device though).

Then I would keep it simple for now and worry about little-endian if an actual use
case pops up.

> 
> 
> > > +
> > > +       ret = spi_write(priv->spi, priv->data, priv->data_size);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       gpiod_set_value(priv->loaddacs, 1);
> > > +       udelay(1);
> > > +       gpiod_set_value(priv->loaddacs, 0);
> > > +
> > Can't we sleep in here?
> 
> indeed, should have used _can_sleep variants (spi_write needs to sleep 
> anyway).
> 
> Probably left over from my thought of doing it in the SPI completion 
> callback.
> 
> 
> > > +       return 0;
> > > +}
> > > +
> > > +static int spidac_decode(struct spidac *priv, const struct iio_chan_spec
> > > *chan)
> > > +{
> > > +       u8 *data = priv->data + chan->address;
> > > +       unsigned int bytes = chan->scan_type.storagebits >> 3;
> > > +       unsigned int i;
> > > +       int val = 0;
> > > +
> > > +       /* Read big-endian value from data */
> > > +       for (i = 0; i < bytes; i++, data++)
> > > +               val = (val << 8) | *data;
> > > +
> > Again, with some refactor I think we can make use of get_unalignedX()...
> > 
> > > +       return val;
> > > +}
> > > +
> > > +static int spidac_read_raw(struct iio_dev *iio_dev,
> > > +                           const struct iio_chan_spec *chan,
> > > +                           int *val, int *val2, long mask)
> > > +{
> > > +       struct spidac *priv;
> > > +
> > > +       switch (mask) {
> > > +       case IIO_CHAN_INFO_RAW:
> > > +               priv = iio_priv(iio_dev);
> > > +
> > > +               mutex_lock(&priv->lock);
> > > +               *val = spidac_decode(priv, chan);
> > > +               mutex_unlock(&priv->lock);
> > > +
> > > +               return IIO_VAL_INT;
> > > +
> > > +       case IIO_CHAN_INFO_SCALE:
> > > +               *val = 1;
> > > +               return IIO_VAL_INT;
> > > +
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > > +}
> > > +
> > > +static int spidac_write_raw(struct iio_dev *iio_dev,
> > > +                            const struct iio_chan_spec *chan,
> > > +                            int val, int val2, long mask)
> > > +{
> > > +       struct spidac *priv = iio_priv(iio_dev);
> > > +       int ret;
> > > +
> > > +       if (mask != IIO_CHAN_INFO_RAW)
> > > +               return -EINVAL;
> > > +
> > nit: I would still keep the switch(). Consistency with read_raw().
> Agree
> > 
> > > +       mutex_lock(&priv->lock);
> > > +       ret = spidac_cmd_single(priv, chan, val);
> > > +       mutex_unlock(&priv->lock);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static const struct iio_info spidac_info = {
> > > +       .read_raw = spidac_read_raw,
> > > +       .write_raw = spidac_write_raw,
> > > +};
> > > +
> > > +static int spidac_probe(struct spi_device *spi)
> > > +{
> > > +       struct iio_dev *iio_dev;
> > > +       struct spidac *priv;
> > > +       struct iio_chan_spec *channels;
> > > +       struct gpio_desc *reset_gpio;
> > > +       u32 num_channels;
> > > +       u32 bits_per_channel;
> > > +       u32 bytes_per_channel;
> > > +       u32 i;
> > > +
> > > +       iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv));
> > > +       if (!iio_dev)
> > > +               return -ENOMEM;
> > > +
> > > +       priv = iio_priv(iio_dev);
> > > +       priv->loaddacs = devm_gpiod_get_optional(&spi->dev, "ldac",
> > > +                                                GPIOD_OUT_LOW);
> > > +       if (IS_ERR(priv->loaddacs))
> > > +               return PTR_ERR(priv->loaddacs);
> > > +
> > > +       reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> > > +                                            GPIOD_OUT_HIGH);
> > > +       if (IS_ERR(reset_gpio))
> > > +               return PTR_ERR(reset_gpio);
> > > +
> > > +       priv->spi = spi;
> > > +       spi_set_drvdata(spi, iio_dev);
> > > +       num_channels = 1;
> > > +       bits_per_channel = 16;
> > > +
> > > +       device_property_read_u32(&spi->dev, "num-channels", &num_channels);
> > > +       device_property_read_u32(&spi->dev, "bits-per-channel",
> > > +                                &bits_per_channel);
> > > +       bytes_per_channel = DIV_ROUND_UP(bits_per_channel, 8);
> > > +
> > > +       channels = devm_kcalloc(&spi->dev, num_channels, sizeof(*channels),
> > > +                               GFP_KERNEL);
> > > +       if (!channels)
> > > +               return -ENOMEM;
> > > +
> > > +       priv->data_size = num_channels * bytes_per_channel;
> > > +       priv->data = devm_kzalloc(&spi->dev, priv->data_size,
> > > +                                 GFP_KERNEL | GFP_DMA);
> > GFP_DMA? This is rather unusual... And if you look at the description of it, does
> > not
> > look like a good idea to use it. Also, consider devm_kcalloc()
> 
> The "data" buffer is to be passed to SPI controller and must be DMA 
> capable. Hence the GFP_DMA.
> 
> Feels oldskool to me as well, but could not come up with a better 
> solution...
> 

Hmm, that has nothing to do with GFP_DMA AFAIK... Note that you're devm_kzalloc()
your buffer so your data is already DMA safe (meaning cacheline aligned - and not
just L1) . Now if DMA is used or not is entirely up to the spi controller (normally
it depends on the amount of data you want to send).

> > > +       if (!priv->data)
> > > +               return -ENOMEM;
> > > +
> > > +       for (i = 0; i < num_channels; i++) {
> > > +               struct iio_chan_spec *chan = &channels[i];
> > > +
> > > +               chan->type = IIO_VOLTAGE;
> > > +               chan->indexed = 1;
> > > +               chan->output = 1;
> > > +               chan->channel = i;
> > > +               chan->address = i * bytes_per_channel;
> > > +               chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > > +               chan->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE);
> > > +               chan->scan_type.sign = 's';
> > > +               chan->scan_type.realbits = bits_per_channel;
> > > +               chan->scan_type.storagebits = bits_per_channel;
> > Hmm does no look right. You pretty much allow any value from FW and I'm fairly
> > sure
> > that 'storagebits' have to be the size of a C data type as we want elements to be
> > naturally aligned when buffering for example. I'm seeing you're not using
> > buffering
> > but still... Is really any arbitrary value what we want here?
> 
> Found out the hard way (while writing ads1298 driver) that this is 
> indeed the case, IIO cannot handle 24-bit buffers for example. This 
> driver doesn't support any buffering though.
> 
> So indeed, I'll have to separate things. This will also affect DT bindings.
> 
> How many bytes per sample to be sent on the SPI bus, and how many bits 
> actually mean anything. For example, 2 bytes per sample and 14-bit 
> resolution.
> 
> > > +       }
> > > +
> > > +       iio_dev->info = &spidac_info;
> > > +       iio_dev->modes = INDIO_DIRECT_MODE;
> > > +       iio_dev->channels = channels;
> > > +       iio_dev->num_channels = num_channels;
> > > +       iio_dev->name = spi_get_device_id(spi)->name;
> > > +
> > > +       mutex_init(&priv->lock);
> > > +
> > > +       if (reset_gpio) {
> > > +               udelay(1);
> > > +               gpiod_set_value(reset_gpio, 0);
> > > +       }
> > > +
> > I would place devm_gpiod_get_optional() close to the place of the reset... Also,
> > any
> > strong reason for udelay()? Consider fsleep() instead.
> 
> That, and can_sleep.
> 
> 
> > 
> > > +       return devm_iio_device_register(&spi->dev, iio_dev);
> > > +}
> > > +
> > > +static const struct spi_device_id spidac_id[] = {
> > > +       {"spi-dac"},
> > no ti,dac714?
> 
> That, and I've been wondering if this table is needed at all?
> 

Yes, otherwise module auto loading won't work. You can remove it and see what happens
:).

- Nuno Sá





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux