Re: [PATCH 2/2] iio: spi-dac: Add driver for SPI shift register DACs
From: Nuno Sá
Date: Wed Dec 13 2023 - 09:03:54 EST
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á