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

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

[Mike Looijmans]

Add a driver for generic serial shift register DACs like TI DAC714.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

---

 .../devicetree/bindings/iio/dac/spidac.yaml   | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/spidac.yaml

diff --git a/Documentation/devicetree/bindings/iio/dac/spidac.yaml b/Documentation/devicetree/bindings/iio/dac/spidac.yaml
new file mode 100644
index 000000000000..be98da728594
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/spidac.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/spidac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic "shift register" SPI DAC
+
+description:
+  Supports simple SPI "shift register" DACs, like TI's DAC714. These DACs have
+  no control registers or commands, they just use a clock and serial data to
+  shift in a raw DAC value. Multiple DACs can be daisy-chained together.
+
+maintainers:
+  - Mike Looijmans <mike.looijmans@topic.nl>
+
+properties:
+  compatible:
+    enum:
+      - spi-dac
+      - ti,dac714
+
+  reg:
+    maxItems: 1
+
+  ldac-gpios:
+    description:
+      LDAC pin to be used as a hardware trigger to update the DAC outputs. Not
+      needed when the DACs use the chip select to update their output.
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Optional reset pin that resets all DACs.
+    maxItems: 1
+
+  num-channels:
+    description:
+      Number of channels (usually the number of DAC chips in series)
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  bits-per-channel:
+    description:
+       Number of bits for each DAC output.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dac@1 {
+            compatible = "spidac";
+            reg = <0x1>;
+            ldac-gpios = <&gpio 42 GPIO_ACTIVE_LOW>;
+        };
+    };
+...
-- 
2.34.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

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

[Mike Looijmans]

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;
+	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;
+
+	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);
+
+	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;
+
+	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;
+
+	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);
+	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;
+	}
+
+	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);
+	}
+
+	return devm_iio_device_register(&spi->dev, iio_dev);
+}
+
+static const struct spi_device_id spidac_id[] = {
+	{"spi-dac"},
+	{}
+};
+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,
+		   },
+	.probe = spidac_probe,
+	.id_table = spidac_id,
+};
+module_spi_driver(spidac_driver);
+
+MODULE_AUTHOR("Mike Looijmans <mike.looijmans@topic.nl>");
+MODULE_DESCRIPTION("SPI shift register DAC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

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

[Nuno Sá]

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á

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

[Mike Looijmans]

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

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


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

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


>
>> +       {}
>> +};
>> +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á
>

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

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

[Nuno Sá]

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

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

[Jonathan Cameron]

On Wed, 13 Dec 2023 10:09:10 +0100
Mike Looijmans <mike.looijmans@topic.nl> wrote:

> Add a driver for generic serial shift register DACs like TI DAC714.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 

Hi Mike,

In general looks good but I think we are going to have enough device specific stuff
here that we will want a compatible match.  Trying to push all the info to
DT is going to get messy fast.  Just take scaling.  There are all sorts of fun
things out there such as only use 3/4 of the reference voltage.

Various comments inline.

Jonathan

> new file mode 100644
> index 000000000000..0c0113d51604
> --- /dev/null
> +++ b/drivers/iio/dac/spi-dac.c
> @@ -0,0 +1,212 @@


> +
> +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;
> +	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;
> +
> +	ret = spi_write(priv->spi, priv->data, priv->data_size);
> +	if (ret)
> +		return ret;
> +
> +	gpiod_set_value(priv->loaddacs, 1);
> +	udelay(1);

Delay needs to come from somewhere.  Some devices will need longer.

> +	gpiod_set_value(priv->loaddacs, 0);
> +
> +	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 */

Why assume it's big endian? In theory at least it might not be
so I think this needs specific compatibles to be used.

> +	for (i = 0; i < bytes; i++, data++)
> +		val = (val << 8) | *data;
> +
> +	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;

If it defaults to 0, don't bother providing it. 
The actual scale here needs to come from somewhere - so I'd argue something
is needed in the dt-binding to indicate that it's a reference regulator, or
a fixed value or similar.  May need to use specific compatibles for that as the
scaling relationships can be a bit odd.


> +		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;
> +
> +	mutex_lock(&priv->lock);
> +	ret = spidac_cmd_single(priv, chan, val);
> +	mutex_unlock(&priv->lock);

	guard(mutex)(&priv->lock);
	return spi_dac_cmd_single()...

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

Use return dev_err_probe() for these as we want the debug info that
stores for deferred probing cases.

> +
> +	reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> +					     GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);
Same here.

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

