* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
@ 2018-11-06 16:37 ` Peter Rosin
0 siblings, 0 replies; 15+ messages in thread
From: Peter Rosin @ 2018-11-06 16:37 UTC (permalink / raw)
To: Chris Coffey, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler
Cc: Slawomir Stepien, Rob Herring, Mark Rutland,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org
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");
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
2018-11-06 16:37 ` Peter Rosin
@ 2018-11-06 20:08 ` Peter Rosin
-1 siblings, 0 replies; 15+ messages in thread
From: Peter Rosin @ 2018-11-06 20:08 UTC (permalink / raw)
To: Chris Coffey, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler
Cc: Slawomir Stepien, Rob Herring, Mark Rutland,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org
T24gMjAxOC0xMS0wNiAxNzozNywgUGV0ZXIgUm9zaW4gd3JvdGU6DQo+IEhpIQ0KPiANCj4gU29t
ZSBjb21tZW50cyBpbmxpbmUuLi4NCj4gDQo+PiArDQo+PiArc3RhdGljIGludCBtY3A0MTAxMF93
cml0ZV9yYXcoc3RydWN0IGlpb19kZXYgKmluZGlvX2RldiwNCj4+ICsJCQkgICAgIHN0cnVjdCBp
aW9fY2hhbl9zcGVjIGNvbnN0ICpjaGFuLA0KPj4gKwkJCSAgICAgaW50IHZhbCwgaW50IHZhbDIs
IGxvbmcgbWFzaykNCj4+ICt7DQo+PiArCWludCBlcnI7DQo+PiArCXN0cnVjdCBtY3A0MTAxMF9k
YXRhICpkYXRhID0gaWlvX3ByaXYoaW5kaW9fZGV2KTsNCj4+ICsJaW50IGNoYW5uZWwgPSBjaGFu
LT5jaGFubmVsOw0KPj4gKw0KPj4gKwlzd2l0Y2ggKG1hc2spIHsNCj4+ICsJY2FzZSBJSU9fQ0hB
Tl9JTkZPX1JBVzoNCj4+ICsJCWlmICh2YWwgPiBNQ1A0MTAxMF9XSVBFUl9NQVggfHwgdmFsIDwg
MCkNCj4+ICsJCQlyZXR1cm4gLUVJTlZBTDsNCj4+ICsJCWJyZWFrOw0KPj4gKw0KPj4gKwlkZWZh
dWx0Og0KPj4gKwkJcmV0dXJuIC1FSU5WQUw7DQo+PiArCX0NCj4+ICsNCj4+ICsJbXV0ZXhfbG9j
aygmZGF0YS0+bG9jayk7DQo+PiArDQo+PiArCWRhdGEtPmJ1ZlswXSA9IE1DUDQxMDEwX1dJUEVS
X0VOQUJMRSA8PCBjaGFubmVsOw0KPj4gKwlkYXRhLT5idWZbMF0gfD0gTUNQNDEwMTBfV1JJVEU7
DQo+IA0KPiBXaWxsIHRoaXMgbm90IGNsb2JiZXIgdGhlIG90aGVyIGNoYW5uZWwgZm9yIG1jcDQy
eHh4IGNoaXBzPz8/DQoNCg0KSSBoYWQgYSBwZWFrIGluIHRoZSBkYXRhc2hlZXQsIGFuZCBubywg
aXQgd2lsbCBub3QuIEl0IHdhcyBqdXN0IHRoZSBuYW1pbmcNCndpdGggLi4uX1dJUEVSX0VOQUJM
RSB0aGF0IHRocmV3IG1lIG9mZi4gSXQgYXNzdW1lZCwgZnJvbSB0aGUgbmFtaW5nLCB0aGF0DQpl
YWNoIGNoYW5uZWwgaGFzIGEgc2VwYXJhdGUgZW5hYmxlIGJpdCBpbiB0aGUgY29tbWFuZCBieXRl
IGFuZCB0aGF0IHlvdQ0Kd2VyZSByZXF1aXJlZCB0byBrZWVwIHRoZSBzdGF0ZSBvZiB0aG9zZSBp
bnRhY3QgZm9yIGVhY2ggYW5kIGV2ZXJ5IGNvbW1hbmQNCnlvdSBzZW50LiBBZnRlciBsb29raW5n
IGF0IHRoZSBkYXRhc2hlZXQsIHRoYXQgaXMgb2J2aW91c2x5IG5vdCB0aGUgY2FzZSwNCmFuZCB0
aGUgY29kZSBpcyBmaW5lLiBCdXQgbWF5IEkgYXNrIGZvciBhIGNoYW5nZSBpbiBuYW1pbmcgaGVy
ZT8NCg0KTUNQNDEwMTBfV0lQRVJfQ0hBTk5FTCBpbnN0ZWFkIG9mIE1DUDQxMDEwX1dJUEVSX0VO
QUJMRSBwZXJoYXBzPw0KDQpDaGVlcnMsDQpQZXRlcg0KDQo+IA0KPj4gKwlkYXRhLT5idWZbMV0g
PSB2YWwgJiAweGZmOw0KPj4gKw0KPj4gKwllcnIgPSBzcGlfd3JpdGUoZGF0YS0+c3BpLCBkYXRh
LT5idWYsIDIpOw0KPj4gKwlpZiAoIWVycikNCj4+ICsJCWRhdGEtPnZhbHVlW2NoYW5uZWxdID0g
dmFsOw0KPj4gKw0KPj4gKwltdXRleF91bmxvY2soJmRhdGEtPmxvY2spOw0KPj4gKw0KPj4gKwly
ZXR1cm4gZXJyOw0KPj4gK30NCg==
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
@ 2018-11-06 20:08 ` Peter Rosin
0 siblings, 0 replies; 15+ messages in thread
From: Peter Rosin @ 2018-11-06 20:08 UTC (permalink / raw)
To: Chris Coffey, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler
Cc: Slawomir Stepien, Rob Herring, Mark Rutland,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org
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;
>> +}
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
2018-11-06 20:08 ` Peter Rosin
(?)
@ 2018-11-07 9:25 ` Chris Coffey
-1 siblings, 0 replies; 15+ messages in thread
From: Chris Coffey @ 2018-11-07 9:25 UTC (permalink / raw)
To: Peter Rosin
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Slawomir Stepien, Rob Herring,
Mark Rutland, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
2018-11-06 16:37 ` Peter Rosin
(?)
(?)
@ 2018-11-07 9:18 ` Chris Coffey
-1 siblings, 0 replies; 15+ messages in thread
From: Chris Coffey @ 2018-11-07 9:18 UTC (permalink / raw)
To: Peter Rosin
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Slawomir Stepien, Rob Herring,
Mark Rutland, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
2018-11-06 16:37 ` Peter Rosin
` (2 preceding siblings ...)
(?)
@ 2018-11-11 14:55 ` Jonathan Cameron
2018-11-11 16:49 ` Peter Rosin
-1 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2018-11-11 14:55 UTC (permalink / raw)
To: Peter Rosin
Cc: Chris Coffey, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Slawomir Stepien, Rob Herring,
Mark Rutland, linux-iio@vger.kernel.org,
devicetree@vger.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");
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
2018-11-11 14:55 ` Jonathan Cameron
@ 2018-11-11 16:49 ` Peter Rosin
0 siblings, 0 replies; 15+ messages in thread
From: Peter Rosin @ 2018-11-11 16:49 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Chris Coffey, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Slawomir Stepien, Rob Herring,
Mark Rutland, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org
T24gMjAxOC0xMS0xMSAxNTo1NSwgSm9uYXRoYW4gQ2FtZXJvbiB3cm90ZToNCj4gT24gVHVlLCA2
IE5vdiAyMDE4IDE2OjM3OjExICswMDAwDQo+IFBldGVyIFJvc2luIDxwZWRhQGF4ZW50aWEuc2U+
IHdyb3RlOg0KPiANCj4+IEhpIQ0KPj4NCj4+IFNvbWUgY29tbWVudHMgaW5saW5lLi4uDQo+IEEg
ZmV3IGFkZGl0aW9ucyBmcm9tIG1lLg0KDQoqc25pcCoNCg0KPj4+ICsJZGF0YSA9IGlpb19wcml2
KGluZGlvX2Rldik7DQo+Pj4gKwlzcGlfc2V0X2RydmRhdGEoc3BpLCBpbmRpb19kZXYpOw0KPj4+
ICsJZGF0YS0+c3BpID0gc3BpOw0KPj4+ICsJZGF0YS0+Y2ZnID0gJm1jcDQxMDEwX2NmZ1tkZXZp
ZF07ICANCj4+DQo+PiBJJ20gbWlzc2luZyBhICJvZl9kZXZpY2VfZ2V0X21hdGNoX2RhdGEoZGV2
KSIgY2FsbCBzb21ld2hlcmUgaW4gaGVyZSwgc2VlIGUuZy4gdGhlDQo+PiBtYXg1NDgxLmMgZmls
ZSBpbiB0aGlzIGRpcmVjdG9yeS4gWWVzLCB0aGF0J3MgbWlzc2luZyBmcm9tIHRoZSBtY3A0MTMx
IGRyaXZlciBhcw0KPj4gd2VsbCwgYnV0IHRoYXQncyBub3QgYSB2YWxpZCByZWFzb24gZm9yIG5v
dCBkb2luZyBpdCBoZXJlIEFGQUlDVC4uLg0KPiBJZiB5b3Ugd2FudCB0byB1c2UgdGhlIGRhdGEg
ZWxlbWVudHMgZnJvbSB0aGUgZGV2aWNldHJlZSBiaW5kaW5ncyB5b3UnbGwgbmVlZA0KPiB0byBk
byB0aGF0LiAgSG93ZXZlciwgdGhlcmUgaXMgYW4gb2RkIHBhdGggdGhhdCBhY3R1YWxseSBtZWFu
cyBpdCB3aWxsIGZhbGwgYmFjaw0KPiB0byBnZXR0aW5nIHRoZSByaWdodCB0aGluZyBhcyB5b3Ug
aGF2ZSBpdCBoZXJlLg0KPiBzcGlfZ2V0X2RldmljZV9pZCBjYWxscyBzcGlfbWF0Y2hfaWQgd2hp
Y2ggY29tcGFyZXMgdGhlIHNkZXYtPm1vZGFsaWFzDQo+IHdpdGggdGhlIGlkIHRhYmxlIG5hbWVz
LiAgbW9kYWxpYXMgaGFzIGJlZW4gY2FyZWZ1bGx5IGNvbnN0cnVjdGVkDQo+IHRvIGJlIHRoZSB0
ZXh0IGFmdGVyIHRoZSBjb21tYSBvbmx5IGFuZCBhcyBzdWNoIHRoaXMgd29ya3MgYXMgaXMuDQo+
IA0KPiBQZXJoYXBzIGl0J3MgbmVhdGVyIHRob3VnaCB0byBqdXN0IHVzZSB0aGUgZGV2aWNldHJl
ZSBhY2Nlc3MgZnVuY3Rpb25zLg0KDQpJc24ndCB0aGF0IHBhcnQgYWJvdXQgbm90IGxvb2tpbmcg
YXQgdGhlIHZlbmRvci1wYXJ0IG9mIHRoZSBEVC1jb21wYXRpYmxlIHNsaWdodGx5DQpmcmFnaWxl
PyBPciB3aWxsIG1vZGFsaWFzL2NoaXAtbnVtYmVyIGNsYXNoZXMgYmUgZGV0ZWN0ZWQgYnkgc29t
ZXRoaW5nPw0KDQpDaGVlcnMsDQpQZXRlcg0K
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
@ 2018-11-11 16:49 ` Peter Rosin
0 siblings, 0 replies; 15+ messages in thread
From: Peter Rosin @ 2018-11-11 16:49 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Chris Coffey, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Slawomir Stepien, Rob Herring,
Mark Rutland, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] iio: potentiometer: Add driver for Microchip MCP41xxx/42xxx
2018-11-11 16:49 ` Peter Rosin
(?)
@ 2018-11-11 17:07 ` Jonathan Cameron
-1 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2018-11-11 17:07 UTC (permalink / raw)
To: Peter Rosin
Cc: Chris Coffey, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Slawomir Stepien, Rob Herring,
Mark Rutland, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org
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
^ permalink raw reply [flat|nested] 15+ messages in thread