| Message ID | 20190125100055.13836-1-ricardo@ribalda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] iio:dac:ti-dac7612: Add driver for Texas Instruments DAC7612 | expand |
On Fri, 25 Jan 2019 11:00:55 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > Digital-to-Analog Converter. > > Datasheet of this chip: > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> Hi Ricardo, Various bits and pieces inline. Jonathan > --- > > v2: Fix range > MAINTAINERS | 6 ++ > drivers/iio/dac/Kconfig | 10 +++ > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > 4 files changed, 186 insertions(+) > create mode 100644 drivers/iio/dac/ti-dac7612.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index d039f66a5cef..30ba5435906b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > F: drivers/clk/keystone/sci-clk.c > F: drivers/reset/reset-ti-sci.c > > +Texas Instruments' DAC7612 DAC Driver > +M: Ricardo Ribalda <ricardo@ribalda.com> > +L: linux-iio@vger.kernel.org > +S: Supported > +F: drivers/iio/dac/ti-dac7612.c > + > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > M: Hans Verkuil <hverkuil@xs4all.nl> > L: linux-media@vger.kernel.org > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > index f28daf67db6a..fbef9107acad 100644 > --- a/drivers/iio/dac/Kconfig > +++ b/drivers/iio/dac/Kconfig > @@ -375,6 +375,16 @@ config TI_DAC7311 > > If compiled as a module, it will be called ti-dac7311. > > +config TI_DAC7612 > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > + depends on SPI_MASTER && GPIOLIB > + help > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > + The driver hand drive the load pin automatically, otherwise > + it needs to be toggled manually. Given the driver doesn't load without that pin, do we need to give the otherwise? I would drop this comment entirely. > + > + If compiled as a module, it will be called ti-dac7612. > + > config VF610_DAC > tristate "Vybrid vf610 DAC driver" > depends on OF > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index f0a37c93de8e..1369fa1d2f0e 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > new file mode 100644 > index 000000000000..278406f69e0c > --- /dev/null > +++ b/drivers/iio/dac/ti-dac7612.c > @@ -0,0 +1,169 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > + * > + * Copyright 2019 Qtechnology A/S > + * 2019 Ricardo Ribalda <ricardo@ribalda.com> > + * > + * Licensed under the GPL-2. > + */ > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/iio.h> > + > +#define NUM_CHANS 2 > +#define RESOLUTION 12 Please prefix any naming that is generic like this with the driver name. Avoids potential redefined clashes in the future. Where they are 'real numbers' rather than Magic numbers I would generally look at using the actual number rather than a define. > + > +struct dac7612 { > + struct spi_device *spi; > + struct gpio_desc *nld; > + uint16_t cache[NUM_CHANS]; > +}; > + > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > +{ > + uint8_t buffer[2]; > + int ret; > + > + if (channel >= NUM_CHANS) Is there any way this can happen? Seems a little over paranoid. > + return -EINVAL; > + > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); BIT(5) is a magic number so should probably come from a define as should the shifts of the other parts. > + buffer[1] = val & 0xff; > + > + priv->cache[channel] = val; > + > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); spi write can potentially do a dma transfer so it needs a dma safe buffer. This one isn't as it is on the stack. Given it is a moderately fast path, best option is to put the buffer in priv and use the __cacheline_aligned trick (plus the fact private data is already cacheline aligned) to get it into it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good tutorial on this and is on youtube if you have time. https://www.youtube.com/watch?v=JDwaMClvV-s > + if (ret) > + return ret; > + > + gpiod_set_value(priv->nld, 0); > + gpiod_set_value(priv->nld, 1); > + > + return 0; > +} > + > +#define dac7612_CHANNEL(chan, name) { \ > + .type = IIO_VOLTAGE, \ > + .channel = (chan), \ > + .address = (chan), \ Not used, so don't set it. > + .indexed = true, \ These are bit fields, so whilst this works, = 1 is more conventional. > + .output = true, \ > + .datasheet_name = name, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = RESOLUTION, \ > + .storagebits = RESOLUTION, \ No need to provide scan_type as we don't have buffered output in mainline. Also this would be wrong as storagebits needs to be 8/16/32/64 etc. Would prefer a straight forward value like RESOLUTION was just expressed as a number. > + }, \ > +} > + > +static const struct iio_chan_spec dac7612_channels[] = { > + dac7612_CHANNEL(0, "OUTA"), > + dac7612_CHANNEL(1, "OUTB"), > +}; > + > +static int dac7612_read_raw(struct iio_dev *iio_dev, > + const struct iio_chan_spec *chan, > + int *val, int *val2, long mask) > +{ > + struct dac7612 *priv; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + priv = iio_priv(iio_dev); > + *val = priv->cache[chan->channel]; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 1000000; This makes me wonder. The units of voltage are millivolts, so is raw value * 1000000 = value in mv? That would make this a very high voltage device. See Documentation/ABI/testing/sysfs-bus-iio* for IIO ABI documentation. > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > +} > + > +static int dac7612_write_raw(struct iio_dev *iio_dev, > + const struct iio_chan_spec *chan, > + int val, int val2, long mask) > +{ > + struct dac7612 *priv; struct dac7612 *priv = iio_priv(iio_dev); (it's just macro magic to get the right point so normally considered fine to be called like this). > + int ret; > + > + if (mask != IIO_CHAN_INFO_RAW) > + return -EINVAL; > + > + if ((val >= BIT(RESOLUTION)) || val < 0) > + return -EINVAL; As not providing a write_fmt function (which is fine) should also sanity check that val2 is 0. > + > + priv = iio_priv(iio_dev); > + if (val == priv->cache[chan->channel]) > + return 0; > + > + mutex_lock(&iio_dev->mlock); > + ret = dac7612_cmd_single(priv, chan->channel, val); > + mutex_unlock(&iio_dev->mlock); > + > + return ret; > +} > + > +static const struct iio_info dac7612_info = { > + .read_raw = dac7612_read_raw, > + .write_raw = dac7612_write_raw, > +}; > + > +static int dac7612_probe(struct spi_device *spi) > +{ > + struct iio_dev *iio_dev; > + struct dac7612 *priv; > + int i; > + int ret; > + > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > + if (!iio_dev) > + return -ENOMEM; > + > + priv = iio_priv(iio_dev); > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); This isn't a particularly standard gpio name. Hence this driver definitely needs a DT binding doc (remember to cc dt list and maintainers). Also this should almost certainly be prefixed with a vendor prefix. It's also not the name I'm seeing on the datasheet, so needs some justifying comments. > + if (IS_ERR(priv->nld)) > + return PTR_ERR(priv->nld); > + priv->spi = spi; > + spi_set_drvdata(spi, iio_dev); > + iio_dev->dev.parent = &spi->dev; > + iio_dev->info = &dac7612_info; > + iio_dev->modes = INDIO_DIRECT_MODE; > + iio_dev->channels = dac7612_channels; > + iio_dev->num_channels = NUM_CHANS; ARRAY_SIZE. > + iio_dev->name = spi_get_device_id(spi)->name; > + > + for (i = 0; i < NUM_CHANS; i++) { ARRAY_SIZE > + ret = dac7612_cmd_single(priv, i, 0); > + if (ret) > + return ret; > + } > + > + return devm_iio_device_register(&spi->dev, iio_dev); > +} > + > +static const struct spi_device_id dac7612_id[] = { > + {"ti-dac7612"}, > + {} > +}; > +MODULE_DEVICE_TABLE(spi, dac7612_id); > + > +static struct spi_driver dac7612_driver = { > + .driver = { > + .name = "ti-dac7612", > + }, > + .probe = dac7612_probe, > + .id_table = dac7612_id, > +}; > +module_spi_driver(dac7612_driver); > + > +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > +MODULE_LICENSE("GPL v2");
HI Jonathan Thanks for your review. I will make the changes and send it back to you after testing it on Monday on real hardware. Until then I have pushed my changes to https://github.com/ribalda/linux/tree/ribalda-iio-v3 in case you want to see it before I send it to the list. (I am still missing the dt-bindings) Some comments inline. Best regards! On Sat, Jan 26, 2019 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 25 Jan 2019 11:00:55 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > > Digital-to-Analog Converter. > > > > Datasheet of this chip: > > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > Hi Ricardo, > > Various bits and pieces inline. > > Jonathan > > > --- > > > > v2: Fix range > > MAINTAINERS | 6 ++ > > drivers/iio/dac/Kconfig | 10 +++ > > drivers/iio/dac/Makefile | 1 + > > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > > 4 files changed, 186 insertions(+) > > create mode 100644 drivers/iio/dac/ti-dac7612.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index d039f66a5cef..30ba5435906b 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > > F: drivers/clk/keystone/sci-clk.c > > F: drivers/reset/reset-ti-sci.c > > > > +Texas Instruments' DAC7612 DAC Driver > > +M: Ricardo Ribalda <ricardo@ribalda.com> > > +L: linux-iio@vger.kernel.org > > +S: Supported > > +F: drivers/iio/dac/ti-dac7612.c > > + > > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > > M: Hans Verkuil <hverkuil@xs4all.nl> > > L: linux-media@vger.kernel.org > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > > index f28daf67db6a..fbef9107acad 100644 > > --- a/drivers/iio/dac/Kconfig > > +++ b/drivers/iio/dac/Kconfig > > @@ -375,6 +375,16 @@ config TI_DAC7311 > > > > If compiled as a module, it will be called ti-dac7311. > > > > +config TI_DAC7612 > > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > > + depends on SPI_MASTER && GPIOLIB > > + help > > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > > + The driver hand drive the load pin automatically, otherwise > > + it needs to be toggled manually. > > Given the driver doesn't load without that pin, do we need to give > the otherwise? I would drop this comment entirely. I am probing the gpio with devm_gpiod_get_optional() so the driver can load without the pin > > > + > > + If compiled as a module, it will be called ti-dac7612. > > + > > config VF610_DAC > > tristate "Vybrid vf610 DAC driver" > > depends on OF > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > > index f0a37c93de8e..1369fa1d2f0e 100644 > > --- a/drivers/iio/dac/Makefile > > +++ b/drivers/iio/dac/Makefile > > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > > new file mode 100644 > > index 000000000000..278406f69e0c > > --- /dev/null > > +++ b/drivers/iio/dac/ti-dac7612.c > > @@ -0,0 +1,169 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > > + * > > + * Copyright 2019 Qtechnology A/S > > + * 2019 Ricardo Ribalda <ricardo@ribalda.com> > > + * > > + * Licensed under the GPL-2. > > + */ > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/spi/spi.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/iio/iio.h> > > + > > +#define NUM_CHANS 2 > > +#define RESOLUTION 12 > Please prefix any naming that is generic like this with > the driver name. Avoids potential redefined clashes in the future. > Where they are 'real numbers' rather than Magic numbers I would > generally look at using the actual number rather than a define. > > > + > > +struct dac7612 { > > + struct spi_device *spi; > > + struct gpio_desc *nld; > > + uint16_t cache[NUM_CHANS]; > > +}; > > + > > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > > +{ > > + uint8_t buffer[2]; > > + int ret; > > + > > + if (channel >= NUM_CHANS) > Is there any way this can happen? Seems a little over paranoid. > > + return -EINVAL; > > + > > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); > BIT(5) is a magic number so should probably come from a define > as should the shifts of the other parts. > > > + buffer[1] = val & 0xff; > > + > > + priv->cache[channel] = val; > > + > > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); > > spi write can potentially do a dma transfer so it needs > a dma safe buffer. This one isn't as it is on the stack. > Given it is a moderately fast path, best option is to put the > buffer in priv and use the __cacheline_aligned trick (plus the > fact private data is already cacheline aligned) to get it into > it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good > tutorial on this and is on youtube if you have time. > > https://www.youtube.com/watch?v=JDwaMClvV-s > > > + if (ret) > > + return ret; > > + > > + gpiod_set_value(priv->nld, 0); > > + gpiod_set_value(priv->nld, 1); > > + > > + return 0; > > +} > > + > > +#define dac7612_CHANNEL(chan, name) { \ > > + .type = IIO_VOLTAGE, \ > > + .channel = (chan), \ > > + .address = (chan), \ > Not used, so don't set it. > > > + .indexed = true, \ > These are bit fields, so whilst this works, = 1 is more conventional. > > > + .output = true, \ > > + .datasheet_name = name, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > + .scan_type = { \ > > + .sign = 'u', \ > > + .realbits = RESOLUTION, \ > > + .storagebits = RESOLUTION, \ > No need to provide scan_type as we don't have buffered output in > mainline. Also this would be wrong as storagebits needs to be > 8/16/32/64 etc. > > Would prefer a straight forward value like RESOLUTION was just > expressed as a number. > > > + }, \ > > +} > > + > > +static const struct iio_chan_spec dac7612_channels[] = { > > + dac7612_CHANNEL(0, "OUTA"), > > + dac7612_CHANNEL(1, "OUTB"), > > +}; > > + > > +static int dac7612_read_raw(struct iio_dev *iio_dev, > > + const struct iio_chan_spec *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct dac7612 *priv; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + priv = iio_priv(iio_dev); > > + *val = priv->cache[chan->channel]; > > + return IIO_VAL_INT; > > + > > + case IIO_CHAN_INFO_SCALE: > > + *val = 1000000; > > This makes me wonder. The units of voltage are millivolts, so is > raw value * 1000000 = value in mv? I tried using IIO_VAL_FRACTIONAL, with val = 1 and val2=1000000, and it only returned -EINVAL. Then I discovered the PLUS_MICRO and I want to remember that cating the range sysfs file returned: 0,000001, but I will try again on Monday. > > That would make this a very high voltage device. > See Documentation/ABI/testing/sysfs-bus-iio* for > IIO ABI documentation. > > > + return IIO_VAL_INT_PLUS_MICRO; > > + > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int dac7612_write_raw(struct iio_dev *iio_dev, > > + const struct iio_chan_spec *chan, > > + int val, int val2, long mask) > > +{ > > + struct dac7612 *priv; > > struct dac7612 *priv = iio_priv(iio_dev); > > (it's just macro magic to get the right point so normally considered > fine to be called like this). > > > > + int ret; > > + > > + if (mask != IIO_CHAN_INFO_RAW) > > + return -EINVAL; > > + > > + if ((val >= BIT(RESOLUTION)) || val < 0) > > + return -EINVAL; > > As not providing a write_fmt function (which is fine) > should also sanity check that val2 is 0. > > > + > > + priv = iio_priv(iio_dev); > > + if (val == priv->cache[chan->channel]) > > + return 0; > > + > > + mutex_lock(&iio_dev->mlock); > > + ret = dac7612_cmd_single(priv, chan->channel, val); > > + mutex_unlock(&iio_dev->mlock); > > + > > + return ret; > > +} > > + > > +static const struct iio_info dac7612_info = { > > + .read_raw = dac7612_read_raw, > > + .write_raw = dac7612_write_raw, > > +}; > > + > > +static int dac7612_probe(struct spi_device *spi) > > +{ > > + struct iio_dev *iio_dev; > > + struct dac7612 *priv; > > + int i; > > + int ret; > > + > > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > > + if (!iio_dev) > > + return -ENOMEM; > > + > > + priv = iio_priv(iio_dev); > > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); > > This isn't a particularly standard gpio name. Hence this driver definitely > needs a DT binding doc (remember to cc dt list and maintainers). Also > this should almost certainly be prefixed with a vendor prefix. > > It's also not the name I'm seeing on the datasheet, so needs some justifying > comments. > > > + if (IS_ERR(priv->nld)) > > + return PTR_ERR(priv->nld); > > + priv->spi = spi; > > + spi_set_drvdata(spi, iio_dev); > > + iio_dev->dev.parent = &spi->dev; > > + iio_dev->info = &dac7612_info; > > + iio_dev->modes = INDIO_DIRECT_MODE; > > + iio_dev->channels = dac7612_channels; > > + iio_dev->num_channels = NUM_CHANS; > ARRAY_SIZE. > > + iio_dev->name = spi_get_device_id(spi)->name; > > + > > + for (i = 0; i < NUM_CHANS; i++) { > ARRAY_SIZE > > + ret = dac7612_cmd_single(priv, i, 0); > > + if (ret) > > + return ret; > > + } > > + > > + return devm_iio_device_register(&spi->dev, iio_dev); > > +} > > + > > +static const struct spi_device_id dac7612_id[] = { > > + {"ti-dac7612"}, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(spi, dac7612_id); > > + > > +static struct spi_driver dac7612_driver = { > > + .driver = { > > + .name = "ti-dac7612", > > + }, > > + .probe = dac7612_probe, > > + .id_table = dac7612_id, > > +}; > > +module_spi_driver(dac7612_driver); > > + > > +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); > > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > > +MODULE_LICENSE("GPL v2"); >
On Sat, 26 Jan 2019 22:34:50 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > HI Jonathan > > Thanks for your review. I will make the changes and send it back to > you after testing it on Monday on real hardware. > > Until then I have pushed my changes to > https://github.com/ribalda/linux/tree/ribalda-iio-v3 in case you want > to see it before I send it to the list. > (I am still missing the dt-bindings) > > Some comments inline. Replies inline. > > Best regards! > > On Sat, Jan 26, 2019 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 25 Jan 2019 11:00:55 +0100 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > > > Digital-to-Analog Converter. > > > > > > Datasheet of this chip: > > > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > Hi Ricardo, > > > > Various bits and pieces inline. > > > > Jonathan > > > > > --- > > > > > > v2: Fix range > > > MAINTAINERS | 6 ++ > > > drivers/iio/dac/Kconfig | 10 +++ > > > drivers/iio/dac/Makefile | 1 + > > > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > > > 4 files changed, 186 insertions(+) > > > create mode 100644 drivers/iio/dac/ti-dac7612.c > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index d039f66a5cef..30ba5435906b 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > > > F: drivers/clk/keystone/sci-clk.c > > > F: drivers/reset/reset-ti-sci.c > > > > > > +Texas Instruments' DAC7612 DAC Driver > > > +M: Ricardo Ribalda <ricardo@ribalda.com> > > > +L: linux-iio@vger.kernel.org > > > +S: Supported > > > +F: drivers/iio/dac/ti-dac7612.c > > > + > > > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > > > M: Hans Verkuil <hverkuil@xs4all.nl> > > > L: linux-media@vger.kernel.org > > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > > > index f28daf67db6a..fbef9107acad 100644 > > > --- a/drivers/iio/dac/Kconfig > > > +++ b/drivers/iio/dac/Kconfig > > > @@ -375,6 +375,16 @@ config TI_DAC7311 > > > > > > If compiled as a module, it will be called ti-dac7311. > > > > > > +config TI_DAC7612 > > > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > > > + depends on SPI_MASTER && GPIOLIB > > > + help > > > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > > > + The driver hand drive the load pin automatically, otherwise > > > + it needs to be toggled manually. > > > > Given the driver doesn't load without that pin, do we need to give > > the otherwise? I would drop this comment entirely. > > I am probing the gpio with devm_gpiod_get_optional() so the driver can > load without the pin Hmm. I thought that would blow up when you tried to set the output but it seems not as the gpiod function just returns without doing anything. So is this a an actual usecase you have? My inclination would be to just make it non optional otherwise. The thought of relying on a random pulse from another device doesn't seem like a good idea to me. > > > > > > + > > > + If compiled as a module, it will be called ti-dac7612. > > > + > > > config VF610_DAC > > > tristate "Vybrid vf610 DAC driver" > > > depends on OF > > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > > > index f0a37c93de8e..1369fa1d2f0e 100644 > > > --- a/drivers/iio/dac/Makefile > > > +++ b/drivers/iio/dac/Makefile > > > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > > > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > > > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > > > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > > > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > > > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > > > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > > > new file mode 100644 > > > index 000000000000..278406f69e0c > > > --- /dev/null > > > +++ b/drivers/iio/dac/ti-dac7612.c > > > @@ -0,0 +1,169 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > > > + * > > > + * Copyright 2019 Qtechnology A/S > > > + * 2019 Ricardo Ribalda <ricardo@ribalda.com> > > > + * > > > + * Licensed under the GPL-2. > > > + */ > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/spi/spi.h> > > > +#include <linux/gpio/consumer.h> > > > +#include <linux/iio/iio.h> > > > + > > > +#define NUM_CHANS 2 > > > +#define RESOLUTION 12 > > Please prefix any naming that is generic like this with > > the driver name. Avoids potential redefined clashes in the future. > > Where they are 'real numbers' rather than Magic numbers I would > > generally look at using the actual number rather than a define. > > > > > + > > > +struct dac7612 { > > > + struct spi_device *spi; > > > + struct gpio_desc *nld; > > > + uint16_t cache[NUM_CHANS]; > > > +}; > > > + > > > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > > > +{ > > > + uint8_t buffer[2]; > > > + int ret; > > > + > > > + if (channel >= NUM_CHANS) > > Is there any way this can happen? Seems a little over paranoid. > > > + return -EINVAL; > > > + > > > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); > > BIT(5) is a magic number so should probably come from a define > > as should the shifts of the other parts. > > > > > + buffer[1] = val & 0xff; > > > + > > > + priv->cache[channel] = val; > > > + > > > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); > > > > spi write can potentially do a dma transfer so it needs > > a dma safe buffer. This one isn't as it is on the stack. > > Given it is a moderately fast path, best option is to put the > > buffer in priv and use the __cacheline_aligned trick (plus the > > fact private data is already cacheline aligned) to get it into > > it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good > > tutorial on this and is on youtube if you have time. > > > > https://www.youtube.com/watch?v=JDwaMClvV-s > > > > > + if (ret) > > > + return ret; > > > + > > > + gpiod_set_value(priv->nld, 0); > > > + gpiod_set_value(priv->nld, 1); > > > + > > > + return 0; > > > +} > > > + > > > +#define dac7612_CHANNEL(chan, name) { \ > > > + .type = IIO_VOLTAGE, \ > > > + .channel = (chan), \ > > > + .address = (chan), \ > > Not used, so don't set it. > > > > > + .indexed = true, \ > > These are bit fields, so whilst this works, = 1 is more conventional. > > > > > + .output = true, \ > > > + .datasheet_name = name, \ > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > > + .scan_type = { \ > > > + .sign = 'u', \ > > > + .realbits = RESOLUTION, \ > > > + .storagebits = RESOLUTION, \ > > No need to provide scan_type as we don't have buffered output in > > mainline. Also this would be wrong as storagebits needs to be > > 8/16/32/64 etc. > > > > Would prefer a straight forward value like RESOLUTION was just > > expressed as a number. > > > > > + }, \ > > > +} > > > + > > > +static const struct iio_chan_spec dac7612_channels[] = { > > > + dac7612_CHANNEL(0, "OUTA"), > > > + dac7612_CHANNEL(1, "OUTB"), > > > +}; > > > + > > > +static int dac7612_read_raw(struct iio_dev *iio_dev, > > > + const struct iio_chan_spec *chan, > > > + int *val, int *val2, long mask) > > > +{ > > > + struct dac7612 *priv; > > > + > > > + switch (mask) { > > > + case IIO_CHAN_INFO_RAW: > > > + priv = iio_priv(iio_dev); > > > + *val = priv->cache[chan->channel]; > > > + return IIO_VAL_INT; > > > + > > > + case IIO_CHAN_INFO_SCALE: > > > + *val = 1000000; > > > > This makes me wonder. The units of voltage are millivolts, so is > > raw value * 1000000 = value in mv? > > I tried using IIO_VAL_FRACTIONAL, with val = 1 and val2=1000000, and > it only returned -EINVAL. That is odd and definitely worth chasing down the reason. > Then I discovered the PLUS_MICRO and I want to remember that cating > the range sysfs file returned: > 0,000001, but I will try again on Monday. > > > > > That would make this a very high voltage device. > > See Documentation/ABI/testing/sysfs-bus-iio* for > > IIO ABI documentation. > > > > > + return IIO_VAL_INT_PLUS_MICRO; > > > + > > > + default: > > > + return -EINVAL; > > > + } > > > +} > > > + > > > +static int dac7612_write_raw(struct iio_dev *iio_dev, > > > + const struct iio_chan_spec *chan, > > > + int val, int val2, long mask) > > > +{ > > > + struct dac7612 *priv; > > > > struct dac7612 *priv = iio_priv(iio_dev); > > > > (it's just macro magic to get the right point so normally considered > > fine to be called like this). > > > > > > > + int ret; > > > + > > > + if (mask != IIO_CHAN_INFO_RAW) > > > + return -EINVAL; > > > + > > > + if ((val >= BIT(RESOLUTION)) || val < 0) > > > + return -EINVAL; > > > > As not providing a write_fmt function (which is fine) > > should also sanity check that val2 is 0. > > > > > + > > > + priv = iio_priv(iio_dev); > > > + if (val == priv->cache[chan->channel]) > > > + return 0; > > > + > > > + mutex_lock(&iio_dev->mlock); > > > + ret = dac7612_cmd_single(priv, chan->channel, val); > > > + mutex_unlock(&iio_dev->mlock); > > > + > > > + return ret; > > > +} > > > + > > > +static const struct iio_info dac7612_info = { > > > + .read_raw = dac7612_read_raw, > > > + .write_raw = dac7612_write_raw, > > > +}; > > > + > > > +static int dac7612_probe(struct spi_device *spi) > > > +{ > > > + struct iio_dev *iio_dev; > > > + struct dac7612 *priv; > > > + int i; > > > + int ret; > > > + > > > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > > > + if (!iio_dev) > > > + return -ENOMEM; > > > + > > > + priv = iio_priv(iio_dev); > > > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); > > > > This isn't a particularly standard gpio name. Hence this driver definitely > > needs a DT binding doc (remember to cc dt list and maintainers). Also > > this should almost certainly be prefixed with a vendor prefix. > > > > It's also not the name I'm seeing on the datasheet, so needs some justifying > > comments. > > > > > + if (IS_ERR(priv->nld)) > > > + return PTR_ERR(priv->nld); > > > + priv->spi = spi; > > > + spi_set_drvdata(spi, iio_dev); > > > + iio_dev->dev.parent = &spi->dev; > > > + iio_dev->info = &dac7612_info; > > > + iio_dev->modes = INDIO_DIRECT_MODE; > > > + iio_dev->channels = dac7612_channels; > > > + iio_dev->num_channels = NUM_CHANS; > > ARRAY_SIZE. > > > + iio_dev->name = spi_get_device_id(spi)->name; > > > + > > > + for (i = 0; i < NUM_CHANS; i++) { > > ARRAY_SIZE > > > + ret = dac7612_cmd_single(priv, i, 0); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + return devm_iio_device_register(&spi->dev, iio_dev); > > > +} > > > + > > > +static const struct spi_device_id dac7612_id[] = { > > > + {"ti-dac7612"}, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(spi, dac7612_id); > > > + > > > +static struct spi_driver dac7612_driver = { > > > + .driver = { > > > + .name = "ti-dac7612", > > > + }, > > > + .probe = dac7612_probe, > > > + .id_table = dac7612_id, > > > +}; > > > +module_spi_driver(dac7612_driver); > > > + > > > +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); > > > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > > > +MODULE_LICENSE("GPL v2"); > > > >
Hi Jonathan On Sun, Jan 27, 2019 at 4:04 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 26 Jan 2019 22:34:50 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > HI Jonathan > > > > Thanks for your review. I will make the changes and send it back to > > you after testing it on Monday on real hardware. > > > > Until then I have pushed my changes to > > https://github.com/ribalda/linux/tree/ribalda-iio-v3 in case you want > > to see it before I send it to the list. > > (I am still missing the dt-bindings) > > > > Some comments inline. > Replies inline. > > > > > Best regards! > > > > On Sat, Jan 26, 2019 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Fri, 25 Jan 2019 11:00:55 +0100 > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > > > > Digital-to-Analog Converter. > > > > > > > > Datasheet of this chip: > > > > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > Hi Ricardo, > > > > > > Various bits and pieces inline. > > > > > > Jonathan > > > > > > > --- > > > > > > > > v2: Fix range > > > > MAINTAINERS | 6 ++ > > > > drivers/iio/dac/Kconfig | 10 +++ > > > > drivers/iio/dac/Makefile | 1 + > > > > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 186 insertions(+) > > > > create mode 100644 drivers/iio/dac/ti-dac7612.c > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > index d039f66a5cef..30ba5435906b 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > > > > F: drivers/clk/keystone/sci-clk.c > > > > F: drivers/reset/reset-ti-sci.c > > > > > > > > +Texas Instruments' DAC7612 DAC Driver > > > > +M: Ricardo Ribalda <ricardo@ribalda.com> > > > > +L: linux-iio@vger.kernel.org > > > > +S: Supported > > > > +F: drivers/iio/dac/ti-dac7612.c > > > > + > > > > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > > > > M: Hans Verkuil <hverkuil@xs4all.nl> > > > > L: linux-media@vger.kernel.org > > > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > > > > index f28daf67db6a..fbef9107acad 100644 > > > > --- a/drivers/iio/dac/Kconfig > > > > +++ b/drivers/iio/dac/Kconfig > > > > @@ -375,6 +375,16 @@ config TI_DAC7311 > > > > > > > > If compiled as a module, it will be called ti-dac7311. > > > > > > > > +config TI_DAC7612 > > > > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > > > > + depends on SPI_MASTER && GPIOLIB > > > > + help > > > > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > > > > + The driver hand drive the load pin automatically, otherwise > > > > + it needs to be toggled manually. > > > > > > Given the driver doesn't load without that pin, do we need to give > > > the otherwise? I would drop this comment entirely. > > > > I am probing the gpio with devm_gpiod_get_optional() so the driver can > > load without the pin > Hmm. I thought that would blow up when you tried to set the output but > it seems not as the gpiod function just returns without doing anything. > > So is this a an actual usecase you have? My inclination would be to just > make it non optional otherwise. The thought of relying on a random pulse > from another device doesn't seem like a good idea to me. I am thinking on a usecase where both voltages need to be set at the same time or on sync with an external event. This is how our board was designed 6 years ago: http://ge.tt/28oRQCu2 But, up to date no one is using it that way :). All that said, the only change to support that usecase is to use the _optional() function, that does not affect the readability of the code. But if it is a showstopper for you I will remove it and make the gpio "mandatory" Thanks and best regatrds! > > > > > > > > > > + > > > > + If compiled as a module, it will be called ti-dac7612. > > > > + > > > > config VF610_DAC > > > > tristate "Vybrid vf610 DAC driver" > > > > depends on OF > > > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > > > > index f0a37c93de8e..1369fa1d2f0e 100644 > > > > --- a/drivers/iio/dac/Makefile > > > > +++ b/drivers/iio/dac/Makefile > > > > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > > > > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > > > > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > > > > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > > > > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > > > > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > > > > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > > > > new file mode 100644 > > > > index 000000000000..278406f69e0c > > > > --- /dev/null > > > > +++ b/drivers/iio/dac/ti-dac7612.c > > > > @@ -0,0 +1,169 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > > > > + * > > > > + * Copyright 2019 Qtechnology A/S > > > > + * 2019 Ricardo Ribalda <ricardo@ribalda.com> > > > > + * > > > > + * Licensed under the GPL-2. > > > > + */ > > > > +#include <linux/kernel.h> > > > > +#include <linux/module.h> > > > > +#include <linux/spi/spi.h> > > > > +#include <linux/gpio/consumer.h> > > > > +#include <linux/iio/iio.h> > > > > + > > > > +#define NUM_CHANS 2 > > > > +#define RESOLUTION 12 > > > Please prefix any naming that is generic like this with > > > the driver name. Avoids potential redefined clashes in the future. > > > Where they are 'real numbers' rather than Magic numbers I would > > > generally look at using the actual number rather than a define. > > > > > > > + > > > > +struct dac7612 { > > > > + struct spi_device *spi; > > > > + struct gpio_desc *nld; > > > > + uint16_t cache[NUM_CHANS]; > > > > +}; > > > > + > > > > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > > > > +{ > > > > + uint8_t buffer[2]; > > > > + int ret; > > > > + > > > > + if (channel >= NUM_CHANS) > > > Is there any way this can happen? Seems a little over paranoid. > > > > + return -EINVAL; > > > > + > > > > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); > > > BIT(5) is a magic number so should probably come from a define > > > as should the shifts of the other parts. > > > > > > > + buffer[1] = val & 0xff; > > > > + > > > > + priv->cache[channel] = val; > > > > + > > > > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); > > > > > > spi write can potentially do a dma transfer so it needs > > > a dma safe buffer. This one isn't as it is on the stack. > > > Given it is a moderately fast path, best option is to put the > > > buffer in priv and use the __cacheline_aligned trick (plus the > > > fact private data is already cacheline aligned) to get it into > > > it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good > > > tutorial on this and is on youtube if you have time. > > > > > > https://www.youtube.com/watch?v=JDwaMClvV-s > > > > > > > + if (ret) > > > > + return ret; > > > > + > > > > + gpiod_set_value(priv->nld, 0); > > > > + gpiod_set_value(priv->nld, 1); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +#define dac7612_CHANNEL(chan, name) { \ > > > > + .type = IIO_VOLTAGE, \ > > > > + .channel = (chan), \ > > > > + .address = (chan), \ > > > Not used, so don't set it. > > > > > > > + .indexed = true, \ > > > These are bit fields, so whilst this works, = 1 is more conventional. > > > > > > > + .output = true, \ > > > > + .datasheet_name = name, \ > > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > > > + .scan_type = { \ > > > > + .sign = 'u', \ > > > > + .realbits = RESOLUTION, \ > > > > + .storagebits = RESOLUTION, \ > > > No need to provide scan_type as we don't have buffered output in > > > mainline. Also this would be wrong as storagebits needs to be > > > 8/16/32/64 etc. > > > > > > Would prefer a straight forward value like RESOLUTION was just > > > expressed as a number. > > > > > > > + }, \ > > > > +} > > > > + > > > > +static const struct iio_chan_spec dac7612_channels[] = { > > > > + dac7612_CHANNEL(0, "OUTA"), > > > > + dac7612_CHANNEL(1, "OUTB"), > > > > +}; > > > > + > > > > +static int dac7612_read_raw(struct iio_dev *iio_dev, > > > > + const struct iio_chan_spec *chan, > > > > + int *val, int *val2, long mask) > > > > +{ > > > > + struct dac7612 *priv; > > > > + > > > > + switch (mask) { > > > > + case IIO_CHAN_INFO_RAW: > > > > + priv = iio_priv(iio_dev); > > > > + *val = priv->cache[chan->channel]; > > > > + return IIO_VAL_INT; > > > > + > > > > + case IIO_CHAN_INFO_SCALE: > > > > + *val = 1000000; > > > > > > This makes me wonder. The units of voltage are millivolts, so is > > > raw value * 1000000 = value in mv? > > > > I tried using IIO_VAL_FRACTIONAL, with val = 1 and val2=1000000, and > > it only returned -EINVAL. > > That is odd and definitely worth chasing down the reason. > > > Then I discovered the PLUS_MICRO and I want to remember that cating > > the range sysfs file returned: > > 0,000001, but I will try again on Monday. > > > > > > > > That would make this a very high voltage device. > > > See Documentation/ABI/testing/sysfs-bus-iio* for > > > IIO ABI documentation. > > > > > > > + return IIO_VAL_INT_PLUS_MICRO; > > > > + > > > > + default: > > > > + return -EINVAL; > > > > + } > > > > +} > > > > + > > > > +static int dac7612_write_raw(struct iio_dev *iio_dev, > > > > + const struct iio_chan_spec *chan, > > > > + int val, int val2, long mask) > > > > +{ > > > > + struct dac7612 *priv; > > > > > > struct dac7612 *priv = iio_priv(iio_dev); > > > > > > (it's just macro magic to get the right point so normally considered > > > fine to be called like this). > > > > > > > > > > + int ret; > > > > + > > > > + if (mask != IIO_CHAN_INFO_RAW) > > > > + return -EINVAL; > > > > + > > > > + if ((val >= BIT(RESOLUTION)) || val < 0) > > > > + return -EINVAL; > > > > > > As not providing a write_fmt function (which is fine) > > > should also sanity check that val2 is 0. > > > > > > > + > > > > + priv = iio_priv(iio_dev); > > > > + if (val == priv->cache[chan->channel]) > > > > + return 0; > > > > + > > > > + mutex_lock(&iio_dev->mlock); > > > > + ret = dac7612_cmd_single(priv, chan->channel, val); > > > > + mutex_unlock(&iio_dev->mlock); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static const struct iio_info dac7612_info = { > > > > + .read_raw = dac7612_read_raw, > > > > + .write_raw = dac7612_write_raw, > > > > +}; > > > > + > > > > +static int dac7612_probe(struct spi_device *spi) > > > > +{ > > > > + struct iio_dev *iio_dev; > > > > + struct dac7612 *priv; > > > > + int i; > > > > + int ret; > > > > + > > > > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > > > > + if (!iio_dev) > > > > + return -ENOMEM; > > > > + > > > > + priv = iio_priv(iio_dev); > > > > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); > > > > > > This isn't a particularly standard gpio name. Hence this driver definitely > > > needs a DT binding doc (remember to cc dt list and maintainers). Also > > > this should almost certainly be prefixed with a vendor prefix. > > > > > > It's also not the name I'm seeing on the datasheet, so needs some justifying > > > comments. > > > > > > > + if (IS_ERR(priv->nld)) > > > > + return PTR_ERR(priv->nld); > > > > + priv->spi = spi; > > > > + spi_set_drvdata(spi, iio_dev); > > > > + iio_dev->dev.parent = &spi->dev; > > > > + iio_dev->info = &dac7612_info; > > > > + iio_dev->modes = INDIO_DIRECT_MODE; > > > > + iio_dev->channels = dac7612_channels; > > > > + iio_dev->num_channels = NUM_CHANS; > > > ARRAY_SIZE. > > > > + iio_dev->name = spi_get_device_id(spi)->name; > > > > + > > > > + for (i = 0; i < NUM_CHANS; i++) { > > > ARRAY_SIZE > > > > + ret = dac7612_cmd_single(priv, i, 0); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + return devm_iio_device_register(&spi->dev, iio_dev); > > > > +} > > > > + > > > > +static const struct spi_device_id dac7612_id[] = { > > > > + {"ti-dac7612"}, > > > > + {} > > > > +}; > > > > +MODULE_DEVICE_TABLE(spi, dac7612_id); > > > > + > > > > +static struct spi_driver dac7612_driver = { > > > > + .driver = { > > > > + .name = "ti-dac7612", > > > > + }, > > > > + .probe = dac7612_probe, > > > > + .id_table = dac7612_id, > > > > +}; > > > > +module_spi_driver(dac7612_driver); > > > > + > > > > +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); > > > > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > > > > +MODULE_LICENSE("GPL v2"); > > > > > > > > -- Ricardo Ribalda
On Mon, 28 Jan 2019 08:59:33 +0100 Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > Hi Jonathan > > On Sun, Jan 27, 2019 at 4:04 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sat, 26 Jan 2019 22:34:50 +0100 > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > HI Jonathan > > > > > > Thanks for your review. I will make the changes and send it back to > > > you after testing it on Monday on real hardware. > > > > > > Until then I have pushed my changes to > > > https://github.com/ribalda/linux/tree/ribalda-iio-v3 in case you want > > > to see it before I send it to the list. > > > (I am still missing the dt-bindings) > > > > > > Some comments inline. > > Replies inline. > > > > > > > > Best regards! > > > > > > On Sat, Jan 26, 2019 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > On Fri, 25 Jan 2019 11:00:55 +0100 > > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > > > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > > > > > Digital-to-Analog Converter. > > > > > > > > > > Datasheet of this chip: > > > > > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > Hi Ricardo, > > > > > > > > Various bits and pieces inline. > > > > > > > > Jonathan > > > > > > > > > --- > > > > > > > > > > v2: Fix range > > > > > MAINTAINERS | 6 ++ > > > > > drivers/iio/dac/Kconfig | 10 +++ > > > > > drivers/iio/dac/Makefile | 1 + > > > > > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 186 insertions(+) > > > > > create mode 100644 drivers/iio/dac/ti-dac7612.c > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > index d039f66a5cef..30ba5435906b 100644 > > > > > --- a/MAINTAINERS > > > > > +++ b/MAINTAINERS > > > > > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > > > > > F: drivers/clk/keystone/sci-clk.c > > > > > F: drivers/reset/reset-ti-sci.c > > > > > > > > > > +Texas Instruments' DAC7612 DAC Driver > > > > > +M: Ricardo Ribalda <ricardo@ribalda.com> > > > > > +L: linux-iio@vger.kernel.org > > > > > +S: Supported > > > > > +F: drivers/iio/dac/ti-dac7612.c > > > > > + > > > > > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > > > > > M: Hans Verkuil <hverkuil@xs4all.nl> > > > > > L: linux-media@vger.kernel.org > > > > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > > > > > index f28daf67db6a..fbef9107acad 100644 > > > > > --- a/drivers/iio/dac/Kconfig > > > > > +++ b/drivers/iio/dac/Kconfig > > > > > @@ -375,6 +375,16 @@ config TI_DAC7311 > > > > > > > > > > If compiled as a module, it will be called ti-dac7311. > > > > > > > > > > +config TI_DAC7612 > > > > > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > > > > > + depends on SPI_MASTER && GPIOLIB > > > > > + help > > > > > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > > > > > + The driver hand drive the load pin automatically, otherwise > > > > > + it needs to be toggled manually. > > > > > > > > Given the driver doesn't load without that pin, do we need to give > > > > the otherwise? I would drop this comment entirely. > > > > > > I am probing the gpio with devm_gpiod_get_optional() so the driver can > > > load without the pin > > Hmm. I thought that would blow up when you tried to set the output but > > it seems not as the gpiod function just returns without doing anything. > > > > So is this a an actual usecase you have? My inclination would be to just > > make it non optional otherwise. The thought of relying on a random pulse > > from another device doesn't seem like a good idea to me. > > I am thinking on a usecase where both voltages need to be set at the > same time or on sync with an external event. > This is how our board was designed 6 years ago: http://ge.tt/28oRQCu2 > > But, up to date no one is using it that way :). > > All that said, the only change to support that usecase is to use the > _optional() function, that does not affect the > readability of the code. But if it is a showstopper for you I will > remove it and make the gpio "mandatory" OK. Add a clear comment on why it is optional and we can leave it as such. Will need to be in the DT docs and the code. Jonathan > > Thanks and best regatrds! > > > > > > > > > > > > > > > > + > > > > > + If compiled as a module, it will be called ti-dac7612. > > > > > + > > > > > config VF610_DAC > > > > > tristate "Vybrid vf610 DAC driver" > > > > > depends on OF > > > > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > > > > > index f0a37c93de8e..1369fa1d2f0e 100644 > > > > > --- a/drivers/iio/dac/Makefile > > > > > +++ b/drivers/iio/dac/Makefile > > > > > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > > > > > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > > > > > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > > > > > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > > > > > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > > > > > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > > > > > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > > > > > new file mode 100644 > > > > > index 000000000000..278406f69e0c > > > > > --- /dev/null > > > > > +++ b/drivers/iio/dac/ti-dac7612.c > > > > > @@ -0,0 +1,169 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > +/* > > > > > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > > > > > + * > > > > > + * Copyright 2019 Qtechnology A/S > > > > > + * 2019 Ricardo Ribalda <ricardo@ribalda.com> > > > > > + * > > > > > + * Licensed under the GPL-2. > > > > > + */ > > > > > +#include <linux/kernel.h> > > > > > +#include <linux/module.h> > > > > > +#include <linux/spi/spi.h> > > > > > +#include <linux/gpio/consumer.h> > > > > > +#include <linux/iio/iio.h> > > > > > + > > > > > +#define NUM_CHANS 2 > > > > > +#define RESOLUTION 12 > > > > Please prefix any naming that is generic like this with > > > > the driver name. Avoids potential redefined clashes in the future. > > > > Where they are 'real numbers' rather than Magic numbers I would > > > > generally look at using the actual number rather than a define. > > > > > > > > > + > > > > > +struct dac7612 { > > > > > + struct spi_device *spi; > > > > > + struct gpio_desc *nld; > > > > > + uint16_t cache[NUM_CHANS]; > > > > > +}; > > > > > + > > > > > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > > > > > +{ > > > > > + uint8_t buffer[2]; > > > > > + int ret; > > > > > + > > > > > + if (channel >= NUM_CHANS) > > > > Is there any way this can happen? Seems a little over paranoid. > > > > > + return -EINVAL; > > > > > + > > > > > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); > > > > BIT(5) is a magic number so should probably come from a define > > > > as should the shifts of the other parts. > > > > > > > > > + buffer[1] = val & 0xff; > > > > > + > > > > > + priv->cache[channel] = val; > > > > > + > > > > > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); > > > > > > > > spi write can potentially do a dma transfer so it needs > > > > a dma safe buffer. This one isn't as it is on the stack. > > > > Given it is a moderately fast path, best option is to put the > > > > buffer in priv and use the __cacheline_aligned trick (plus the > > > > fact private data is already cacheline aligned) to get it into > > > > it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good > > > > tutorial on this and is on youtube if you have time. > > > > > > > > https://www.youtube.com/watch?v=JDwaMClvV-s > > > > > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + gpiod_set_value(priv->nld, 0); > > > > > + gpiod_set_value(priv->nld, 1); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +#define dac7612_CHANNEL(chan, name) { \ > > > > > + .type = IIO_VOLTAGE, \ > > > > > + .channel = (chan), \ > > > > > + .address = (chan), \ > > > > Not used, so don't set it. > > > > > > > > > + .indexed = true, \ > > > > These are bit fields, so whilst this works, = 1 is more conventional. > > > > > > > > > + .output = true, \ > > > > > + .datasheet_name = name, \ > > > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > > > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > > > > + .scan_type = { \ > > > > > + .sign = 'u', \ > > > > > + .realbits = RESOLUTION, \ > > > > > + .storagebits = RESOLUTION, \ > > > > No need to provide scan_type as we don't have buffered output in > > > > mainline. Also this would be wrong as storagebits needs to be > > > > 8/16/32/64 etc. > > > > > > > > Would prefer a straight forward value like RESOLUTION was just > > > > expressed as a number. > > > > > > > > > + }, \ > > > > > +} > > > > > + > > > > > +static const struct iio_chan_spec dac7612_channels[] = { > > > > > + dac7612_CHANNEL(0, "OUTA"), > > > > > + dac7612_CHANNEL(1, "OUTB"), > > > > > +}; > > > > > + > > > > > +static int dac7612_read_raw(struct iio_dev *iio_dev, > > > > > + const struct iio_chan_spec *chan, > > > > > + int *val, int *val2, long mask) > > > > > +{ > > > > > + struct dac7612 *priv; > > > > > + > > > > > + switch (mask) { > > > > > + case IIO_CHAN_INFO_RAW: > > > > > + priv = iio_priv(iio_dev); > > > > > + *val = priv->cache[chan->channel]; > > > > > + return IIO_VAL_INT; > > > > > + > > > > > + case IIO_CHAN_INFO_SCALE: > > > > > + *val = 1000000; > > > > > > > > This makes me wonder. The units of voltage are millivolts, so is > > > > raw value * 1000000 = value in mv? > > > > > > I tried using IIO_VAL_FRACTIONAL, with val = 1 and val2=1000000, and > > > it only returned -EINVAL. > > > > That is odd and definitely worth chasing down the reason. > > > > > Then I discovered the PLUS_MICRO and I want to remember that cating > > > the range sysfs file returned: > > > 0,000001, but I will try again on Monday. > > > > > > > > > > > That would make this a very high voltage device. > > > > See Documentation/ABI/testing/sysfs-bus-iio* for > > > > IIO ABI documentation. > > > > > > > > > + return IIO_VAL_INT_PLUS_MICRO; > > > > > + > > > > > + default: > > > > > + return -EINVAL; > > > > > + } > > > > > +} > > > > > + > > > > > +static int dac7612_write_raw(struct iio_dev *iio_dev, > > > > > + const struct iio_chan_spec *chan, > > > > > + int val, int val2, long mask) > > > > > +{ > > > > > + struct dac7612 *priv; > > > > > > > > struct dac7612 *priv = iio_priv(iio_dev); > > > > > > > > (it's just macro magic to get the right point so normally considered > > > > fine to be called like this). > > > > > > > > > > > > > + int ret; > > > > > + > > > > > + if (mask != IIO_CHAN_INFO_RAW) > > > > > + return -EINVAL; > > > > > + > > > > > + if ((val >= BIT(RESOLUTION)) || val < 0) > > > > > + return -EINVAL; > > > > > > > > As not providing a write_fmt function (which is fine) > > > > should also sanity check that val2 is 0. > > > > > > > > > + > > > > > + priv = iio_priv(iio_dev); > > > > > + if (val == priv->cache[chan->channel]) > > > > > + return 0; > > > > > + > > > > > + mutex_lock(&iio_dev->mlock); > > > > > + ret = dac7612_cmd_single(priv, chan->channel, val); > > > > > + mutex_unlock(&iio_dev->mlock); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static const struct iio_info dac7612_info = { > > > > > + .read_raw = dac7612_read_raw, > > > > > + .write_raw = dac7612_write_raw, > > > > > +}; > > > > > + > > > > > +static int dac7612_probe(struct spi_device *spi) > > > > > +{ > > > > > + struct iio_dev *iio_dev; > > > > > + struct dac7612 *priv; > > > > > + int i; > > > > > + int ret; > > > > > + > > > > > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > > > > > + if (!iio_dev) > > > > > + return -ENOMEM; > > > > > + > > > > > + priv = iio_priv(iio_dev); > > > > > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); > > > > > > > > This isn't a particularly standard gpio name. Hence this driver definitely > > > > needs a DT binding doc (remember to cc dt list and maintainers). Also > > > > this should almost certainly be prefixed with a vendor prefix. > > > > > > > > It's also not the name I'm seeing on the datasheet, so needs some justifying > > > > comments. > > > > > > > > > + if (IS_ERR(priv->nld)) > > > > > + return PTR_ERR(priv->nld); > > > > > + priv->spi = spi; > > > > > + spi_set_drvdata(spi, iio_dev); > > > > > + iio_dev->dev.parent = &spi->dev; > > > > > + iio_dev->info = &dac7612_info; > > > > > + iio_dev->modes = INDIO_DIRECT_MODE; > > > > > + iio_dev->channels = dac7612_channels; > > > > > + iio_dev->num_channels = NUM_CHANS; > > > > ARRAY_SIZE. > > > > > + iio_dev->name = spi_get_device_id(spi)->name; > > > > > + > > > > > + for (i = 0; i < NUM_CHANS; i++) { > > > > ARRAY_SIZE > > > > > + ret = dac7612_cmd_single(priv, i, 0); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > + > > > > > + return devm_iio_device_register(&spi->dev, iio_dev); > > > > > +} > > > > > + > > > > > +static const struct spi_device_id dac7612_id[] = { > > > > > + {"ti-dac7612"}, > > > > > + {} > > > > > +}; > > > > > +MODULE_DEVICE_TABLE(spi, dac7612_id); > > > > > + > > > > > +static struct spi_driver dac7612_driver = { > > > > > + .driver = { > > > > > + .name = "ti-dac7612", > > > > > + }, > > > > > + .probe = dac7612_probe, > > > > > + .id_table = dac7612_id, > > > > > +}; > > > > > +module_spi_driver(dac7612_driver); > > > > > + > > > > > +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); > > > > > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > > > > > +MODULE_LICENSE("GPL v2"); > > > > > > > > > > > > > > > -- > Ricardo Ribalda
Hi Jonathan On Tue, Jan 29, 2019 at 1:45 PM Jonathan Cameron <jonathan.cameron@huawei.com> wrote: > > On Mon, 28 Jan 2019 08:59:33 +0100 > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > Hi Jonathan > > > > On Sun, Jan 27, 2019 at 4:04 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Sat, 26 Jan 2019 22:34:50 +0100 > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > HI Jonathan > > > > > > > > Thanks for your review. I will make the changes and send it back to > > > > you after testing it on Monday on real hardware. > > > > > > > > Until then I have pushed my changes to > > > > https://github.com/ribalda/linux/tree/ribalda-iio-v3 in case you want > > > > to see it before I send it to the list. > > > > (I am still missing the dt-bindings) > > > > > > > > Some comments inline. > > > Replies inline. > > > > > > > > > > > Best regards! > > > > > > > > On Sat, Jan 26, 2019 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > > > On Fri, 25 Jan 2019 11:00:55 +0100 > > > > > Ricardo Ribalda Delgado <ricardo@ribalda.com> wrote: > > > > > > > > > > > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > > > > > > Digital-to-Analog Converter. > > > > > > > > > > > > Datasheet of this chip: > > > > > > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> > > > > > Hi Ricardo, > > > > > > > > > > Various bits and pieces inline. > > > > > > > > > > Jonathan > > > > > > > > > > > --- > > > > > > > > > > > > v2: Fix range > > > > > > MAINTAINERS | 6 ++ > > > > > > drivers/iio/dac/Kconfig | 10 +++ > > > > > > drivers/iio/dac/Makefile | 1 + > > > > > > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > > > > > > 4 files changed, 186 insertions(+) > > > > > > create mode 100644 drivers/iio/dac/ti-dac7612.c > > > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > > index d039f66a5cef..30ba5435906b 100644 > > > > > > --- a/MAINTAINERS > > > > > > +++ b/MAINTAINERS > > > > > > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > > > > > > F: drivers/clk/keystone/sci-clk.c > > > > > > F: drivers/reset/reset-ti-sci.c > > > > > > > > > > > > +Texas Instruments' DAC7612 DAC Driver > > > > > > +M: Ricardo Ribalda <ricardo@ribalda.com> > > > > > > +L: linux-iio@vger.kernel.org > > > > > > +S: Supported > > > > > > +F: drivers/iio/dac/ti-dac7612.c > > > > > > + > > > > > > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > > > > > > M: Hans Verkuil <hverkuil@xs4all.nl> > > > > > > L: linux-media@vger.kernel.org > > > > > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > > > > > > index f28daf67db6a..fbef9107acad 100644 > > > > > > --- a/drivers/iio/dac/Kconfig > > > > > > +++ b/drivers/iio/dac/Kconfig > > > > > > @@ -375,6 +375,16 @@ config TI_DAC7311 > > > > > > > > > > > > If compiled as a module, it will be called ti-dac7311. > > > > > > > > > > > > +config TI_DAC7612 > > > > > > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > > > > > > + depends on SPI_MASTER && GPIOLIB > > > > > > + help > > > > > > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > > > > > > + The driver hand drive the load pin automatically, otherwise > > > > > > + it needs to be toggled manually. > > > > > > > > > > Given the driver doesn't load without that pin, do we need to give > > > > > the otherwise? I would drop this comment entirely. > > > > > > > > I am probing the gpio with devm_gpiod_get_optional() so the driver can > > > > load without the pin > > > Hmm. I thought that would blow up when you tried to set the output but > > > it seems not as the gpiod function just returns without doing anything. > > > > > > So is this a an actual usecase you have? My inclination would be to just > > > make it non optional otherwise. The thought of relying on a random pulse > > > from another device doesn't seem like a good idea to me. > > > > I am thinking on a usecase where both voltages need to be set at the > > same time or on sync with an external event. > > This is how our board was designed 6 years ago: http://ge.tt/28oRQCu2 > > > > But, up to date no one is using it that way :). > > > > All that said, the only change to support that usecase is to use the > > _optional() function, that does not affect the > > readability of the code. But if it is a showstopper for you I will > > remove it and make the gpio "mandatory" > > OK. Add a clear comment on why it is optional and we can leave it as such. > Will need to be in the DT docs and the code. > Text on v3 is good enough or I make a v4 with a better explanation? Thanks! > Jonathan > > > > > Thanks and best regatrds! > > > > > > > > > > > > > > > > > > > > > > + > > > > > > + If compiled as a module, it will be called ti-dac7612. > > > > > > + > > > > > > config VF610_DAC > > > > > > tristate "Vybrid vf610 DAC driver" > > > > > > depends on OF > > > > > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > > > > > > index f0a37c93de8e..1369fa1d2f0e 100644 > > > > > > --- a/drivers/iio/dac/Makefile > > > > > > +++ b/drivers/iio/dac/Makefile > > > > > > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > > > > > > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > > > > > > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > > > > > > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > > > > > > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > > > > > > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > > > > > > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > > > > > > new file mode 100644 > > > > > > index 000000000000..278406f69e0c > > > > > > --- /dev/null > > > > > > +++ b/drivers/iio/dac/ti-dac7612.c > > > > > > @@ -0,0 +1,169 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > > +/* > > > > > > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > > > > > > + * > > > > > > + * Copyright 2019 Qtechnology A/S > > > > > > + * 2019 Ricardo Ribalda <ricardo@ribalda.com> > > > > > > + * > > > > > > + * Licensed under the GPL-2. > > > > > > + */ > > > > > > +#include <linux/kernel.h> > > > > > > +#include <linux/module.h> > > > > > > +#include <linux/spi/spi.h> > > > > > > +#include <linux/gpio/consumer.h> > > > > > > +#include <linux/iio/iio.h> > > > > > > + > > > > > > +#define NUM_CHANS 2 > > > > > > +#define RESOLUTION 12 > > > > > Please prefix any naming that is generic like this with > > > > > the driver name. Avoids potential redefined clashes in the future. > > > > > Where they are 'real numbers' rather than Magic numbers I would > > > > > generally look at using the actual number rather than a define. > > > > > > > > > > > + > > > > > > +struct dac7612 { > > > > > > + struct spi_device *spi; > > > > > > + struct gpio_desc *nld; > > > > > > + uint16_t cache[NUM_CHANS]; > > > > > > +}; > > > > > > + > > > > > > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > > > > > > +{ > > > > > > + uint8_t buffer[2]; > > > > > > + int ret; > > > > > > + > > > > > > + if (channel >= NUM_CHANS) > > > > > Is there any way this can happen? Seems a little over paranoid. > > > > > > + return -EINVAL; > > > > > > + > > > > > > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); > > > > > BIT(5) is a magic number so should probably come from a define > > > > > as should the shifts of the other parts. > > > > > > > > > > > + buffer[1] = val & 0xff; > > > > > > + > > > > > > + priv->cache[channel] = val; > > > > > > + > > > > > > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); > > > > > > > > > > spi write can potentially do a dma transfer so it needs > > > > > a dma safe buffer. This one isn't as it is on the stack. > > > > > Given it is a moderately fast path, best option is to put the > > > > > buffer in priv and use the __cacheline_aligned trick (plus the > > > > > fact private data is already cacheline aligned) to get it into > > > > > it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good > > > > > tutorial on this and is on youtube if you have time. > > > > > > > > > > https://www.youtube.com/watch?v=JDwaMClvV-s > > > > > > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + gpiod_set_value(priv->nld, 0); > > > > > > + gpiod_set_value(priv->nld, 1); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +#define dac7612_CHANNEL(chan, name) { \ > > > > > > + .type = IIO_VOLTAGE, \ > > > > > > + .channel = (chan), \ > > > > > > + .address = (chan), \ > > > > > Not used, so don't set it. > > > > > > > > > > > + .indexed = true, \ > > > > > These are bit fields, so whilst this works, = 1 is more conventional. > > > > > > > > > > > + .output = true, \ > > > > > > + .datasheet_name = name, \ > > > > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > > > > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > > > > > > + .scan_type = { \ > > > > > > + .sign = 'u', \ > > > > > > + .realbits = RESOLUTION, \ > > > > > > + .storagebits = RESOLUTION, \ > > > > > No need to provide scan_type as we don't have buffered output in > > > > > mainline. Also this would be wrong as storagebits needs to be > > > > > 8/16/32/64 etc. > > > > > > > > > > Would prefer a straight forward value like RESOLUTION was just > > > > > expressed as a number. > > > > > > > > > > > + }, \ > > > > > > +} > > > > > > + > > > > > > +static const struct iio_chan_spec dac7612_channels[] = { > > > > > > + dac7612_CHANNEL(0, "OUTA"), > > > > > > + dac7612_CHANNEL(1, "OUTB"), > > > > > > +}; > > > > > > + > > > > > > +static int dac7612_read_raw(struct iio_dev *iio_dev, > > > > > > + const struct iio_chan_spec *chan, > > > > > > + int *val, int *val2, long mask) > > > > > > +{ > > > > > > + struct dac7612 *priv; > > > > > > + > > > > > > + switch (mask) { > > > > > > + case IIO_CHAN_INFO_RAW: > > > > > > + priv = iio_priv(iio_dev); > > > > > > + *val = priv->cache[chan->channel]; > > > > > > + return IIO_VAL_INT; > > > > > > + > > > > > > + case IIO_CHAN_INFO_SCALE: > > > > > > + *val = 1000000; > > > > > > > > > > This makes me wonder. The units of voltage are millivolts, so is > > > > > raw value * 1000000 = value in mv? > > > > > > > > I tried using IIO_VAL_FRACTIONAL, with val = 1 and val2=1000000, and > > > > it only returned -EINVAL. > > > > > > That is odd and definitely worth chasing down the reason. > > > > > > > Then I discovered the PLUS_MICRO and I want to remember that cating > > > > the range sysfs file returned: > > > > 0,000001, but I will try again on Monday. > > > > > > > > > > > > > > That would make this a very high voltage device. > > > > > See Documentation/ABI/testing/sysfs-bus-iio* for > > > > > IIO ABI documentation. > > > > > > > > > > > + return IIO_VAL_INT_PLUS_MICRO; > > > > > > + > > > > > > + default: > > > > > > + return -EINVAL; > > > > > > + } > > > > > > +} > > > > > > + > > > > > > +static int dac7612_write_raw(struct iio_dev *iio_dev, > > > > > > + const struct iio_chan_spec *chan, > > > > > > + int val, int val2, long mask) > > > > > > +{ > > > > > > + struct dac7612 *priv; > > > > > > > > > > struct dac7612 *priv = iio_priv(iio_dev); > > > > > > > > > > (it's just macro magic to get the right point so normally considered > > > > > fine to be called like this). > > > > > > > > > > > > > > > > + int ret; > > > > > > + > > > > > > + if (mask != IIO_CHAN_INFO_RAW) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + if ((val >= BIT(RESOLUTION)) || val < 0) > > > > > > + return -EINVAL; > > > > > > > > > > As not providing a write_fmt function (which is fine) > > > > > should also sanity check that val2 is 0. > > > > > > > > > > > + > > > > > > + priv = iio_priv(iio_dev); > > > > > > + if (val == priv->cache[chan->channel]) > > > > > > + return 0; > > > > > > + > > > > > > + mutex_lock(&iio_dev->mlock); > > > > > > + ret = dac7612_cmd_single(priv, chan->channel, val); > > > > > > + mutex_unlock(&iio_dev->mlock); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > + > > > > > > +static const struct iio_info dac7612_info = { > > > > > > + .read_raw = dac7612_read_raw, > > > > > > + .write_raw = dac7612_write_raw, > > > > > > +}; > > > > > > + > > > > > > +static int dac7612_probe(struct spi_device *spi) > > > > > > +{ > > > > > > + struct iio_dev *iio_dev; > > > > > > + struct dac7612 *priv; > > > > > > + int i; > > > > > > + int ret; > > > > > > + > > > > > > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > > > > > > + if (!iio_dev) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + priv = iio_priv(iio_dev); > > > > > > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); > > > > > > > > > > This isn't a particularly standard gpio name. Hence this driver definitely > > > > > needs a DT binding doc (remember to cc dt list and maintainers). Also > > > > > this should almost certainly be prefixed with a vendor prefix. > > > > > > > > > > It's also not the name I'm seeing on the datasheet, so needs some justifying > > > > > comments. > > > > > > > > > > > + if (IS_ERR(priv->nld)) > > > > > > + return PTR_ERR(priv->nld); > > > > > > + priv->spi = spi; > > > > > > + spi_set_drvdata(spi, iio_dev); > > > > > > + iio_dev->dev.parent = &spi->dev; > > > > > > + iio_dev->info = &dac7612_info; > > > > > > + iio_dev->modes = INDIO_DIRECT_MODE; > > > > > > + iio_dev->channels = dac7612_channels; > > > > > > + iio_dev->num_channels = NUM_CHANS; > > > > > ARRAY_SIZE. > > > > > > + iio_dev->name = spi_get_device_id(spi)->name; > > > > > > + > > > > > > + for (i = 0; i < NUM_CHANS; i++) { > > > > > ARRAY_SIZE > > > > > > + ret = dac7612_cmd_single(priv, i, 0); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > + return devm_iio_device_register(&spi->dev, iio_dev); > > > > > > +} > > > > > > + > > > > > > +static const struct spi_device_id dac7612_id[] = { > > > > > > + {"ti-dac7612"}, > > > > > > + {} > > > > > > +}; > > > > > > +MODULE_DEVICE_TABLE(spi, dac7612_id); > > > > > > + > > > > > > +static struct spi_driver dac7612_driver = { > > > > > > + .driver = { > > > > > > + .name = "ti-dac7612", > > > > > > + }, > > > > > > + .probe = dac7612_probe, > > > > > > + .id_table = dac7612_id, > > > > > > +}; > > > > > > +module_spi_driver(dac7612_driver); > > > > > > + > > > > > > +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); > > > > > > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > > > > > > +MODULE_LICENSE("GPL v2"); > > > > > > > > > > > > > > > > > > > > > > -- > > Ricardo Ribalda > >
diff --git a/MAINTAINERS b/MAINTAINERS index d039f66a5cef..30ba5435906b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt F: drivers/clk/keystone/sci-clk.c F: drivers/reset/reset-ti-sci.c +Texas Instruments' DAC7612 DAC Driver +M: Ricardo Ribalda <ricardo@ribalda.com> +L: linux-iio@vger.kernel.org +S: Supported +F: drivers/iio/dac/ti-dac7612.c + THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <hverkuil@xs4all.nl> L: linux-media@vger.kernel.org diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index f28daf67db6a..fbef9107acad 100644 --- a/drivers/iio/dac/Kconfig +++ b/drivers/iio/dac/Kconfig @@ -375,6 +375,16 @@ config TI_DAC7311 If compiled as a module, it will be called ti-dac7311. +config TI_DAC7612 + tristate "Texas Instruments 12-bit 2-channel DAC driver" + depends on SPI_MASTER && GPIOLIB + help + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB + The driver hand drive the load pin automatically, otherwise + it needs to be toggled manually. + + If compiled as a module, it will be called ti-dac7612. + config VF610_DAC tristate "Vybrid vf610 DAC driver" depends on OF diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index f0a37c93de8e..1369fa1d2f0e 100644 --- a/drivers/iio/dac/Makefile +++ b/drivers/iio/dac/Makefile @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o obj-$(CONFIG_VF610_DAC) += vf610_dac.o diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c new file mode 100644 index 000000000000..278406f69e0c --- /dev/null +++ b/drivers/iio/dac/ti-dac7612.c @@ -0,0 +1,169 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter + * + * Copyright 2019 Qtechnology A/S + * 2019 Ricardo Ribalda <ricardo@ribalda.com> + * + * Licensed under the GPL-2. + */ +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/spi/spi.h> +#include <linux/gpio/consumer.h> +#include <linux/iio/iio.h> + +#define NUM_CHANS 2 +#define RESOLUTION 12 + +struct dac7612 { + struct spi_device *spi; + struct gpio_desc *nld; + uint16_t cache[NUM_CHANS]; +}; + +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) +{ + uint8_t buffer[2]; + int ret; + + if (channel >= NUM_CHANS) + return -EINVAL; + + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); + buffer[1] = val & 0xff; + + priv->cache[channel] = val; + + ret = spi_write(priv->spi, buffer, sizeof(buffer)); + if (ret) + return ret; + + gpiod_set_value(priv->nld, 0); + gpiod_set_value(priv->nld, 1); + + return 0; +} + +#define dac7612_CHANNEL(chan, name) { \ + .type = IIO_VOLTAGE, \ + .channel = (chan), \ + .address = (chan), \ + .indexed = true, \ + .output = true, \ + .datasheet_name = name, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .scan_type = { \ + .sign = 'u', \ + .realbits = RESOLUTION, \ + .storagebits = RESOLUTION, \ + }, \ +} + +static const struct iio_chan_spec dac7612_channels[] = { + dac7612_CHANNEL(0, "OUTA"), + dac7612_CHANNEL(1, "OUTB"), +}; + +static int dac7612_read_raw(struct iio_dev *iio_dev, + const struct iio_chan_spec *chan, + int *val, int *val2, long mask) +{ + struct dac7612 *priv; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + priv = iio_priv(iio_dev); + *val = priv->cache[chan->channel]; + return IIO_VAL_INT; + + case IIO_CHAN_INFO_SCALE: + *val = 1000000; + return IIO_VAL_INT_PLUS_MICRO; + + default: + return -EINVAL; + } +} + +static int dac7612_write_raw(struct iio_dev *iio_dev, + const struct iio_chan_spec *chan, + int val, int val2, long mask) +{ + struct dac7612 *priv; + int ret; + + if (mask != IIO_CHAN_INFO_RAW) + return -EINVAL; + + if ((val >= BIT(RESOLUTION)) || val < 0) + return -EINVAL; + + priv = iio_priv(iio_dev); + if (val == priv->cache[chan->channel]) + return 0; + + mutex_lock(&iio_dev->mlock); + ret = dac7612_cmd_single(priv, chan->channel, val); + mutex_unlock(&iio_dev->mlock); + + return ret; +} + +static const struct iio_info dac7612_info = { + .read_raw = dac7612_read_raw, + .write_raw = dac7612_write_raw, +}; + +static int dac7612_probe(struct spi_device *spi) +{ + struct iio_dev *iio_dev; + struct dac7612 *priv; + int i; + int ret; + + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); + if (!iio_dev) + return -ENOMEM; + + priv = iio_priv(iio_dev); + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); + if (IS_ERR(priv->nld)) + return PTR_ERR(priv->nld); + priv->spi = spi; + spi_set_drvdata(spi, iio_dev); + iio_dev->dev.parent = &spi->dev; + iio_dev->info = &dac7612_info; + iio_dev->modes = INDIO_DIRECT_MODE; + iio_dev->channels = dac7612_channels; + iio_dev->num_channels = NUM_CHANS; + iio_dev->name = spi_get_device_id(spi)->name; + + for (i = 0; i < NUM_CHANS; i++) { + ret = dac7612_cmd_single(priv, i, 0); + if (ret) + return ret; + } + + return devm_iio_device_register(&spi->dev, iio_dev); +} + +static const struct spi_device_id dac7612_id[] = { + {"ti-dac7612"}, + {} +}; +MODULE_DEVICE_TABLE(spi, dac7612_id); + +static struct spi_driver dac7612_driver = { + .driver = { + .name = "ti-dac7612", + }, + .probe = dac7612_probe, + .id_table = dac7612_id, +}; +module_spi_driver(dac7612_driver); + +MODULE_AUTHOR("Ricardo Ribalda <ricardo@ribalda.com>"); +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); +MODULE_LICENSE("GPL v2");
It is a driver for Texas Instruments Dual, 12-Bit Serial Input Digital-to-Analog Converter. Datasheet of this chip: http://www.ti.com/lit/ds/sbas106/sbas106.pdf Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com> --- v2: Fix range MAINTAINERS | 6 ++ drivers/iio/dac/Kconfig | 10 +++ drivers/iio/dac/Makefile | 1 + drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ 4 files changed, 186 insertions(+) create mode 100644 drivers/iio/dac/ti-dac7612.c