[PATCH 1/3] iio: dac: add support for ltc166x

[PATCH 1/3] iio: dac: add support for ltc166x

[Marcus Folkesson]

LTC1665/LTC1660 is a 8/10-bit Digital-to-Analog Converter
(DAC) with eight individual channels.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 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@gmail.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;
+		priv->value[chan->channel] = val;
+		val <<= chan->scan_type.shift;
+		return regmap_write(priv->regmap, chan->channel, val);
+	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',				\
+		.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;
+
+	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@gmail.com>");
+MODULE_DESCRIPTION("Linear Technology LTC166X DAC");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0.rc2

[PATCH 2/3] dt-bindings: iio: dac: add bindings for ltc166x

[Marcus Folkesson]

LTC1665/LTC1660 is a 8/10-bit Digital-to-Analog Converter (DAC)
with eight individual channels.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 .../devicetree/bindings/iio/dac/ltc166x.txt         | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/ltc166x.txt

diff --git a/Documentation/devicetree/bindings/iio/dac/ltc166x.txt b/Documentation/devicetree/bindings/iio/dac/ltc166x.txt
new file mode 100644
index 000000000000..c5b5f22d6c64
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/ltc166x.txt
@@ -0,0 +1,21 @@
+* Linear Technology Micropower octal 8-Bit and 10-Bit DACs
+
+Required properties:
+ - compatible: Must be one of the following:
+		"lltc,ltc1660"
+		"lltc,ltc1665"
+ - reg: SPI chip select number for the device
+ - vref-supply: Phandle to the voltage reference supply
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+	 Documentation/devicetree/bindings/spi/spi-bus.txt.
+	 Max frequency for this chip is 5 MHz.
+
+Example:
+dac@0 {
+	compatible = "lltc,ltc1660";
+	reg = <0>;
+	spi-max-frequency = <5000000>;
+	vref-supply = <&vref_reg>;
+};
-- 
2.11.0.rc2

[PATCH 3/3] MAINTAINERS: add entry for ltc166x DAC driver

[Marcus Folkesson]

Add entry for ltc166x DAC driver and add myself as
maintainer of this driver.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9276da915d9d..2dc4c773fb2e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8363,6 +8363,13 @@ L:	linux-scsi@vger.kernel.org
 S:	Maintained
 F:	drivers/scsi/sym53c8xx_2/
 
+LTC166X DAC DRIVER
+M:	Marcus Folkesson <marcus.folkesson@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/dac/ltc166x.txt
+F:	drivers/iio/dac/ltc166x.c
+
 LTC4261 HARDWARE MONITOR DRIVER
 M:	Guenter Roeck <linux@roeck-us.net>
 L:	linux-hwmon@vger.kernel.org
-- 
2.11.0.rc2

Re: [PATCH 2/3] dt-bindings: iio: dac: add bindings for ltc166x

[Rob Herring]

On Sat, 11 Aug 2018 22:02:25 +0200, Marcus Folkesson wrote:
> LTC1665/LTC1660 is a 8/10-bit Digital-to-Analog Converter (DAC)
> with eight individual channels.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  .../devicetree/bindings/iio/dac/ltc166x.txt         | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/ltc166x.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/3] iio: dac: add support for ltc166x

[Jonathan Cameron]

On Sat, 11 Aug 2018 22:02:24 +0200
Marcus Folkesson <marcus.folkesson@gmail.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@gmail.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@gmail.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@gmail.com>");
> +MODULE_DESCRIPTION("Linear Technology LTC166X DAC");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 1/3] iio: dac: add support for ltc166x

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 11920 bytes --]

Hi Jonathan,

Thanks for your comments!

On Sun, Aug 19, 2018 at 05:38:50PM +0100, Jonathan Cameron wrote:
> On Sat, 11 Aug 2018 22:02:24 +0200
> Marcus Folkesson <marcus.folkesson@gmail.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@gmail.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...

Hah, I can imagine. I will switch do use ltc1660 all over the place.
> 
> 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@gmail.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 '-'.
> 
> 

You are right.
I will go for
	val &=  GENMASK(chan->scan_type.realbits - 1, 0);
instead.

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

I will switch the order, thanks.

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

Yeah, I was actually struggling with how I should handle the different
bit-lengts, then I saw that other drivers (most of all DAC drivers for Analog
Devices) was using the "buffer variables" for stashing these values.

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

Will do, thanks!

> 
> > +
> > +	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@gmail.com>");
> > +MODULE_DESCRIPTION("Linear Technology LTC166X DAC");
> > +MODULE_LICENSE("GPL v2");
> 

Best regards
Marcus Folkesson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]