As pointed out by Nuno, don't use GFP_DMA - its a historical artifact.

> +	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';

You aren't using it yet, so don't set it but we'd need information on this being a bipolar
channel to set this as s.  Otherwise normal unipolar ADCs are going to be interpreted as
negative by standard tooling.

> +		chan->scan_type.realbits = bits_per_channel;
> +		chan->scan_type.storagebits = bits_per_channel;

Nuno observed this one.  Fine to have realbits reflect the actual bits per channel
but storagebits is both only relevant if doing buffered storage and must be power of 2 (>= 8)

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

This is unfortunately possibly flaky when fallback compatibles get used.
I'd just hard code it for now.

> +
> +	mutex_init(&priv->lock);
> +
> +	if (reset_gpio) {
> +		udelay(1);
> +		gpiod_set_value(reset_gpio, 0);
> +	}
> +
> +	return devm_iio_device_register(&spi->dev, iio_dev);
> +}
> +
> +static const struct spi_device_id spidac_id[] = {
> +	{"spi-dac"},

Nuno pointed out why you need dac714 here.

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

No comma on terminating entries. Also, consistent spacing for {}

> +};
> +MODULE_DEVICE_TABLE(of, spidac_of_match);
> +
> +static struct spi_driver spidac_driver = {
> +	.driver = {
> +		   .name = "spi-dac",
> +		   .of_match_table = spidac_of_match,
> +		   },
Align closing brackets as 
	},

> +	.probe = spidac_probe,
> +	.id_table = spidac_id,
> +};
> +module_spi_driver(spidac_driver);
> +
> +MODULE_AUTHOR("Mike Looijmans <mike.looijmans@topic.nl>");
> +MODULE_DESCRIPTION("SPI shift register DAC driver");
> +MODULE_LICENSE("GPL");

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

[Rob Herring]

On Wed, 13 Dec 2023 10:09: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>
> 
> ---
> 
>  .../devicetree/bindings/iio/dac/spidac.yaml   | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/spidac.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/dac/spidac.yaml:44:8: [warning] wrong indentation: expected 6 but found 7 (indentation)

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/iio/dac/spidac.example.dtb: /example-0/spi/dac@1: failed to match any schema with compatible: ['spidac']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231213090910.25410-1-mike.looijmans@topic.nl

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Rob Herring]

On Wed, Dec 13, 2023 at 10:09:09AM +0100, Mike Looijmans wrote:
> Add a driver for generic serial shift register DACs like TI DAC714.

This is not a driver.

> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> 
> ---
> 
>  .../devicetree/bindings/iio/dac/spidac.yaml   | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/spidac.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/spidac.yaml b/Documentation/devicetree/bindings/iio/dac/spidac.yaml
> new file mode 100644
> index 000000000000..be98da728594
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/spidac.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/spidac.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic "shift register" SPI DAC
> +
> +description:
> +  Supports simple SPI "shift register" DACs, like TI's DAC714. These DACs have
> +  no control registers or commands, they just use a clock and serial data to
> +  shift in a raw DAC value. Multiple DACs can be daisy-chained together.
> +
> +maintainers:
> +  - Mike Looijmans <mike.looijmans@topic.nl>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - spi-dac
> +      - ti,dac714

Why does TI chip need a specific compatible and others don't?

Are power supplies on these chips the same?

> +
> +  reg:
> +    maxItems: 1
> +
> +  ldac-gpios:
> +    description:
> +      LDAC pin to be used as a hardware trigger to update the DAC outputs. Not
> +      needed when the DACs use the chip select to update their output.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description:
> +      Optional reset pin that resets all DACs.
> +    maxItems: 1
> +
> +  num-channels:
> +    description:
> +      Number of channels (usually the number of DAC chips in series)

usually? What other possible option is there? If something else, how is 
the driver going to distinguish that?

default: 1

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +  bits-per-channel:

Perhaps 'channel-bits' as -bits is a standard unit suffix.

> +    description:
> +       Number of bits for each DAC output.
> +    $ref: /schemas/types.yaml#/definitions/uint32

Constraints? I assume all DACs are much less than 2^32 bits. default?

> +
> +required:
> +  - compatible
> +  - reg

Don't you always need to know how many bits?

> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dac@1 {
> +            compatible = "spidac";
> +            reg = <0x1>;
> +            ldac-gpios = <&gpio 42 GPIO_ACTIVE_LOW>;
> +        };
> +    };
> +...
> -- 
> 2.34.1
> 
> 
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topic.nl
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail