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]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("MCP4131 digital potentiometer");
+MODULE_LICENSE("GPL v2");
--
2.7.3
Hi Slawomir,
On 20 March 2016 at 15:30, Slawomir Stepien <[email protected]> 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 <[email protected]>
> +
> +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
On Mar 20, 2016 17:12, Joachim Eastwood wrote:
> Hi Slawomir,
>
> On 20 March 2016 at 15:30, Slawomir Stepien <[email protected]> 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
On 20/03/16 16:12, Joachim Eastwood wrote:
> Hi Slawomir,
>
> On 20 March 2016 at 15:30, Slawomir Stepien <[email protected]> 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 <[email protected]>
>
>> +
>> +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
>
Hi Jonathan,
On 20 March 2016 at 18:25, Jonathan Cameron <[email protected]> wrote:
> On 20/03/16 16:12, Joachim Eastwood wrote:
>>> +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.
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
On 20 March 2016 18:15:50 GMT+00:00, Joachim Eastwood <[email protected]> wrote:
>Hi Jonathan,
>
>On 20 March 2016 at 18:25, Jonathan Cameron <[email protected]> wrote:
>> On 20/03/16 16:12, Joachim Eastwood wrote:
>>>> +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.
>
>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.
Agreed
>
>
>regards,
>Joachim Eastwood
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Mar 20, 2016 19:15, Joachim Eastwood wrote:
> Hi Jonathan,
>
> On 20 March 2016 at 18:25, Jonathan Cameron <[email protected]> wrote:
> > On 20/03/16 16:12, Joachim Eastwood wrote:
> >>> +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.
>
> 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.
Just to see if I get it.
For write case I should use the spi_write as it is:
case MCP4131_WRITE:
spi_write(...);
For read case I should create new function (e.g. mcp4131_read) that will look
like spi_read but with additional tx_buf content so I can read the data on miso?
case MCP4131_READ:
mcp4131_read(...)
Keep the needed buffers (transfer/message) local.
--
Slawomir Stepien