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

["Nuno Sá" <noname.nuno@gmail.com>]

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

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

With some rework, I think we can also make use of put_unalignedX()...

> +
> +       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? 
 
> +       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().

> +       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()

> +       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? 

> +       }
> +
> +       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.

> +       return devm_iio_device_register(&spi->dev, iio_dev);
> +}
> +
> +static const struct spi_device_id spidac_id[] = {
> +       {"spi-dac"},

no ti,dac714?

> +       {}
> +};
> +MODULE_DEVICE_TABLE(spi, spidac_id);
> +
> +static const struct of_device_id spidac_of_match[] = {
> +       { .compatible = "spi-dac" },
> +       { .compatible = "ti,dac714" },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, spidac_of_match);
> +
> +static struct spi_driver spidac_driver = {
> +       .driver = {
> +                  .name = "spi-dac",
> +                  .of_match_table = spidac_of_match,
> +                  },

indentation...

- Nuno Sá