Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx

[Jonathan Cameron <jic23@kernel.org>]

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");
> >   
>