iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X

The following functionalities are supported:
- write, read from volatile memory

Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf

Signed-off-by Slawomir Stepien <[email protected]>

drivers/iio/potentiometer/mcp4131.c new file +53 -0

+
+struct mcp4131_data {
+       struct spi_device *spi;
+       const struct mcp4131_cfg *cfg;
+       struct mutex lock;
+       struct spi_transfer xfer;
+       struct spi_message msg;
+       u8 buf[2] ____cacheline_aligned;
+};
+
+#define MCP4131_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 mcp4131_channels[] = {
+       MCP4131_CHANNEL(0),
+       MCP4131_CHANNEL(1),
+};
+
+static int mcp4131_exec(struct mcp4131_data *data,
+               u8 addr, u8 cmd,
+               u16 val)
+{
+       int err;
+       struct spi_device *spi = data->spi;
+
+       data->xfer.tx_buf = data->buf;
+       data->xfer.rx_buf = data->buf;
+
+       switch (cmd) {
+       case MCP4131_READ:
+               data->xfer.len = 2; /* Two bytes transfer for this command */
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
+               data->buf[1] = 0;
+               break;
+
+       case MCP4131_WRITE:
+               data->xfer.len = 2;
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
+                       MCP4131_WRITE | (val >> 8);
+               data->buf[1] = val & 0xFF; /* 8 bits here */
+               break;
+
+       default:
+               return -EINVAL;
+       }
+
+       dev_dbg(&spi->dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",

drivers/iio/potentiometer/mcp4131.c new file +29 -0

+
+       dev_dbg(&spi->dev, "mcp4131_exec: rx0: 0x%x rx1: 0x%x\n",
+                       data->buf[0], data->buf[1]);
+
+       return 0;
+}
+
+static int mcp4131_read_raw(struct iio_dev *indio_dev,
+                           struct iio_chan_spec const *chan,
+                           int *val, int *val2, long mask)
+{
+       int err;
+       struct mcp4131_data *data = iio_priv(indio_dev);
+       int address = chan->channel;
+
+       mutex_lock(&data->lock);
+
+       switch (mask) {
+       case IIO_CHAN_INFO_RAW:
+               err = mcp4131_exec(data, address, MCP4131_READ, 0);
+               if (err) {
+                       mutex_unlock(&data->lock);
+                       return err;
+               }
+
+               /* Error, bad address/command combination */
+               if (!MCP4131_CMDERR(data->buf)) {
+                       mutex_unlock(&data->lock);
+                       return -EIO;

drivers/iio/potentiometer/mcp4131.c new file +22 -0

+               return IIO_VAL_FRACTIONAL;
+       }
+
+       mutex_unlock(&data->lock);
+
+       return -EINVAL;
+}
+
+static int mcp4131_write_raw(struct iio_dev *indio_dev,
+                            struct iio_chan_spec const *chan,
+                            int val, int val2, long mask)
+{
+       int err;
+       struct mcp4131_data *data = iio_priv(indio_dev);
+       int address = chan->channel << MCP4131_WIPER_SHIFT;
+
+       mutex_lock(&data->lock);
+
+       switch (mask) {
+       case IIO_CHAN_INFO_RAW:
+               if (val > data->cfg->max_pos || val < 0) {
+                       mutex_unlock(&data->lock);

Hi Slawomir,

Quoted 2 lines · Slawomir Stepien

The following functionalities are supported:
- write, read from volatile memory

I think it would be useful if you could put 'potentiometer' either in the subject and/or commit text so it is more obvious what this driver is for.

drivers/iio/potentiometer/mcp4131.c line 187

Quoted 8 lines

drivers/iio/potentiometer/mcp4131.c new file +8 -0

+       spi_message_init(&data->msg);
+       spi_message_add_tail(&data->xfer, &data->msg);
+
+       err = spi_sync(spi, &data->msg);
+       if (err) {
+               dev_err(&spi->dev, "spi_sync(): %d\n", err);
+               return err;
+       }

Isn't this init, add, sync sequence basically open coding of what spi_write/spi_read does?
If you could use those you could also get rid transfer/message structs in priv data.
Also it these any reason why the data buffer can just be a local variable in mcp4131_read_raw/mcp4131_write_raw?
If it could be I think it should be possible to move the lock into the mcp4131_exec function.

drivers/iio/potentiometer/mcp4131.c line 226

Quoted 4 lines

drivers/iio/potentiometer/mcp4131.c new file +4 -0

+       case IIO_CHAN_INFO_SCALE:
+               *val = 1000 * data->cfg->kohms;
+               *val2 = data->cfg->max_pos;
+               mutex_unlock(&data->lock);

Is locking really necessary for IIO_CHAN_INFO_SCALE? Isn't all data->cfg stuff constant?

Quoted 14 lines

drivers/iio/potentiometer/mcp4131.c new file +14 -0

+static int mcp4131_write_raw(struct iio_dev *indio_dev,
+                            struct iio_chan_spec const *chan,
+                            int val, int val2, long mask)
+{
+       int err;
+       struct mcp4131_data *data = iio_priv(indio_dev);
+       int address = chan->channel << MCP4131_WIPER_SHIFT;
+
+       mutex_lock(&data->lock);
+
+       switch (mask) {
+       case IIO_CHAN_INFO_RAW:
+               if (val > data->cfg->max_pos || val < 0) {
+                       mutex_unlock(&data->lock);

drivers/iio/potentiometer/mcp4131.c line 258

Quoted 10 lines

drivers/iio/potentiometer/mcp4131.c new file +10 -0

+                       return -EINVAL;
+               }
+               break;
+       default:
+               mutex_unlock(&data->lock);
+               return -EINVAL;
+       }
+
+       err = mcp4131_exec(data, address, MCP4131_WRITE, val);
+       mutex_unlock(&data->lock);

While this is not a huge function it is usually good practice to keep the locking scope as small as possible.

So wouldn't this be sufficient here.

    mutex_lock(&data->lock);
    err = mcp4131_exec(data, address, MCP4131_WRITE, val);
    mutex_unlock(&data->lock);

Of course if you are able move the lock into mcp4131_exec this will go away.

regards,
Joachim Eastwood

Hi Slawomir,

Quoted 2 lines · Slawomir Stepien

The following functionalities are supported:
- write, read from volatile memory

I think it would be useful if you could put 'potentiometer' either in the subject and/or commit text so it is more obvious what this driver is for.

Quoted 64 lines

Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf

Signed-off-by Slawomir Stepien <[email protected]>

drivers/iio/potentiometer/mcp4131.c new file +60 -0

+
+struct mcp4131_data {
+       struct spi_device *spi;
+       const struct mcp4131_cfg *cfg;
+       struct mutex lock;
+       struct spi_transfer xfer;
+       struct spi_message msg;
+       u8 buf[2] ____cacheline_aligned;
+};
+
+#define MCP4131_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 mcp4131_channels[] = {
+       MCP4131_CHANNEL(0),
+       MCP4131_CHANNEL(1),
+};
+
+static int mcp4131_exec(struct mcp4131_data *data,
+               u8 addr, u8 cmd,
+               u16 val)
+{
+       int err;
+       struct spi_device *spi = data->spi;
+
+       data->xfer.tx_buf = data->buf;
+       data->xfer.rx_buf = data->buf;
+
+       switch (cmd) {
+       case MCP4131_READ:
+               data->xfer.len = 2; /* Two bytes transfer for this command */
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
+               data->buf[1] = 0;
+               break;
+
+       case MCP4131_WRITE:
+               data->xfer.len = 2;
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
+                       MCP4131_WRITE | (val >> 8);
+               data->buf[1] = val & 0xFF; /* 8 bits here */
+               break;
+
+       default:
+               return -EINVAL;
+       }
+
+       dev_dbg(&spi->dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
+                       data->buf[0], data->buf[1]);
+
+       spi_message_init(&data->msg);
+       spi_message_add_tail(&data->xfer, &data->msg);
+
+       err = spi_sync(spi, &data->msg);
+       if (err) {

drivers/iio/potentiometer/mcp4131.c line 187

Quoted 3 lines

drivers/iio/potentiometer/mcp4131.c new file +3 -0

+               dev_err(&spi->dev, "spi_sync(): %d\n", err);
+               return err;
+       }

Isn't this init, add, sync sequence basically open coding of what spi_write/spi_read does?
If you could use those you could also get rid transfer/message structs in priv data.

Also it these any reason why the data buffer can just be a local variable in mcp4131_read_raw/mcp4131_write_raw?

If it could be I think it should be possible to move the lock into the mcp4131_exec function.

Quoted 29 lines

drivers/iio/potentiometer/mcp4131.c new file +29 -0

+
+       dev_dbg(&spi->dev, "mcp4131_exec: rx0: 0x%x rx1: 0x%x\n",
+                       data->buf[0], data->buf[1]);
+
+       return 0;
+}
+
+static int mcp4131_read_raw(struct iio_dev *indio_dev,
+                           struct iio_chan_spec const *chan,
+                           int *val, int *val2, long mask)
+{
+       int err;
+       struct mcp4131_data *data = iio_priv(indio_dev);
+       int address = chan->channel;
+
+       mutex_lock(&data->lock);
+
+       switch (mask) {
+       case IIO_CHAN_INFO_RAW:
+               err = mcp4131_exec(data, address, MCP4131_READ, 0);
+               if (err) {
+                       mutex_unlock(&data->lock);
+                       return err;
+               }
+
+               /* Error, bad address/command combination */
+               if (!MCP4131_CMDERR(data->buf)) {
+                       mutex_unlock(&data->lock);
+                       return -EIO;

drivers/iio/potentiometer/mcp4131.c line 226

Quoted 10 lines

drivers/iio/potentiometer/mcp4131.c new file +10 -0

+               }
+
+               *val = MCP4131_RAW(data->buf);
+               mutex_unlock(&data->lock);
+               return IIO_VAL_INT;
+
+       case IIO_CHAN_INFO_SCALE:
+               *val = 1000 * data->cfg->kohms;
+               *val2 = data->cfg->max_pos;
+               mutex_unlock(&data->lock);

Is locking really necessary for IIO_CHAN_INFO_SCALE? Isn't all data->cfg stuff constant?

Quoted 22 lines

drivers/iio/potentiometer/mcp4131.c new file +22 -0

+               return IIO_VAL_FRACTIONAL;
+       }
+
+       mutex_unlock(&data->lock);
+
+       return -EINVAL;
+}
+
+static int mcp4131_write_raw(struct iio_dev *indio_dev,
+                            struct iio_chan_spec const *chan,
+                            int val, int val2, long mask)
+{
+       int err;
+       struct mcp4131_data *data = iio_priv(indio_dev);
+       int address = chan->channel << MCP4131_WIPER_SHIFT;
+
+       mutex_lock(&data->lock);
+
+       switch (mask) {
+       case IIO_CHAN_INFO_RAW:
+               if (val > data->cfg->max_pos || val < 0) {
+                       mutex_unlock(&data->lock);

drivers/iio/potentiometer/mcp4131.c line 258

Quoted 10 lines

drivers/iio/potentiometer/mcp4131.c new file +10 -0

+                       return -EINVAL;
+               }
+               break;
+       default:
+               mutex_unlock(&data->lock);
+               return -EINVAL;
+       }
+
+       err = mcp4131_exec(data, address, MCP4131_WRITE, val);
+       mutex_unlock(&data->lock);

While this is not a huge function it is usually good practice to keep the locking scope as small as possible.

So wouldn't this be sufficient here.

    mutex_lock(&data->lock);
    err = mcp4131_exec(data, address, MCP4131_WRITE, val);
    mutex_unlock(&data->lock);

Of course if you are able move the lock into mcp4131_exec this will go away.

regards,
Joachim Eastwood

Quoted 36 lines · Joachim Eastwood

Quoted 36 lines

drivers/iio/potentiometer/mcp4131.c new file +36 -0

+static int mcp4131_exec(struct mcp4131_data *data,
+               u8 addr, u8 cmd,
+               u16 val)
+{
+       int err;
+       struct spi_device *spi = data->spi;
+
+       data->xfer.tx_buf = data->buf;
+       data->xfer.rx_buf = data->buf;
+
+       switch (cmd) {
+       case MCP4131_READ:
+               data->xfer.len = 2; /* Two bytes transfer for this command */
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
+               data->buf[1] = 0;
+               break;
+
+       case MCP4131_WRITE:
+               data->xfer.len = 2;
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
+                       MCP4131_WRITE | (val >> 8);
+               data->buf[1] = val & 0xFF; /* 8 bits here */
+               break;
+
+       default:
+               return -EINVAL;
+       }
+
+       dev_dbg(&spi->dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
+                       data->buf[0], data->buf[1]);
+
+       spi_message_init(&data->msg);
+       spi_message_add_tail(&data->xfer, &data->msg);
+
+       err = spi_sync(spi, &data->msg);
+       if (err) {

Quoted 8 lines

drivers/iio/potentiometer/mcp4131.c line 187

Quoted 3 lines

drivers/iio/potentiometer/mcp4131.c new file +3 -0

+               dev_err(&spi->dev, "spi_sync(): %d\n", err);
+               return err;
+       }

Isn't this init, add, sync sequence basically open coding of what spi_write/spi_read does?
If you could use those you could also get rid transfer/message structs in priv data.

I initially wrote the same comment, then realised it's more nuanced than that. Whilst this initially looks like an w8r8 type cycle it's actually something like w4r12 in the read case anyway. The write case could indeed be done with spi_write.

Hi Jonathan,

Quoted 14 lines · Jonathan Cameron

Quoted 5 lines · Joachim Eastwood

Quoted 5 lines

drivers/iio/potentiometer/mcp4131.c new file +5 -0

+static int mcp4131_exec(struct mcp4131_data *data,
+               u8 addr, u8 cmd,
+               u16 val)
+{
+       int err;

Quoted 8 lines

drivers/iio/potentiometer/mcp4131.c

Quoted 8 lines

drivers/iio/potentiometer/mcp4131.c new file +8 -0

+       struct spi_device *spi = data->spi;
+
+       data->xfer.tx_buf = data->buf;
+       data->xfer.rx_buf = data->buf;
+
+       switch (cmd) {
+       case MCP4131_READ:
+               data->xfer.len = 2; /* Two bytes transfer for this

command */

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |

MCP4131_READ;

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

drivers/iio/potentiometer/mcp4131.c new file +21 -0

+               data->buf[1] = 0;
+               break;
+
+       case MCP4131_WRITE:
+               data->xfer.len = 2;
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
+                       MCP4131_WRITE | (val >> 8);
+               data->buf[1] = val & 0xFF; /* 8 bits here */
+               break;
+
+       default:
+               return -EINVAL;
+       }
+
+       dev_dbg(&spi->dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
+                       data->buf[0], data->buf[1]);
+
+       spi_message_init(&data->msg);
+       spi_message_add_tail(&data->xfer, &data->msg);
+
+       err = spi_sync(spi, &data->msg);

Quoted 8 lines

Quoted 8 lines

drivers/iio/potentiometer/mcp4131.c line 187

Quoted 4 lines

drivers/iio/potentiometer/mcp4131.c new file +4 -0

+       if (err) {
+               dev_err(&spi->dev, "spi_sync(): %d\n", err);
+               return err;
+       }

Isn't this init, add, sync sequence basically open coding of what spi_write/spi_read does?
If you could use those you could also get rid transfer/message

structs

Quoted 2 lines

Quoted 1 line

in priv data.

I initially wrote the same comment, then realised it's more nuanced

than

Quoted 1 line

that. Whilst this initially looks like an w8r8 type cycle it's

actually

Quoted 1 line

something like w4r12 in the read case anyway. The write case could

indeed

Quoted 1 line

be done with spi_write.

Indeed. I didn't notice that for the read case.

The read case could almost be copy of spi_read, though. One would only need to add ".tx_buf = buf" when setting up the transfer struct, I think. Having it in its a own function with a comment would make it easier to spot the difference.

regards,
Joachim Eastwood

Hi Jonathan,

Quoted 45 lines · Jonathan Cameron

Quoted 36 lines · Joachim Eastwood

Quoted 36 lines

drivers/iio/potentiometer/mcp4131.c new file +36 -0

+static int mcp4131_exec(struct mcp4131_data *data,
+               u8 addr, u8 cmd,
+               u16 val)
+{
+       int err;
+       struct spi_device *spi = data->spi;
+
+       data->xfer.tx_buf = data->buf;
+       data->xfer.rx_buf = data->buf;
+
+       switch (cmd) {
+       case MCP4131_READ:
+               data->xfer.len = 2; /* Two bytes transfer for this command */
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
+               data->buf[1] = 0;
+               break;
+
+       case MCP4131_WRITE:
+               data->xfer.len = 2;
+               data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
+                       MCP4131_WRITE | (val >> 8);
+               data->buf[1] = val & 0xFF; /* 8 bits here */
+               break;
+
+       default:
+               return -EINVAL;
+       }
+
+       dev_dbg(&spi->dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
+                       data->buf[0], data->buf[1]);
+
+       spi_message_init(&data->msg);
+       spi_message_add_tail(&data->xfer, &data->msg);
+
+       err = spi_sync(spi, &data->msg);
+       if (err) {

Quoted 8 lines

drivers/iio/potentiometer/mcp4131.c line 187

Quoted 3 lines

drivers/iio/potentiometer/mcp4131.c new file +3 -0

+               dev_err(&spi->dev, "spi_sync(): %d\n", err);
+               return err;
+       }

Isn't this init, add, sync sequence basically open coding of what spi_write/spi_read does?
If you could use those you could also get rid transfer/message structs in priv data.

Quoted 4 lines

I initially wrote the same comment, then realised it's more nuanced than that. Whilst this initially looks like an w8r8 type cycle it's actually something like w4r12 in the read case anyway. The write case could indeed be done with spi_write.

Indeed. I didn't notice that for the read case.

The read case could almost be copy of spi_read, though. One would only need to add ".tx_buf = buf" when setting up the transfer struct, I think. Having it in its a own function with a comment would make it easier to spot the difference.