Message-ID: <20180819173850.3d55ea7e@archlinux>
Date:   Sun, 19 Aug 2018 17:38:50 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Marcus Folkesson <marcus.folkesson@...il.com>
Cc:     Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] iio: dac: add support for ltc166x

On Sat, 11 Aug 2018 22:02:24 +0200
Marcus Folkesson <marcus.folkesson@...il.com> wrote:

> LTC1665/LTC1660 is a 8/10-bit Digital-to-Analog Converter
> (DAC) with eight individual channels.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@...il.com>

So first rule we try to stick to that this breaks is never use wild cards
in drivers.  You probably wouldn't believe how often this has gone wrong with
a manufacturer deciding to use other values that wild card covers...

Please pick a part and name everything after that.  Either one is fine.

A few really minor things inline.  Nice little driver.

Thanks,

Jonathan


> ---
>  drivers/iio/dac/Kconfig   |  10 ++
>  drivers/iio/dac/Makefile  |   1 +
>  drivers/iio/dac/ltc166x.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 255 insertions(+)
>  create mode 100644 drivers/iio/dac/ltc166x.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 76db0768e454..04cfa6bb9dc1 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -120,6 +120,16 @@ config AD5624R_SPI
>  	  Say yes here to build support for Analog Devices AD5624R, AD5644R and
>  	  AD5664R converters (DAC). This driver uses the common SPI interface.
>  
> +config LTC166X
> +	tristate "Linear Technology LTC1660/LTC1665 DAC SPI driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Linear Technology
> +	  LTC1660 and LTC1665 Digital to Analog Converters.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ltc166x.
> +
>  config LTC2632
>  	tristate "Linear Technology LTC2632-12/10/8 DAC spi driver"
>  	depends on SPI
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 81e710ed7491..380749c87c26 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_CIO_DAC) += cio-dac.o
>  obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
>  obj-$(CONFIG_DS4424) += ds4424.o
>  obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
> +obj-$(CONFIG_LTC166X) += ltc166x.o
>  obj-$(CONFIG_LTC2632) += ltc2632.o
>  obj-$(CONFIG_M62332) += m62332.o
>  obj-$(CONFIG_MAX517) += max517.o
> diff --git a/drivers/iio/dac/ltc166x.c b/drivers/iio/dac/ltc166x.c
> new file mode 100644
> index 000000000000..0031f2b50f14
> --- /dev/null
> +++ b/drivers/iio/dac/ltc166x.c
> @@ -0,0 +1,244 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Linear Technology LTC1665/LTC1660, 8 channels DAC
> + *
> + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@...il.com>
> + */
> +#include <linux/bitops.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#define LTC166X_REG_WAKE	0x0
> +#define LTC166X_REG_DAC_A	0x1
> +#define LTC166X_REG_DAC_B	0x2
> +#define LTC166X_REG_DAC_C	0x3
> +#define LTC166X_REG_DAC_D	0x4
> +#define LTC166X_REG_DAC_E	0x5
> +#define LTC166X_REG_DAC_F	0x6
> +#define LTC166X_REG_DAC_G	0x7
> +#define LTC166X_REG_DAC_H	0x8
> +#define LTC166X_REG_SLEEP	0xe
> +
> +#define LTC166X_NUM_CHANNELS	8
> +
> +static const struct regmap_config ltc166x_regmap_config = {
> +	.reg_bits = 4,
> +	.val_bits = 12,
> +};
> +
> +enum ltc166x_supported_device_ids {
> +	ID_LTC1660,
> +	ID_LTC1665,
> +};
> +
> +struct ltc166x_priv {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	struct regulator *vref_reg;
> +	unsigned int value[LTC166X_NUM_CHANNELS];
> +	unsigned int vref_mv;
> +};
> +
> +static int ltc166x_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val,
> +		int *val2,
> +		long mask)
> +{
> +	struct ltc166x_priv *priv = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = priv->value[chan->channel];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = priv->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltc166x_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val,
> +		int val2,
> +		long mask)
> +{
> +	struct ltc166x_priv *priv = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		if (val > GENMASK(chan->scan_type.realbits-1, 0))
> +			return -EINVAL;

nothing makes val positive..  Also spaces around the '-'.


> +		priv->value[chan->channel] = val;
> +		val <<= chan->scan_type.shift;
> +		return regmap_write(priv->regmap, chan->channel, val);

Given that this could fail, usual convention is set the cached value
priv->value[] only after we know the write succeeded.  

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#define LTC166X_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.indexed = 1,					\
> +	.output = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = {					\
> +		.sign = 'u',				\

Supplying this for a DAC is unusual given scan_type only becomes relevant
(in the IIO core) when we have buffers involved.  For output devices
we don't currently have buffered support.

Hmm. You are using it as a convenient stashing location though so perhaps
fair enough.. You could only specify the realbits and shift, but that seems
a bit odd so perhaps best to leave this as it is.


> +		.realbits = (bits),			\
> +		.storagebits = 16,			\
> +		.shift = 12 - (bits),			\
> +	},						\
> +}
> +
> +#define  LTC166X_OCTAL_CHANNELS(bits) {			\
> +		LTC166X_CHAN(LTC166X_REG_DAC_A, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_B, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_C, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_D, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_E, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_F, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_G, bits),	\
> +		LTC166X_CHAN(LTC166X_REG_DAC_H, bits),	\
> +}
> +
> +static const struct iio_chan_spec ltc166x_channels[][LTC166X_NUM_CHANNELS] = {
> +	[ID_LTC1660] = LTC166X_OCTAL_CHANNELS(10),
> +	[ID_LTC1665] = LTC166X_OCTAL_CHANNELS(8),
> +};
> +
> +static const struct iio_info ltc166x_info = {
> +	.read_raw = &ltc166x_read_raw,
> +	.write_raw = &ltc166x_write_raw,
> +};
> +
> +static int __maybe_unused ltc166x_suspend(struct device *dev)
> +{
> +	struct ltc166x_priv *priv = iio_priv(spi_get_drvdata(
> +						to_spi_device(dev)));
> +	return regmap_write(priv->regmap, LTC166X_REG_SLEEP, 0x00);
> +}
> +
> +static int __maybe_unused ltc166x_resume(struct device *dev)
> +{
> +	struct ltc166x_priv *priv = iio_priv(spi_get_drvdata(
> +						to_spi_device(dev)));
> +	return regmap_write(priv->regmap, LTC166X_REG_WAKE, 0x00);
> +}
> +static SIMPLE_DEV_PM_OPS(ltc166x_pm_ops, ltc166x_suspend, ltc166x_resume);
> +
> +static int ltc166x_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc166x_priv *priv;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	priv = iio_priv(indio_dev);
> +	priv->regmap = devm_regmap_init_spi(spi, &ltc166x_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		dev_err(&spi->dev, "failed to register spi regmap %ld\n",
> +			PTR_ERR(priv->regmap));
> +		return PTR_ERR(priv->regmap);
> +	}
> +
> +	priv->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(priv->vref_reg)) {
> +		dev_err(&spi->dev, "vref regulator not specified\n");
> +		return PTR_ERR(priv->vref_reg);
> +	}
> +
> +	ret = regulator_enable(priv->vref_reg);
> +	if (ret) {
> +		dev_err(&spi->dev, "failed to enable vref regulator: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_get_voltage(priv->vref_reg);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "failed to read vref regulator: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +	priv->vref_mv = ret / 1000;

It is admittedly fairly unlikely that someone might share this regulator
across multiple devices, but in theory this might change.

So I'd slightly prefer this read being done where we actually need
the answer, so in the read_raw callback.

Lots of drivers do it in the wrong place and given how unlikely such
as setup is, I'm not that fussy about it.  However as you need to respin
anyway for the naming, we might as well make this perfect.

> +
> +	priv->spi = spi;
> +	spi_set_drvdata(spi, indio_dev);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &ltc166x_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc166x_channels[id->driver_data];
> +	indio_dev->num_channels = LTC166X_NUM_CHANNELS;
> +	indio_dev->name = id->name;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "failed to register iio device: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	regulator_disable(priv->vref_reg);
> +
> +	return ret;
> +}
> +
> +static int ltc166x_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ltc166x_priv *priv = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	regulator_disable(priv->vref_reg);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ltc166x_dt_ids[] = {
> +	{ .compatible = "lltc,ltc1660", .data = (void *)ID_LTC1660 },
> +	{ .compatible = "lltc,ltc1665", .data = (void *)ID_LTC1665 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ltc166x_dt_ids);
> +
> +static const struct spi_device_id ltc166x_id[] = {
> +	{"ltc1660", ID_LTC1660},
> +	{"ltc1665", ID_LTC1665},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, ltc166x_id);
> +
> +static struct spi_driver ltc166x_driver = {
> +	.driver = {
> +		.name = "ltc166x",
> +		.of_match_table = ltc166x_dt_ids,
> +		.pm = &ltc166x_pm_ops,
> +	},
> +	.probe	= ltc166x_probe,
> +	.remove = ltc166x_remove,
> +	.id_table = ltc166x_id,
> +};
> +module_spi_driver(ltc166x_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@...il.com>");
> +MODULE_DESCRIPTION("Linear Technology LTC166X DAC");
> +MODULE_LICENSE("GPL v2");

Powered by blists - more mailing lists