* [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
@ 2016-03-20 14:30 Slawomir Stepien
2016-03-20 16:12 ` Joachim Eastwood
0 siblings, 1 reply; 4+ messages in thread
From: Slawomir Stepien @ 2016-03-20 14:30 UTC (permalink / raw)
To: jic23, knaack.h, lars, pmeerw; +Cc: linux-iio, linux-kernel
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 <sst@poczta.fm>
---
Changes since v2:
- Removed unnecessary parentheses in MCP4131_WIPER_SHIFT macro
- Replaced the rx and tx SPI buffers with one buf that is ____cacheline_aligned
- Changed mutex locking position
- Replaced the devid with pointer to model configuration
- Removed positive ("Registered" & "Unregistered") dev_info prints
- Removed the mcp4131_remove callback
Changes since v1:
- One line summary changed
- Fixed module name and OF compatible
- Based code on Peter Rosin's code from mcp4531.c
- No additional sysfs attributes
- Deleted not used devid struct memeber
- Changed mcp4131_exec function arguments:
- write value is now u16
- no need to pass return buffer array - rx array from mcp4131_data is used
- Added missing new line characters to dev_info
---
.../bindings/iio/potentiometer/mcp4131.txt | 84 ++++
drivers/iio/potentiometer/Kconfig | 18 +
drivers/iio/potentiometer/Makefile | 1 +
drivers/iio/potentiometer/mcp4131.c | 520 +++++++++++++++++++++
4 files changed, 623 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
create mode 100644 drivers/iio/potentiometer/mcp4131.c
diff --git a/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
new file mode 100644
index 0000000..3ccba16
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/mcp4131.txt
@@ -0,0 +1,84 @@
+* Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer
+ driver
+
+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,mcp4131-502"
+ "microchip,mcp4131-103"
+ "microchip,mcp4131-503"
+ "microchip,mcp4131-104"
+ "microchip,mcp4132-502"
+ "microchip,mcp4132-103"
+ "microchip,mcp4132-503"
+ "microchip,mcp4132-104"
+ "microchip,mcp4141-502"
+ "microchip,mcp4141-103"
+ "microchip,mcp4141-503"
+ "microchip,mcp4141-104"
+ "microchip,mcp4142-502"
+ "microchip,mcp4142-103"
+ "microchip,mcp4142-503"
+ "microchip,mcp4142-104"
+ "microchip,mcp4151-502"
+ "microchip,mcp4151-103"
+ "microchip,mcp4151-503"
+ "microchip,mcp4151-104"
+ "microchip,mcp4152-502"
+ "microchip,mcp4152-103"
+ "microchip,mcp4152-503"
+ "microchip,mcp4152-104"
+ "microchip,mcp4161-502"
+ "microchip,mcp4161-103"
+ "microchip,mcp4161-503"
+ "microchip,mcp4161-104"
+ "microchip,mcp4162-502"
+ "microchip,mcp4162-103"
+ "microchip,mcp4162-503"
+ "microchip,mcp4162-104"
+ "microchip,mcp4231-502"
+ "microchip,mcp4231-103"
+ "microchip,mcp4231-503"
+ "microchip,mcp4231-104"
+ "microchip,mcp4232-502"
+ "microchip,mcp4232-103"
+ "microchip,mcp4232-503"
+ "microchip,mcp4232-104"
+ "microchip,mcp4241-502"
+ "microchip,mcp4241-103"
+ "microchip,mcp4241-503"
+ "microchip,mcp4241-104"
+ "microchip,mcp4242-502"
+ "microchip,mcp4242-103"
+ "microchip,mcp4242-503"
+ "microchip,mcp4242-104"
+ "microchip,mcp4251-502"
+ "microchip,mcp4251-103"
+ "microchip,mcp4251-503"
+ "microchip,mcp4251-104"
+ "microchip,mcp4252-502"
+ "microchip,mcp4252-103"
+ "microchip,mcp4252-503"
+ "microchip,mcp4252-104"
+ "microchip,mcp4261-502"
+ "microchip,mcp4261-103"
+ "microchip,mcp4261-503"
+ "microchip,mcp4261-104"
+ "microchip,mcp4262-502"
+ "microchip,mcp4262-103"
+ "microchip,mcp4262-503"
+ "microchip,mcp4262-104"
+
+Example:
+mcp4131: mcp4131@0 {
+ compatible = "mcp4131-502";
+ reg = <0>;
+ spi-max-frequency = <500000>;
+};
diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index fd75db7..5ef181a 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -5,6 +5,24 @@
menu "Digital potentiometers"
+config MCP4131
+ tristate "Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Digital Potentiometer driver"
+ depends on SPI
+ help
+ Say yes here to build support for the Microchip
+ MCP4131, MCP4132,
+ MCP4141, MCP4142,
+ MCP4151, MCP4152,
+ MCP4161, MCP4162,
+ MCP4231, MCP4232,
+ MCP4241, MCP4242,
+ MCP4251, MCP4252,
+ MCP4261, MCP4262,
+ digital potentiomenter chips.
+
+ To compile this driver as a module, choose M here: the
+ module will be called mcp4131.
+
config MCP4531
tristate "Microchip MCP45xx/MCP46xx Digital Potentiometer driver"
depends on I2C
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 8afe492..cd5c80b 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -3,4 +3,5 @@
#
# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_MCP4131) += mcp4131.o
obj-$(CONFIG_MCP4531) += mcp4531.o
diff --git a/drivers/iio/potentiometer/mcp4131.c b/drivers/iio/potentiometer/mcp4131.c
new file mode 100644
index 0000000..bd322ff
--- /dev/null
+++ b/drivers/iio/potentiometer/mcp4131.c
@@ -0,0 +1,520 @@
+/*
+ * Industrial I/O driver for Microchip digital potentiometers
+ *
+ * Copyright (c) 2016 Slawomir Stepien
+ * Based on: Peter Rosin's code from mcp4531.c
+ *
+ * Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
+ *
+ * DEVID #Wipers #Positions Resistor Opts (kOhm)
+ * mcp4131 1 129 5, 10, 50, 100
+ * mcp4132 1 129 5, 10, 50, 100
+ * mcp4141 1 129 5, 10, 50, 100
+ * mcp4142 1 129 5, 10, 50, 100
+ * mcp4151 1 257 5, 10, 50, 100
+ * mcp4152 1 257 5, 10, 50, 100
+ * mcp4161 1 257 5, 10, 50, 100
+ * mcp4162 1 257 5, 10, 50, 100
+ * mcp4231 2 129 5, 10, 50, 100
+ * mcp4232 2 129 5, 10, 50, 100
+ * mcp4241 2 129 5, 10, 50, 100
+ * mcp4242 2 129 5, 10, 50, 100
+ * mcp4251 2 257 5, 10, 50, 100
+ * mcp4252 2 257 5, 10, 50, 100
+ * mcp4261 2 257 5, 10, 50, 100
+ * mcp4262 2 257 5, 10, 50, 100
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+/*
+ * TODO:
+ * 1. Write wiper setting to EEPROM for EEPROM capable models.
+ */
+
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/err.h>
+
+#include <linux/iio/iio.h>
+
+#define MCP4131_WRITE (0x00 << 2)
+#define MCP4131_READ (0x03 << 2)
+
+#define MCP4131_WIPER_SHIFT 4
+#define MCP4131_CMDERR(r) ((r[0]) & 0x02)
+#define MCP4131_RAW(r) ((r[0]) == 0xff ? 0x100 : (r[1]))
+
+struct mcp4131_cfg {
+ int wipers;
+ int max_pos;
+ int kohms;
+};
+
+enum mcp4131_type {
+ MCP413x_502 = 0,
+ MCP413x_103,
+ MCP413x_503,
+ MCP413x_104,
+ MCP414x_502,
+ MCP414x_103,
+ MCP414x_503,
+ MCP414x_104,
+ MCP415x_502,
+ MCP415x_103,
+ MCP415x_503,
+ MCP415x_104,
+ MCP416x_502,
+ MCP416x_103,
+ MCP416x_503,
+ MCP416x_104,
+ MCP423x_502,
+ MCP423x_103,
+ MCP423x_503,
+ MCP423x_104,
+ MCP424x_502,
+ MCP424x_103,
+ MCP424x_503,
+ MCP424x_104,
+ MCP425x_502,
+ MCP425x_103,
+ MCP425x_503,
+ MCP425x_104,
+ MCP426x_502,
+ MCP426x_103,
+ MCP426x_503,
+ MCP426x_104,
+};
+
+static const struct mcp4131_cfg mcp4131_cfg[] = {
+ [MCP413x_502] = { .wipers = 1, .max_pos = 128, .kohms = 5, },
+ [MCP413x_103] = { .wipers = 1, .max_pos = 128, .kohms = 10, },
+ [MCP413x_503] = { .wipers = 1, .max_pos = 128, .kohms = 50, },
+ [MCP413x_104] = { .wipers = 1, .max_pos = 128, .kohms = 100, },
+ [MCP414x_502] = { .wipers = 1, .max_pos = 128, .kohms = 5, },
+ [MCP414x_103] = { .wipers = 1, .max_pos = 128, .kohms = 10, },
+ [MCP414x_503] = { .wipers = 1, .max_pos = 128, .kohms = 50, },
+ [MCP414x_104] = { .wipers = 1, .max_pos = 128, .kohms = 100, },
+ [MCP415x_502] = { .wipers = 1, .max_pos = 256, .kohms = 5, },
+ [MCP415x_103] = { .wipers = 1, .max_pos = 256, .kohms = 10, },
+ [MCP415x_503] = { .wipers = 1, .max_pos = 256, .kohms = 50, },
+ [MCP415x_104] = { .wipers = 1, .max_pos = 256, .kohms = 100, },
+ [MCP416x_502] = { .wipers = 1, .max_pos = 256, .kohms = 5, },
+ [MCP416x_103] = { .wipers = 1, .max_pos = 256, .kohms = 10, },
+ [MCP416x_503] = { .wipers = 1, .max_pos = 256, .kohms = 50, },
+ [MCP416x_104] = { .wipers = 1, .max_pos = 256, .kohms = 100, },
+ [MCP423x_502] = { .wipers = 2, .max_pos = 128, .kohms = 5, },
+ [MCP423x_103] = { .wipers = 2, .max_pos = 128, .kohms = 10, },
+ [MCP423x_503] = { .wipers = 2, .max_pos = 128, .kohms = 50, },
+ [MCP423x_104] = { .wipers = 2, .max_pos = 128, .kohms = 100, },
+ [MCP424x_502] = { .wipers = 2, .max_pos = 128, .kohms = 5, },
+ [MCP424x_103] = { .wipers = 2, .max_pos = 128, .kohms = 10, },
+ [MCP424x_503] = { .wipers = 2, .max_pos = 128, .kohms = 50, },
+ [MCP424x_104] = { .wipers = 2, .max_pos = 128, .kohms = 100, },
+ [MCP425x_502] = { .wipers = 2, .max_pos = 256, .kohms = 5, },
+ [MCP425x_103] = { .wipers = 2, .max_pos = 256, .kohms = 10, },
+ [MCP425x_503] = { .wipers = 2, .max_pos = 256, .kohms = 50, },
+ [MCP425x_104] = { .wipers = 2, .max_pos = 256, .kohms = 100, },
+ [MCP426x_502] = { .wipers = 2, .max_pos = 256, .kohms = 5, },
+ [MCP426x_103] = { .wipers = 2, .max_pos = 256, .kohms = 10, },
+ [MCP426x_503] = { .wipers = 2, .max_pos = 256, .kohms = 50, },
+ [MCP426x_104] = { .wipers = 2, .max_pos = 256, .kohms = 100, },
+};
+
+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) {
+ dev_err(&spi->dev, "spi_sync(): %d\n", err);
+ return err;
+ }
+
+ 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;
+ }
+
+ *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);
+ 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);
+ return -EINVAL;
+ }
+ break;
+ default:
+ mutex_unlock(&data->lock);
+ return -EINVAL;
+ }
+
+ err = mcp4131_exec(data, address, MCP4131_WRITE, val);
+ mutex_unlock(&data->lock);
+
+ return err;
+}
+
+static const struct iio_info mcp4131_info = {
+ .read_raw = mcp4131_read_raw,
+ .write_raw = mcp4131_write_raw,
+ .driver_module = THIS_MODULE,
+};
+
+static int mcp4131_probe(struct spi_device *spi)
+{
+ int err;
+ struct device *dev = &spi->dev;
+ unsigned long devid = spi_get_device_id(spi)->driver_data;
+ struct mcp4131_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 = &mcp4131_cfg[devid];
+
+ mutex_init(&data->lock);
+
+ indio_dev->dev.parent = dev;
+ indio_dev->info = &mcp4131_info;
+ indio_dev->channels = mcp4131_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 mcp4131_dt_ids[] = {
+ { .compatible = "microchip,mcp4131-502",
+ .data = &mcp4131_cfg[MCP413x_502] },
+ { .compatible = "microchip,mcp4131-103",
+ .data = &mcp4131_cfg[MCP413x_103] },
+ { .compatible = "microchip,mcp4131-503",
+ .data = &mcp4131_cfg[MCP413x_503] },
+ { .compatible = "microchip,mcp4131-104",
+ .data = &mcp4131_cfg[MCP413x_104] },
+ { .compatible = "microchip,mcp4132-502",
+ .data = &mcp4131_cfg[MCP413x_502] },
+ { .compatible = "microchip,mcp4132-103",
+ .data = &mcp4131_cfg[MCP413x_103] },
+ { .compatible = "microchip,mcp4132-503",
+ .data = &mcp4131_cfg[MCP413x_503] },
+ { .compatible = "microchip,mcp4132-104",
+ .data = &mcp4131_cfg[MCP413x_104] },
+ { .compatible = "microchip,mcp4141-502",
+ .data = &mcp4131_cfg[MCP414x_502] },
+ { .compatible = "microchip,mcp4141-103",
+ .data = &mcp4131_cfg[MCP414x_103] },
+ { .compatible = "microchip,mcp4141-503",
+ .data = &mcp4131_cfg[MCP414x_503] },
+ { .compatible = "microchip,mcp4141-104",
+ .data = &mcp4131_cfg[MCP414x_104] },
+ { .compatible = "microchip,mcp4142-502",
+ .data = &mcp4131_cfg[MCP414x_502] },
+ { .compatible = "microchip,mcp4142-103",
+ .data = &mcp4131_cfg[MCP414x_103] },
+ { .compatible = "microchip,mcp4142-503",
+ .data = &mcp4131_cfg[MCP414x_503] },
+ { .compatible = "microchip,mcp4142-104",
+ .data = &mcp4131_cfg[MCP414x_104] },
+ { .compatible = "microchip,mcp4151-502",
+ .data = &mcp4131_cfg[MCP415x_502] },
+ { .compatible = "microchip,mcp4151-103",
+ .data = &mcp4131_cfg[MCP415x_103] },
+ { .compatible = "microchip,mcp4151-503",
+ .data = &mcp4131_cfg[MCP415x_503] },
+ { .compatible = "microchip,mcp4151-104",
+ .data = &mcp4131_cfg[MCP415x_104] },
+ { .compatible = "microchip,mcp4152-502",
+ .data = &mcp4131_cfg[MCP415x_502] },
+ { .compatible = "microchip,mcp4152-103",
+ .data = &mcp4131_cfg[MCP415x_103] },
+ { .compatible = "microchip,mcp4152-503",
+ .data = &mcp4131_cfg[MCP415x_503] },
+ { .compatible = "microchip,mcp4152-104",
+ .data = &mcp4131_cfg[MCP415x_104] },
+ { .compatible = "microchip,mcp4161-502",
+ .data = &mcp4131_cfg[MCP416x_502] },
+ { .compatible = "microchip,mcp4161-103",
+ .data = &mcp4131_cfg[MCP416x_103] },
+ { .compatible = "microchip,mcp4161-503",
+ .data = &mcp4131_cfg[MCP416x_503] },
+ { .compatible = "microchip,mcp4161-104",
+ .data = &mcp4131_cfg[MCP416x_104] },
+ { .compatible = "microchip,mcp4162-502",
+ .data = &mcp4131_cfg[MCP416x_502] },
+ { .compatible = "microchip,mcp4162-103",
+ .data = &mcp4131_cfg[MCP416x_103] },
+ { .compatible = "microchip,mcp4162-503",
+ .data = &mcp4131_cfg[MCP416x_503] },
+ { .compatible = "microchip,mcp4162-104",
+ .data = &mcp4131_cfg[MCP416x_104] },
+ { .compatible = "microchip,mcp4231-502",
+ .data = &mcp4131_cfg[MCP423x_502] },
+ { .compatible = "microchip,mcp4231-103",
+ .data = &mcp4131_cfg[MCP423x_103] },
+ { .compatible = "microchip,mcp4231-503",
+ .data = &mcp4131_cfg[MCP423x_503] },
+ { .compatible = "microchip,mcp4231-104",
+ .data = &mcp4131_cfg[MCP423x_104] },
+ { .compatible = "microchip,mcp4232-502",
+ .data = &mcp4131_cfg[MCP423x_502] },
+ { .compatible = "microchip,mcp4232-103",
+ .data = &mcp4131_cfg[MCP423x_103] },
+ { .compatible = "microchip,mcp4232-503",
+ .data = &mcp4131_cfg[MCP423x_503] },
+ { .compatible = "microchip,mcp4232-104",
+ .data = &mcp4131_cfg[MCP423x_104] },
+ { .compatible = "microchip,mcp4241-502",
+ .data = &mcp4131_cfg[MCP424x_502] },
+ { .compatible = "microchip,mcp4241-103",
+ .data = &mcp4131_cfg[MCP424x_103] },
+ { .compatible = "microchip,mcp4241-503",
+ .data = &mcp4131_cfg[MCP424x_503] },
+ { .compatible = "microchip,mcp4241-104",
+ .data = &mcp4131_cfg[MCP424x_104] },
+ { .compatible = "microchip,mcp4242-502",
+ .data = &mcp4131_cfg[MCP424x_502] },
+ { .compatible = "microchip,mcp4242-103",
+ .data = &mcp4131_cfg[MCP424x_103] },
+ { .compatible = "microchip,mcp4242-503",
+ .data = &mcp4131_cfg[MCP424x_503] },
+ { .compatible = "microchip,mcp4242-104",
+ .data = &mcp4131_cfg[MCP424x_104] },
+ { .compatible = "microchip,mcp4251-502",
+ .data = &mcp4131_cfg[MCP425x_502] },
+ { .compatible = "microchip,mcp4251-103",
+ .data = &mcp4131_cfg[MCP425x_103] },
+ { .compatible = "microchip,mcp4251-503",
+ .data = &mcp4131_cfg[MCP425x_503] },
+ { .compatible = "microchip,mcp4251-104",
+ .data = &mcp4131_cfg[MCP425x_104] },
+ { .compatible = "microchip,mcp4252-502",
+ .data = &mcp4131_cfg[MCP425x_502] },
+ { .compatible = "microchip,mcp4252-103",
+ .data = &mcp4131_cfg[MCP425x_103] },
+ { .compatible = "microchip,mcp4252-503",
+ .data = &mcp4131_cfg[MCP425x_503] },
+ { .compatible = "microchip,mcp4252-104",
+ .data = &mcp4131_cfg[MCP425x_104] },
+ { .compatible = "microchip,mcp4261-502",
+ .data = &mcp4131_cfg[MCP426x_502] },
+ { .compatible = "microchip,mcp4261-103",
+ .data = &mcp4131_cfg[MCP426x_103] },
+ { .compatible = "microchip,mcp4261-503",
+ .data = &mcp4131_cfg[MCP426x_503] },
+ { .compatible = "microchip,mcp4261-104",
+ .data = &mcp4131_cfg[MCP426x_104] },
+ { .compatible = "microchip,mcp4262-502",
+ .data = &mcp4131_cfg[MCP426x_502] },
+ { .compatible = "microchip,mcp4262-103",
+ .data = &mcp4131_cfg[MCP426x_103] },
+ { .compatible = "microchip,mcp4262-503",
+ .data = &mcp4131_cfg[MCP426x_503] },
+ { .compatible = "microchip,mcp4262-104",
+ .data = &mcp4131_cfg[MCP426x_104] },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mcp4131_dt_ids);
+#endif /* CONFIG_OF */
+
+static const struct spi_device_id mcp4131_id[] = {
+ { "mcp4131-502", MCP413x_502 },
+ { "mcp4131-103", MCP413x_103 },
+ { "mcp4131-503", MCP413x_503 },
+ { "mcp4131-104", MCP413x_104 },
+ { "mcp4132-502", MCP413x_502 },
+ { "mcp4132-103", MCP413x_103 },
+ { "mcp4132-503", MCP413x_503 },
+ { "mcp4132-104", MCP413x_104 },
+ { "mcp4141-502", MCP414x_502 },
+ { "mcp4141-103", MCP414x_103 },
+ { "mcp4141-503", MCP414x_503 },
+ { "mcp4141-104", MCP414x_104 },
+ { "mcp4142-502", MCP414x_502 },
+ { "mcp4142-103", MCP414x_103 },
+ { "mcp4142-503", MCP414x_503 },
+ { "mcp4142-104", MCP414x_104 },
+ { "mcp4151-502", MCP415x_502 },
+ { "mcp4151-103", MCP415x_103 },
+ { "mcp4151-503", MCP415x_503 },
+ { "mcp4151-104", MCP415x_104 },
+ { "mcp4152-502", MCP415x_502 },
+ { "mcp4152-103", MCP415x_103 },
+ { "mcp4152-503", MCP415x_503 },
+ { "mcp4152-104", MCP415x_104 },
+ { "mcp4161-502", MCP416x_502 },
+ { "mcp4161-103", MCP416x_103 },
+ { "mcp4161-503", MCP416x_503 },
+ { "mcp4161-104", MCP416x_104 },
+ { "mcp4162-502", MCP416x_502 },
+ { "mcp4162-103", MCP416x_103 },
+ { "mcp4162-503", MCP416x_503 },
+ { "mcp4162-104", MCP416x_104 },
+ { "mcp4231-502", MCP423x_502 },
+ { "mcp4231-103", MCP423x_103 },
+ { "mcp4231-503", MCP423x_503 },
+ { "mcp4231-104", MCP423x_104 },
+ { "mcp4232-502", MCP423x_502 },
+ { "mcp4232-103", MCP423x_103 },
+ { "mcp4232-503", MCP423x_503 },
+ { "mcp4232-104", MCP423x_104 },
+ { "mcp4241-502", MCP424x_502 },
+ { "mcp4241-103", MCP424x_103 },
+ { "mcp4241-503", MCP424x_503 },
+ { "mcp4241-104", MCP424x_104 },
+ { "mcp4242-502", MCP424x_502 },
+ { "mcp4242-103", MCP424x_103 },
+ { "mcp4242-503", MCP424x_503 },
+ { "mcp4242-104", MCP424x_104 },
+ { "mcp4251-502", MCP425x_502 },
+ { "mcp4251-103", MCP425x_103 },
+ { "mcp4251-503", MCP425x_503 },
+ { "mcp4251-104", MCP425x_104 },
+ { "mcp4252-502", MCP425x_502 },
+ { "mcp4252-103", MCP425x_103 },
+ { "mcp4252-503", MCP425x_503 },
+ { "mcp4252-104", MCP425x_104 },
+ { "mcp4261-502", MCP426x_502 },
+ { "mcp4261-103", MCP426x_103 },
+ { "mcp4261-503", MCP426x_503 },
+ { "mcp4261-104", MCP426x_104 },
+ { "mcp4262-502", MCP426x_502 },
+ { "mcp4262-103", MCP426x_103 },
+ { "mcp4262-503", MCP426x_503 },
+ { "mcp4262-104", MCP426x_104 },
+ {}
+};
+MODULE_DEVICE_TABLE(spi, mcp4131_id);
+
+static struct spi_driver mcp4131_driver = {
+ .driver = {
+ .name = "mcp4131",
+ .of_match_table = of_match_ptr(mcp4131_dt_ids),
+ },
+ .probe = mcp4131_probe,
+ .id_table = mcp4131_id,
+};
+
+module_spi_driver(mcp4131_driver);
+
+MODULE_AUTHOR("Slawomir Stepien <sst@poczta.fm>");
+MODULE_DESCRIPTION("MCP4131 digital potentiometer");
+MODULE_LICENSE("GPL v2");
--
2.7.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
2016-03-20 14:30 [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Slawomir Stepien
@ 2016-03-20 16:12 ` Joachim Eastwood
2016-03-20 17:24 ` Slawomir Stepien
2016-03-20 17:25 ` Jonathan Cameron
0 siblings, 2 replies; 4+ messages in thread
From: Joachim Eastwood @ 2016-03-20 16:12 UTC (permalink / raw)
To: Slawomir Stepien
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel@vger.kernel.org
Hi Slawomir,
On 20 March 2016 at 15:30, Slawomir Stepien <sst@poczta.fm> wrote:
> 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.
> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
>
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
> +
> +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) {
> + 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.
> +
> + 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;
> + }
> +
> + *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?
> + 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);
> + 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
2016-03-20 16:12 ` Joachim Eastwood
@ 2016-03-20 17:24 ` Slawomir Stepien
2016-03-20 17:25 ` Jonathan Cameron
1 sibling, 0 replies; 4+ messages in thread
From: Slawomir Stepien @ 2016-03-20 17:24 UTC (permalink / raw)
To: Joachim Eastwood
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel@vger.kernel.org
On Mar 20, 2016 17:12, Joachim Eastwood wrote:
> Hi Slawomir,
>
> On 20 March 2016 at 15:30, Slawomir Stepien <sst@poczta.fm> wrote:
> > 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.
Ok
> > + 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.
Ok I'll try that.
> > + 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?
Yes you're right here. Didn't see it like that.
> > +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);
> > + 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.
I'll see if it's possible to move the whole locking into this function.
Thank you for comments.
> regards,
> Joachim Eastwood
--
Slawomir Stepien
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
2016-03-20 16:12 ` Joachim Eastwood
2016-03-20 17:24 ` Slawomir Stepien
@ 2016-03-20 17:25 ` Jonathan Cameron
1 sibling, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2016-03-20 17:25 UTC (permalink / raw)
To: Joachim Eastwood, Slawomir Stepien
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, linux-kernel@vger.kernel.org
On 20/03/16 16:12, Joachim Eastwood wrote:
> Hi Slawomir,
>
> On 20 March 2016 at 15:30, Slawomir Stepien <sst@poczta.fm> wrote:
>> 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.
>
>> Datasheet: http://ww1.microchip.com/downloads/en/DeviceDoc/22060b.pdf
>>
>> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
>
>> +
>> +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) {
>> + 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.
>
> Also it these any reason why the data buffer can just be a local
> variable in mcp4131_read_raw/mcp4131_write_raw?
Only with if it is allocated on the heap as required to enforce the rule
it is on a separate cache line. Much easier to do that once!
> If it could be I think it should be possible to move the lock into the
> mcp4131_exec function.
>
>> +
>> + 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;
>> + }
>> +
>> + *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?
>
>
>> + 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);
>> + 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-20 17:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-20 14:30 [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X Slawomir Stepien
2016-03-20 16:12 ` Joachim Eastwood
2016-03-20 17:24 ` Slawomir Stepien
2016-03-20 17:25 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).