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 <[email protected]> wrote: > > > LTC1665/LTC1660 is a 8/10-bit Digital-to-Analog Converter > > (DAC) with eight individual channels. > > > > Signed-off-by: Marcus Folkesson <[email protected]> > > 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 <[email protected]> > > + */ > > +#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 = <c166x_read_raw, > > + .write_raw = <c166x_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, <c166x_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 = <c166x_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 = <c166x_pm_ops, > > + }, > > + .probe = ltc166x_probe, > > + .remove = ltc166x_remove, > > + .id_table = ltc166x_id, > > +}; > > +module_spi_driver(ltc166x_driver); > > + > > +MODULE_AUTHOR("Marcus Folkesson <[email protected]>"); > > +MODULE_DESCRIPTION("Linear Technology LTC166X DAC"); > > +MODULE_LICENSE("GPL v2"); > Best regards Marcus Folkesson
signature.asc
Description: PGP signature
