Hi! Some comments inline... On 2018-11-06 12:31, Chris Coffey wrote: > This patch adds driver support for the Microchip MCP41xxx/42xxx family > of digital potentiometers: > > DEVICE Wipers Positions Resistance (kOhm) > MCP41010 1 256 10 > MCP41050 1 256 50 > MCP41100 1 256 100 > MCP42010 2 256 10 > MCP42050 2 256 50 > MCP42100 2 256 100 > > Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf > > Signed-off-by: Chris Coffey <cmc@xxxxxxxxxxxxx> > --- > .../bindings/iio/potentiometer/mcp41010.txt | 29 +++ > drivers/iio/potentiometer/Kconfig | 12 ++ > drivers/iio/potentiometer/Makefile | 1 + > drivers/iio/potentiometer/mcp41010.c | 216 +++++++++++++++++++++ > 4 files changed, 258 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt > create mode 100644 drivers/iio/potentiometer/mcp41010.c > > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt > new file mode 100644 > index 0000000000..17565acace > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp41010.txt > @@ -0,0 +1,29 @@ > +* Microchip MCP41010/41050/41100/42010/42050/42100 Digital Potentiometer > + driver > + > +Datasheet publicly available at: > +http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf > + > +The node for this driver must be a child node of a SPI controller, hence > +all mandatory properties described in > + > + Documentation/devicetree/bindings/spi/spi-bus.txt > + > +must be specified. > + > +Required properties: > + - compatible: Must be one of the following, depending on the > + model: > + "microchip,mcp41010" > + "microchip,mcp41050" > + "microchip,mcp41100" > + "microchip,mcp42010" > + "microchip,mcp42050" > + "microchip,mcp42100" > + > +Example: > +mcp41010: potentiometer@0 { > + compatible = "mcp41010"; > + reg = <0>; > + spi-max-frequency = <500000>; > +}; > diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig > index 79ec2eba49..6303cbe799 100644 > --- a/drivers/iio/potentiometer/Kconfig > +++ b/drivers/iio/potentiometer/Kconfig > @@ -90,6 +90,18 @@ config MCP4531 > To compile this driver as a module, choose M here: the > module will be called mcp4531. > > +config MCP41010 > + tristate "Microchip MCP41xxx/MCP42xxx Digital Potentiometer driver" > + depends on SPI > + help > + Say yes here to build support for the Microchip > + MCP41010, MCP41050, MCP41100, > + MCP42010, MCP42050, MCP42100 > + digital potentiometer chips. > + > + To compile this driver as a module, choose M here: the > + module will be called mcp41010. > + > config TPL0102 > tristate "Texas Instruments digital potentiometer driver" > depends on I2C > diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile > index 4af657883c..8ff55138cf 100644 > --- a/drivers/iio/potentiometer/Makefile > +++ b/drivers/iio/potentiometer/Makefile > @@ -11,4 +11,5 @@ obj-$(CONFIG_MAX5487) += max5487.o > obj-$(CONFIG_MCP4018) += mcp4018.o > obj-$(CONFIG_MCP4131) += mcp4131.o > obj-$(CONFIG_MCP4531) += mcp4531.o > +obj-$(CONFIG_MCP41010) += mcp41010.o > obj-$(CONFIG_TPL0102) += tpl0102.o > diff --git a/drivers/iio/potentiometer/mcp41010.c b/drivers/iio/potentiometer/mcp41010.c > new file mode 100644 > index 0000000000..4068e8eb57 > --- /dev/null > +++ b/drivers/iio/potentiometer/mcp41010.c > @@ -0,0 +1,216 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Industrial I/O driver for Microchip digital potentiometers > + * > + * Copyright (c) 2018 Chris Coffey <cmc@xxxxxxxxxxxxx> > + * Based on: Slawomir Stepien's code from mcp4131.c > + * > + * Datasheet: http://ww1.microchip.com/downloads/en/devicedoc/11195c.pdf > + * > + * DEVID #Wipers #Positions Resistance (kOhm) > + * mcp41010 1 256 10 > + * mcp41050 1 256 50 > + * mcp41100 1 256 100 > + * mcp42010 2 256 10 > + * mcp42050 2 256 50 > + * mcp42100 2 256 100 > + * > + */ > + > +#include <linux/cache.h> > +#include <linux/err.h> > +#include <linux/export.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/types.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/spi/spi.h> > + > +#define MCP41010_MAX_WIPERS 2 > +#define MCP41010_WRITE (0x01 << 4) > +#define MCP41010_WIPER_MAX 255 > +#define MCP41010_WIPER_ENABLE BIT(0) > + > +struct mcp41010_cfg { > + int wipers; > + int kohms; > +}; > + > +enum mcp41010_type { > + MCP41010 = 0, > + MCP41050, > + MCP41100, > + MCP42010, > + MCP42050, > + MCP42100, > +}; > + > +static const struct mcp41010_cfg mcp41010_cfg[] = { > + [MCP41010] = { .wipers = 1, .kohms = 10, }, > + [MCP41050] = { .wipers = 1, .kohms = 50, }, > + [MCP41100] = { .wipers = 1, .kohms = 100, }, > + [MCP42010] = { .wipers = 2, .kohms = 10, }, > + [MCP42050] = { .wipers = 2, .kohms = 50, }, > + [MCP42100] = { .wipers = 2, .kohms = 100, }, > +}; > + > +struct mcp41010_data { > + struct spi_device *spi; > + const struct mcp41010_cfg *cfg; > + struct mutex lock; /* Protect write sequences */ > + unsigned int value[MCP41010_MAX_WIPERS]; /* Cache wiper values */ > + u8 buf[2] ____cacheline_aligned; > +}; > + > +#define MCP41010_CHANNEL(ch) { \ > + .type = IIO_RESISTANCE, \ > + .indexed = 1, \ > + .output = 1, \ > + .channel = (ch), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > +} > + > +static const struct iio_chan_spec mcp41010_channels[] = { > + MCP41010_CHANNEL(0), > + MCP41010_CHANNEL(1), > +}; > + > +static int mcp41010_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct mcp41010_data *data = iio_priv(indio_dev); > + int channel = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + *val = data->value[channel]; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 1000 * data->cfg->kohms; > + *val2 = MCP41010_WIPER_MAX; > + return IIO_VAL_FRACTIONAL; > + } > + > + return -EINVAL; > +} > + > +static int mcp41010_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + int err; > + struct mcp41010_data *data = iio_priv(indio_dev); > + int channel = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (val > MCP41010_WIPER_MAX || val < 0) > + return -EINVAL; > + break; > + > + default: > + return -EINVAL; > + } > + > + mutex_lock(&data->lock); > + > + data->buf[0] = MCP41010_WIPER_ENABLE << channel; > + data->buf[0] |= MCP41010_WRITE; Will this not clobber the other channel for mcp42xxx chips??? > + data->buf[1] = val & 0xff; > + > + err = spi_write(data->spi, data->buf, 2); > + if (!err) > + data->value[channel] = val; > + > + mutex_unlock(&data->lock); > + > + return err; > +} > + > +static const struct iio_info mcp41010_info = { > + .read_raw = mcp41010_read_raw, > + .write_raw = mcp41010_write_raw, > +}; > + > +static int mcp41010_probe(struct spi_device *spi) > +{ > + int err; > + struct device *dev = &spi->dev; > + unsigned long devid = spi_get_device_id(spi)->driver_data; > + struct mcp41010_data *data; > + struct iio_dev *indio_dev; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + data = iio_priv(indio_dev); > + spi_set_drvdata(spi, indio_dev); > + data->spi = spi; > + data->cfg = &mcp41010_cfg[devid]; I'm missing a "of_device_get_match_data(dev)" call somewhere in here, see e.g. the max5481.c file in this directory. Yes, that's missing from the mcp4131 driver as well, but that's not a valid reason for not doing it here AFAICT... Cheers, Peter > + > + mutex_init(&data->lock); > + > + indio_dev->dev.parent = dev; > + indio_dev->info = &mcp41010_info; > + indio_dev->channels = mcp41010_channels; > + indio_dev->num_channels = data->cfg->wipers; > + indio_dev->name = spi_get_device_id(spi)->name; > + > + err = devm_iio_device_register(dev, indio_dev); > + if (err) { > + dev_info(&spi->dev, "Unable to register %s\n", indio_dev->name); > + return err; > + } > + > + return 0; > +} > + > +#if defined(CONFIG_OF) > +static const struct of_device_id mcp41010_dt_ids[] = { > + { .compatible = "microchip,mcp41010", > + .data = &mcp41010_cfg[MCP41010] }, > + { .compatible = "microchip,mcp41050", > + .data = &mcp41010_cfg[MCP41050] }, > + { .compatible = "microchip,mcp41100", > + .data = &mcp41010_cfg[MCP41100] }, > + { .compatible = "microchip,mcp42010", > + .data = &mcp41010_cfg[MCP42010] }, > + { .compatible = "microchip,mcp42050", > + .data = &mcp41010_cfg[MCP42050] }, > + { .compatible = "microchip,mcp42100", > + .data = &mcp41010_cfg[MCP42100] }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, mcp41010_dt_ids); > +#endif /* CONFIG_OF */ > + > +static const struct spi_device_id mcp41010_id[] = { > + { "mcp41010", MCP41010 }, > + { "mcp41050", MCP41050 }, > + { "mcp41100", MCP41100 }, > + { "mcp42010", MCP42010 }, > + { "mcp42050", MCP42050 }, > + { "mcp42100", MCP42100 }, > + {} > +}; > +MODULE_DEVICE_TABLE(spi, mcp41010_id); > + > +static struct spi_driver mcp41010_driver = { > + .driver = { > + .name = "mcp41010", > + .of_match_table = of_match_ptr(mcp41010_dt_ids), > + }, > + .probe = mcp41010_probe, > + .id_table = mcp41010_id, > +}; > + > +module_spi_driver(mcp41010_driver); > + > +MODULE_AUTHOR("Chris Coffey <cmc@xxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("MCP41010 digital potentiometer"); > +MODULE_LICENSE("GPL v2"); >
