Message-ID: <20190126204155.6fbb9512@archlinux>
Date:   Sat, 26 Jan 2019 20:41:55 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Ricardo Ribalda Delgado <ricardo@...alda.com>
Cc:     Alexandru Ardelean <ardeleanalex@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        alexandru.ardelean@...log.com, linux-iio@...r.kernel.org
Subject: Re: [PATCH v2] iio:dac:ti-dac7612: Add driver for Texas Instruments
 DAC7612

On Fri, 25 Jan 2019 11:00:55 +0100
Ricardo Ribalda Delgado <ricardo@...alda.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@...alda.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@...alda.com>
> +L:	linux-iio@...r.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@...all.nl>
>  L:	linux-media@...r.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@...alda.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@...alda.com>");
> +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver");
> +MODULE_LICENSE("GPL v2");

Powered by blists - more mailing lists