| Message ID | 20181106113130.9612-1-cmc@babblebit.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx | expand |
On Tue, Nov 06, 2018 at 11:31:30AM +0000, 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@babblebit.net> > --- > .../bindings/iio/potentiometer/mcp41010.txt | 29 +++ WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.txt Please run checkpatch.pl on this patch once. > +#define MCP41010_MAX_WIPERS 2 > +#define MCP41010_WRITE (0x01 << 4) #define MCP41010_WRITE BIT(4) ? > +#define MCP41010_WIPER_MAX 255 > +#define MCP41010_WIPER_ENABLE BIT(0) > + > +struct mcp41010_cfg { > + int wipers; > + int kohms; > +}; > + > +enum mcp41010_type { > + MCP41010 = 0, 0 initialisation is implicit, hence `= 0` not required. > + MCP41050, > + MCP41100, > + MCP42010, > + MCP42050, > + MCP42100, > +}; [] > +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; This looks weird. > + default: > + return -EINVAL; > + } > + > + mutex_lock(&data->lock); > + > + data->buf[0] = MCP41010_WIPER_ENABLE << channel; > + data->buf[0] |= MCP41010_WRITE; > + 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 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]; > + > + 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; > + } I think direct return is preferred. Nevermind ...
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@babblebit.net> > --- > .../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@babblebit.net> > + * 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@babblebit.net>"); > +MODULE_DESCRIPTION("MCP41010 digital potentiometer"); > +MODULE_LICENSE("GPL v2"); >
On 2018-11-06 17:37, Peter Rosin wrote: > Hi! > > Some comments inline... > >> + >> +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??? I had a peak in the datasheet, and no, it will not. It was just the naming with ..._WIPER_ENABLE that threw me off. It assumed, from the naming, that each channel has a separate enable bit in the command byte and that you were required to keep the state of those intact for each and every command you sent. After looking at the datasheet, that is obviously not the case, and the code is fine. But may I ask for a change in naming here? MCP41010_WIPER_CHANNEL instead of MCP41010_WIPER_ENABLE perhaps? Cheers, Peter > >> + 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; >> +}
Thank you for the review. I have a question inline. On Tue, Nov 06, 2018 at 09:18:06PM +0530, Himanshu Jha wrote: > On Tue, Nov 06, 2018 at 11:31:30AM +0000, 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@babblebit.net> > > --- > > .../bindings/iio/potentiometer/mcp41010.txt | 29 +++ > > WARNING: DT binding docs and includes should be a separate patch. > See: Documentation/devicetree/bindings/submitting-patches.txt > > Please run checkpatch.pl on this patch once. > Oops. I ran checkpatch.pl on the individual files, but neglected to run it on the patch file itself. I'll split the patch in v2. checkpatch.pl also reports a warning about the MAINTAINERS file: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? ... but looking through the tree, many IIO drivers don't have corresponding entries in MAINTAINERS; is this one of those checkpatch warnings that can be safely ignored? Or should all new drivers have a corresponding entry in MAINTAINERS? Thanks again, Chris
Thank you for the review! A few comments inline. On Tue, Nov 06, 2018 at 04:37:11PM +0000, Peter Rosin wrote: > 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: > > [snip] > > + > > + data->buf[0] = MCP41010_WIPER_ENABLE << channel; > > + data->buf[0] |= MCP41010_WRITE; > > Will this not clobber the other channel for mcp42xxx chips??? > I see you followed up on this in another message, so I'll respond there. [snip] > > +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 > Ah, interesting; I was unaware of of_device_get_match_data(). I'll add that for v2. Thanks again, Chris
On Tue, Nov 06, 2018 at 08:08:15PM +0000, Peter Rosin wrote: > On 2018-11-06 17:37, Peter Rosin wrote: > > Hi! > > > > Some comments inline... > > [snip] > >> + > >> + data->buf[0] = MCP41010_WIPER_ENABLE << channel; > >> + data->buf[0] |= MCP41010_WRITE; > > > > Will this not clobber the other channel for mcp42xxx chips??? > > > I had a peak in the datasheet, and no, it will not. It was just the naming > with ..._WIPER_ENABLE that threw me off. It assumed, from the naming, that > each channel has a separate enable bit in the command byte and that you > were required to keep the state of those intact for each and every command > you sent. After looking at the datasheet, that is obviously not the case, > and the code is fine. But may I ask for a change in naming here? > > MCP41010_WIPER_CHANNEL instead of MCP41010_WIPER_ENABLE perhaps? > > Cheers, > Peter > I agree -- my naming for this is indeed ambiguous/confusing. I'll change it for v2. Thanks again, Chris
On Wed, Nov 07, 2018 at 09:05:50AM +0000, Chris Coffey wrote: > Thank you for the review. I have a question inline. > > On Tue, Nov 06, 2018 at 09:18:06PM +0530, Himanshu Jha wrote: > > On Tue, Nov 06, 2018 at 11:31:30AM +0000, 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@babblebit.net> > > > --- > > > .../bindings/iio/potentiometer/mcp41010.txt | 29 +++ > > > > WARNING: DT binding docs and includes should be a separate patch. > > See: Documentation/devicetree/bindings/submitting-patches.txt > > > > Please run checkpatch.pl on this patch once. > > > > Oops. I ran checkpatch.pl on the individual files, but neglected to run > it on the patch file itself. I'll split the patch in v2. No problem. It is better for DT maintainers, and they get less spam. It was recent effort by Rob IIRC, "133712a2ec84 checkpatch: DT bindings should be a separate patch" When you run on source file use `-f` flag and on patch just naked run is fine. > checkpatch.pl also reports a warning about the MAINTAINERS file: > WARNING: added, moved or deleted file(s), does MAINTAINERS need > updating? > > ... but looking through the tree, many IIO drivers don't have > corresponding entries in MAINTAINERS; is this one of those checkpatch > warnings that can be safely ignored? Or should all new drivers have a > corresponding entry in MAINTAINERS? It's upto you actually. Not a necessity to be a maintainer. But when you add your name to MAINTAINERS you would need to review the patches sent on the driver and maybe test it too. And if you're not sure, then leave it as-is! Without the entry in MAINTAINERS too, you can review/test patches sent on your driver(which is what I do ..) Obligations! ;)
On Wed, 7 Nov 2018 15:18:08 +0530 Himanshu Jha <himanshujha199640@gmail.com> wrote: > On Wed, Nov 07, 2018 at 09:05:50AM +0000, Chris Coffey wrote: > > Thank you for the review. I have a question inline. > > > > On Tue, Nov 06, 2018 at 09:18:06PM +0530, Himanshu Jha wrote: > > > On Tue, Nov 06, 2018 at 11:31:30AM +0000, 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@babblebit.net> > > > > --- > > > > .../bindings/iio/potentiometer/mcp41010.txt | 29 +++ > > > > > > WARNING: DT binding docs and includes should be a separate patch. > > > See: Documentation/devicetree/bindings/submitting-patches.txt > > > > > > Please run checkpatch.pl on this patch once. > > > > > > > Oops. I ran checkpatch.pl on the individual files, but neglected to run > > it on the patch file itself. I'll split the patch in v2. > > No problem. > > It is better for DT maintainers, and they get less spam. It was recent > effort by Rob IIRC, > > "133712a2ec84 checkpatch: DT bindings should be a separate patch" > > When you run on source file use `-f` flag and on patch just naked > run is fine. > > > checkpatch.pl also reports a warning about the MAINTAINERS file: > > WARNING: added, moved or deleted file(s), does MAINTAINERS need > > updating? > > > > ... but looking through the tree, many IIO drivers don't have > > corresponding entries in MAINTAINERS; is this one of those checkpatch > > warnings that can be safely ignored? Or should all new drivers have a > > corresponding entry in MAINTAINERS? > > It's upto you actually. > Not a necessity to be a maintainer. > > But when you add your name to MAINTAINERS you would need to review the > patches sent on the driver and maybe test it too. > > And if you're not sure, then leave it as-is! > > Without the entry in MAINTAINERS too, you can review/test patches > sent on your driver(which is what I do ..) > > Obligations! ;) Yes, mostly MAINTAINERS entries are needed when it's not obvious from the driver who should be looking at patches. Often this is when someone else had taken it over later, or you want all patches to be cc'd to a list of relevant people (so company mailing lists for example). If people want to put themselves in MAINTAINERS then we never mind, but personally I don't see why every single driver needs to be listed. Jonathan >
On Tue, 6 Nov 2018 16:37:11 +0000 Peter Rosin <peda@axentia.se> wrote: > Hi! > > Some comments inline... A few additions from me. thanks, Jonathan > > 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@babblebit.net> > > --- > > .../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@babblebit.net> > > + * 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 > > + * nitpick! No point in having this blank line. > > + */ > > + > > +#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... If you want to use the data elements from the devicetree bindings you'll need to do that. However, there is an odd path that actually means it will fall back to getting the right thing as you have it here. spi_get_device_id calls spi_match_id which compares the sdev->modalias with the id table names. modalias has been carefully constructed to be the text after the comma only and as such this works as is. Perhaps it's neater though to just use the devicetree access functions. > > 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 */ Drop these CONFIG_OF build protections. It is possible to instantiate a device via it's devicetree bindings form ACPI, without having CONFIG_OF true. These sort of protections prevent that approach. > > + > > +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), Don't use that of_match_ptr wrapper as that also breaks the ACPI probing method. It's a rather obscure thing that only came to my attention fairly recently, so always a bit of a surprise. Not worth going through and removing protections from existing drivers, but let's not break any more! > > + }, > > + .probe = mcp41010_probe, > > + .id_table = mcp41010_id, > > +}; > > + > > +module_spi_driver(mcp41010_driver); > > + > > +MODULE_AUTHOR("Chris Coffey <cmc@babblebit.net>"); > > +MODULE_DESCRIPTION("MCP41010 digital potentiometer"); > > +MODULE_LICENSE("GPL v2"); > > >
On 2018-11-11 15:55, Jonathan Cameron wrote: > On Tue, 6 Nov 2018 16:37:11 +0000 > Peter Rosin <peda@axentia.se> wrote: > >> Hi! >> >> Some comments inline... > A few additions from me. *snip* >>> + 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... > If you want to use the data elements from the devicetree bindings you'll need > to do that. However, there is an odd path that actually means it will fall back > to getting the right thing as you have it here. > spi_get_device_id calls spi_match_id which compares the sdev->modalias > with the id table names. modalias has been carefully constructed > to be the text after the comma only and as such this works as is. > > Perhaps it's neater though to just use the devicetree access functions. Isn't that part about not looking at the vendor-part of the DT-compatible slightly fragile? Or will modalias/chip-number clashes be detected by something? Cheers, Peter
On Sun, 11 Nov 2018 16:49:28 +0000 Peter Rosin <peda@axentia.se> wrote: > On 2018-11-11 15:55, Jonathan Cameron wrote: > > On Tue, 6 Nov 2018 16:37:11 +0000 > > Peter Rosin <peda@axentia.se> wrote: > > > >> Hi! > >> > >> Some comments inline... > > A few additions from me. > > *snip* > > >>> + 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... > > If you want to use the data elements from the devicetree bindings you'll need > > to do that. However, there is an odd path that actually means it will fall back > > to getting the right thing as you have it here. > > spi_get_device_id calls spi_match_id which compares the sdev->modalias > > with the id table names. modalias has been carefully constructed > > to be the text after the comma only and as such this works as is. > > > > Perhaps it's neater though to just use the devicetree access functions. > > Isn't that part about not looking at the vendor-part of the DT-compatible slightly > fragile? Or will modalias/chip-number clashes be detected by something? Yes :) Well actually in this case not so bad. It will use a match on the of_device_id to find which driver to probe if there is one. The modalias part is only being abused to then figure out which device we have within the driver. I definitely prefer the explicit route. Just wanted to point out it 'worked' :) > > Cheers, > Peter
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@babblebit.net> + * 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; + 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]; + + 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@babblebit.net>"); +MODULE_DESCRIPTION("MCP41010 digital potentiometer"); +MODULE_LICENSE("GPL v2");
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@babblebit.net> --- .../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