* add mcp4728 I2C DAC driver
@ 2023-07-10 20:43 Andrea Collamati
2023-07-11 7:33 ` Krzysztof Kozlowski
0 siblings, 1 reply; 2+ messages in thread
From: Andrea Collamati @ 2023-07-10 20:43 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Andrea Collamati
Cc: linux-iio, devicetree, linux-kernel
From 01b156ca1b27be83f4c74c288dbc0bcad178fe0b Mon Sep 17 00:00:00 2001
From: Andrea Collamati <andrea.collamati@gmail.com>
Date: Mon, 10 Jul 2023 16:20:40 +0200
Subject: [PATCH] iio: add mcp4728 I2C DAC driver
Microchip MCP4728 is a 12-bit quad channel
digital-to-analog converter (DAC) with I2C interface.
This patch adds support for per-channel gain, power state and power down mode control.
Current state could be saved to on-chip EEPROM.
Internal voltage reference and external vdd ref are supported.
---
.../bindings/iio/dac/microchip,mcp4728.yaml | 42 ++
drivers/iio/dac/Kconfig | 12 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/mcp4728.c | 641 ++++++++++++++++++
4 files changed, 696 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
create mode 100644 drivers/iio/dac/mcp4728.c
diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
new file mode 100644
index 000000000000..68f4e359a921
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip mcp4728
+
+maintainers:
+ - Andrea Collamati <andrea.collamati@gmail.com>
+
+properties:
+ compatible:
+ enum:
+ - microchip,mcp4728
+ reg:
+ maxItems: 1
+
+ vdd-supply:
+ description: |
+ Provides both power and acts as the reference supply on the mcp4728
+ when Internal Vref is not selected.
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mcp4728_dac@64 {
+ compatible = "microchip,mcp4728";
+ reg = <0x60>;
+ vdd-supply = <&vdac_vdd>;
+ };
+ };
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index c0bf0d84197f..6c96cd30994a 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -376,6 +376,18 @@ config MCP4725
To compile this driver as a module, choose M here: the module
will be called mcp4725.
+config MCP4728
+ tristate "MCP4728 DAC driver"
+ depends on I2C
+ help
+ Say Y here if you want to build a driver for the Microchip
+ MCP4728 quad channel, 12-bit digital-to-analog converter (DAC)
+ with I2C interface.
+
+ To compile this driver as a module, choose M here: the module
+ will be called mcp4728.
+
+
config MCP4922
tristate "MCP4902, MCP4912, MCP4922 DAC driver"
depends on SPI
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index ec3e42713f00..35ac62a61b05 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_M62332) += m62332.o
obj-$(CONFIG_MAX517) += max517.o
obj-$(CONFIG_MAX5821) += max5821.o
obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP4728) += mcp4728.o
obj-$(CONFIG_MCP4922) += mcp4922.o
obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
obj-$(CONFIG_STM32_DAC) += stm32-dac.o
diff --git a/drivers/iio/dac/mcp4728.c b/drivers/iio/dac/mcp4728.c
new file mode 100644
index 000000000000..d304fa8ffa26
--- /dev/null
+++ b/drivers/iio/dac/mcp4728.c
@@ -0,0 +1,641 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for Microchip MCP4728
+ *
+ * Copyright (C) 2023 Andrea Collamati <andrea.collamati@gmail.com>
+ *
+ * Based on mcp4725 by Peter Meerwald <pmeerw@pmeerw.net>
+ *
+ * Driver for the Microchip I2C 12-bit digital-to-analog quad channels
+ * converter (DAC).
+ *
+ * (7-bit I2C slave address 0x60, the three LSBs can be configured in
+ * hardware)
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define MCP4728_DRV_NAME "mcp4728"
+
+#define MCP4728_RESOLUTION 12
+#define MCP4728_N_CHANNELS 4
+
+#define MCP4728_CMD_POS 3
+#define MCP4728_CMD_UDAC_POS 0
+#define MCP4728_CMD_CH_SEL_POS 1
+
+#define MCP4728_CMD_VREF_MASK 0x80
+#define MCP4728_CMD_VREF_POS 7
+
+#define MCP4728_CMD_PDMODE_MASK 0x60
+#define MCP4728_CMD_PDMODE_POS 5
+
+#define MCP4728_CMD_GAIN_MASK 0x10
+#define MCP4728_CMD_GAIN_POS 4
+
+#define MCP4728_MW_CMD 0x08 // Multiwrite Command
+#define MCP4728_SW_CMD 0x0A // Sequential Write Command (include eeprom)
+
+#define MCP4728_READ_RESPONSE_LEN (MCP4728_N_CHANNELS * 3 * 2) // Read Message
+#define MCP4728_WRITE_EEPROM_LEN \
+ (1 + MCP4728_N_CHANNELS * 2) // Sequential Write
+
+enum vref_mode {
+ MCP4728_VREF_EXTERNAL_VDD = 0,
+ MCP4728_VRED_INTERNAL_2048mV = 1,
+};
+
+enum gain_mode {
+ MCP4728_GAIN_X1 = 0,
+ MCP4728_GAIN_X2 = 1,
+};
+
+enum iio_powerdown_mode {
+ MCP4728_IIO_1K,
+ MCP4728_IIO_100K,
+ MCP4728_IIO_500K,
+};
+
+struct mcp4728_channel_data {
+ enum vref_mode ref_mode;
+ enum iio_powerdown_mode pd_mode;
+ enum gain_mode g_mode;
+ u16 dac_value;
+};
+
+struct mcp4728_data {
+ struct i2c_client *client;
+ int id;
+ struct regulator *vdd_reg;
+ bool powerdown;
+ struct mcp4728_channel_data channel_data[MCP4728_N_CHANNELS];
+};
+
+#define MCP4728_CHAN(chan) \
+ { \
+ .type = IIO_VOLTAGE, .output = 1, .indexed = 1, \
+ .channel = chan, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .ext_info = mcp4728_ext_info, \
+ }
+
+static int mcp4728_suspend(struct device *dev);
+static int mcp4728_resume(struct device *dev);
+
+static ssize_t mcp4728_store_eeprom(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ u8 outbuf[MCP4728_WRITE_EEPROM_LEN];
+ int tries = 20;
+ u8 inbuf[3];
+ bool state;
+ int ret;
+ unsigned int i;
+
+ ret = kstrtobool(buf, &state);
+ if (ret < 0)
+ return ret;
+
+ if (!state)
+ return 0;
+
+ outbuf[0] = MCP4728_SW_CMD << MCP4728_CMD_POS; // Command ID
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ struct mcp4728_channel_data *ch = &(data->channel_data[i]);
+ int offset = 1 + i * 2;
+
+ outbuf[offset] = ch->ref_mode << MCP4728_CMD_VREF_POS;
+ if (data->powerdown) {
+ u8 mcp4728_pd_mode = ch->pd_mode + 1;
+
+ outbuf[1] |= mcp4728_pd_mode << MCP4728_CMD_PDMODE_POS;
+ }
+
+ outbuf[offset] |= ch->g_mode << MCP4728_CMD_GAIN_POS;
+ outbuf[offset] |= ch->dac_value >> 8;
+ outbuf[offset + 1] = ch->dac_value & 0xff;
+ }
+
+ ret = i2c_master_send(data->client, outbuf, MCP4728_WRITE_EEPROM_LEN);
+ if (ret < 0)
+ return ret;
+ else if (ret != MCP4728_WRITE_EEPROM_LEN)
+ return -EIO;
+
+ /* wait RDY signal for write complete, takes up to 50ms */
+ while (tries--) {
+ msleep(20);
+ ret = i2c_master_recv(data->client, inbuf, 3);
+ if (ret < 0)
+ return ret;
+ else if (ret != 3)
+ return -EIO;
+
+ if (inbuf[0] & 0x80) // check RDY flag
+ break;
+ }
+
+ if (tries < 0) {
+ dev_err(&data->client->dev, "%s failed, incomplete\n",
+ __func__);
+ return -EIO;
+ }
+ return len;
+}
+
+static IIO_DEVICE_ATTR(store_eeprom, 0200, NULL, mcp4728_store_eeprom, 0);
+
+static struct attribute *mcp4728_attributes[] = {
+ &iio_dev_attr_store_eeprom.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group mcp4728_attribute_group = {
+ .attrs = mcp4728_attributes,
+};
+
+enum chip_id {
+ MCP4728,
+};
+
+static int mcp4728_program_channel_cfg(int channel, struct iio_dev *indio_dev)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ struct mcp4728_channel_data *ch = &(data->channel_data[channel]);
+ u8 outbuf[3];
+ int ret;
+
+ outbuf[0] = MCP4728_MW_CMD << MCP4728_CMD_POS; // Command ID
+ outbuf[0] |= channel << MCP4728_CMD_CH_SEL_POS; // Channel Selector
+ outbuf[0] |= 0; // UDAC = 0
+
+ outbuf[1] = ch->ref_mode << MCP4728_CMD_VREF_POS;
+ if (data->powerdown) {
+ u8 mcp4728_pd_mode = ch->pd_mode + 1;
+
+ outbuf[1] |= mcp4728_pd_mode << MCP4728_CMD_PDMODE_POS;
+ }
+
+ outbuf[1] |= ch->g_mode << MCP4728_CMD_GAIN_POS;
+
+ outbuf[1] |= ch->dac_value >> 8;
+ outbuf[2] = ch->dac_value & 0xff;
+
+ ret = i2c_master_send(data->client, outbuf, 3);
+ if (ret < 0)
+ return ret;
+ else if (ret != 3)
+ return -EIO;
+ else
+ return 0;
+}
+
+// powerdown mode
+static const char *const mcp4728_powerdown_modes[] = {
+ "1kohm_to_gnd",
+ "100kohm_to_gnd",
+ "500kohm_to_gnd"
+};
+
+static int mcp4728_get_powerdown_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ return data->channel_data[chan->channel].pd_mode;
+}
+
+static int mcp4728_set_powerdown_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int mode)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ data->channel_data[chan->channel].pd_mode = mode;
+
+ return 0;
+}
+
+static ssize_t mcp4728_read_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ return sysfs_emit(buf, "%d\n", data->powerdown);
+}
+
+static ssize_t mcp4728_write_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf, size_t len)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ bool state;
+ int ret;
+
+ ret = kstrtobool(buf, &state);
+ if (ret)
+ return ret;
+
+ if (state)
+ ret = mcp4728_suspend(&data->client->dev);
+ else
+ ret = mcp4728_resume(&data->client->dev);
+ if (ret < 0)
+ return ret;
+
+ return len;
+}
+
+static const struct iio_enum mcp4728_powerdown_mode_enum = {
+ .items = mcp4728_powerdown_modes,
+ .num_items = ARRAY_SIZE(mcp4728_powerdown_modes),
+ .get = mcp4728_get_powerdown_mode,
+ .set = mcp4728_set_powerdown_mode,
+};
+
+// vref mode
+static const char *const mcp4728_vref_modes[] = {
+ "vdd_ext",
+ "internal",
+};
+
+static int mcp4728_get_vref_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ return data->channel_data[chan->channel].ref_mode;
+}
+
+static int mcp4728_set_vref_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int mode)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int ret;
+
+ data->channel_data[chan->channel].ref_mode = mode;
+
+ if (mode == MCP4728_VREF_EXTERNAL_VDD &&
+ data->channel_data[chan->channel].g_mode == MCP4728_GAIN_X2) {
+ dev_warn(
+ &data->client->dev,
+ "CH%d: Gain x2 not effective when vref is vdd, force to x1",
+ chan->channel);
+ data->channel_data[chan->channel].g_mode = MCP4728_GAIN_X1;
+ }
+
+ ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
+
+ return ret;
+}
+static const struct iio_enum mcp4728_vref_mode_enum = {
+ .items = mcp4728_vref_modes,
+ .num_items = ARRAY_SIZE(mcp4728_vref_modes),
+ .get = mcp4728_get_vref_mode,
+ .set = mcp4728_set_vref_mode,
+};
+
+// gain
+static const char *const mcp4728_gain_modes[] = {
+ "x1",
+ "x2",
+};
+
+static int mcp4728_get_gain_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ return data->channel_data[chan->channel].g_mode;
+}
+
+static int mcp4728_set_gain_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int mode)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int ret;
+
+ if (mode == MCP4728_GAIN_X2 &&
+ data->channel_data[chan->channel].ref_mode ==
+ MCP4728_VREF_EXTERNAL_VDD) {
+ dev_err(&data->client->dev,
+ "CH%d: Gain x2 not effective when vref is vdd",
+ chan->channel);
+ return -EINVAL;
+ }
+
+ data->channel_data[chan->channel].g_mode = mode;
+
+ ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
+
+ return ret;
+}
+
+static const struct iio_enum mcp4728_gain_mode_enum = {
+ .items = mcp4728_gain_modes,
+ .num_items = ARRAY_SIZE(mcp4728_gain_modes),
+ .get = mcp4728_get_gain_mode,
+ .set = mcp4728_set_gain_mode,
+};
+
+static const struct iio_chan_spec_ext_info mcp4728_ext_info[] = {
+ {
+ .name = "powerdown",
+ .read = mcp4728_read_powerdown,
+ .write = mcp4728_write_powerdown,
+ .shared = IIO_SEPARATE,
+ },
+ IIO_ENUM("powerdown_mode", IIO_SEPARATE, &mcp4728_powerdown_mode_enum),
+ IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
+ &mcp4728_powerdown_mode_enum),
+ IIO_ENUM("vref_mode", IIO_SEPARATE, &mcp4728_vref_mode_enum),
+ IIO_ENUM_AVAILABLE("vref_mode", IIO_SHARED_BY_TYPE,
+ &mcp4728_vref_mode_enum),
+ IIO_ENUM("gain_mode", IIO_SEPARATE, &mcp4728_gain_mode_enum),
+ IIO_ENUM_AVAILABLE("gain_mode", IIO_SHARED_BY_TYPE,
+ &mcp4728_gain_mode_enum),
+ {},
+};
+
+static const struct iio_chan_spec mcp4728_channels[MCP4728_N_CHANNELS] = {
+ MCP4728_CHAN(0),
+ MCP4728_CHAN(1),
+ MCP4728_CHAN(2),
+ MCP4728_CHAN(3),
+};
+
+static int mcp4728_full_scale_mV(u32 *full_scale_mV, int channel,
+ struct mcp4728_data *data)
+{
+ int ret;
+
+ if (data->channel_data[channel].ref_mode == MCP4728_VREF_EXTERNAL_VDD)
+ ret = regulator_get_voltage(data->vdd_reg);
+ else
+ ret = 2048000;
+
+ if (ret < 0)
+ return ret;
+
+ if (ret == 0)
+ return -EINVAL;
+
+ *full_scale_mV = ret / 1000;
+ return 0;
+}
+
+static u32 mcp4728_raw_to_mV(u32 raw, int channel, struct mcp4728_data *data)
+{
+ int ret;
+ u32 full_scale_mV;
+
+ ret = mcp4728_full_scale_mV(&full_scale_mV, channel, data);
+ if (ret)
+ return ret;
+
+ return (((raw + 1) * full_scale_mV) >> MCP4728_RESOLUTION);
+}
+
+static int mcp4728_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ *val = data->channel_data[chan->channel].dac_value;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ if (data->channel_data[chan->channel].ref_mode ==
+ MCP4728_VREF_EXTERNAL_VDD)
+ ret = regulator_get_voltage(data->vdd_reg);
+ else
+ ret = 2048000;
+
+ if (ret < 0)
+ return ret;
+
+ *val = ret / 1000;
+ *val2 = MCP4728_RESOLUTION;
+ return IIO_VAL_FRACTIONAL_LOG2;
+ }
+ return -EINVAL;
+}
+
+static int mcp4728_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (val < 0 || val > GENMASK(MCP4728_RESOLUTION - 1, 0))
+ return -EINVAL;
+ data->channel_data[chan->channel].dac_value = val;
+ ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static const struct iio_info mcp4728_info = {
+ .read_raw = mcp4728_read_raw,
+ .write_raw = mcp4728_write_raw,
+ .attrs = &mcp4728_attribute_group,
+};
+
+static int mcp4728_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int err = 0;
+ unsigned int i;
+
+ data->powerdown = true;
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ int ret = mcp4728_program_channel_cfg(i, indio_dev);
+
+ if (ret)
+ err = ret; //save last error
+ }
+ return err;
+}
+
+static int mcp4728_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct mcp4728_data *data = iio_priv(indio_dev);
+ int err = 0;
+ unsigned int i;
+
+ data->powerdown = false;
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ int ret = mcp4728_program_channel_cfg(i, indio_dev);
+
+ if (ret)
+ err = ret; //save last error
+ }
+ return err;
+}
+static DEFINE_SIMPLE_DEV_PM_OPS(mcp4728_pm_ops, mcp4728_suspend,
+ mcp4728_resume);
+
+static int mcp4728_init_channels_data(struct mcp4728_data *data)
+{
+ u8 inbuf[MCP4728_READ_RESPONSE_LEN];
+ int ret;
+ unsigned int i;
+
+ ret = i2c_master_recv(data->client, inbuf, MCP4728_READ_RESPONSE_LEN);
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "failed to read mcp4728 conf. Err=%d\n", ret);
+ return ret;
+ } else if (ret != MCP4728_READ_RESPONSE_LEN) {
+ dev_err(&data->client->dev,
+ "failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
+ ret);
+ return -EIO;
+ }
+
+ for (i = 0; i < MCP4728_N_CHANNELS; i++) {
+ struct mcp4728_channel_data *ch = &(data->channel_data[i]);
+ u8 r2 = inbuf[i * 6 + 1];
+ u8 r3 = inbuf[i * 6 + 2];
+ u32 dac_mv;
+
+ ch->dac_value = (r2 & 0x0F) << 8 | r3;
+ ch->ref_mode = (r2 & MCP4728_CMD_VREF_MASK) >>
+ MCP4728_CMD_VREF_POS;
+ ch->pd_mode = (r2 & MCP4728_CMD_PDMODE_MASK) >>
+ MCP4728_CMD_PDMODE_POS;
+ ch->g_mode = (r2 & MCP4728_CMD_GAIN_MASK) >>
+ MCP4728_CMD_GAIN_POS;
+ if (ch->g_mode == MCP4728_GAIN_X2 &&
+ ch->ref_mode == MCP4728_VREF_EXTERNAL_VDD)
+ dev_warn(&data->client->dev,
+ "CH%d: Gain x2 not effective when vref is vdd",
+ i);
+
+ dac_mv = mcp4728_raw_to_mV(ch->dac_value, i, data);
+ dev_info(&data->client->dev,
+ "CH%d: Voltage=%dmV VRef=%d PowerDown=%d Gain=%d\n", i,
+ dac_mv, ch->ref_mode, ch->pd_mode, ch->g_mode);
+ }
+
+ return 0;
+}
+
+static int mcp4728_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct mcp4728_data *data;
+ struct iio_dev *indio_dev;
+ int err;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (indio_dev == NULL)
+ return -ENOMEM;
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+ if (dev_fwnode(&client->dev))
+ data->id = (uintptr_t)device_get_match_data(&client->dev);
+ else
+ data->id = id->driver_data;
+
+ data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+ if (IS_ERR(data->vdd_reg))
+ return PTR_ERR(data->vdd_reg);
+
+ err = regulator_enable(data->vdd_reg);
+ if (err)
+ goto err_disable_vdd_reg;
+
+ err = mcp4728_init_channels_data(data);
+ if (err) {
+ dev_err(&client->dev,
+ "failed to read mcp4728 current configuration\n");
+ goto err_disable_vdd_reg;
+ }
+
+ indio_dev->name = id->name;
+ indio_dev->info = &mcp4728_info;
+ indio_dev->channels = mcp4728_channels;
+ indio_dev->num_channels = MCP4728_N_CHANNELS;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ err = iio_device_register(indio_dev);
+ if (err)
+ goto err_disable_vdd_reg;
+
+ return 0;
+
+err_disable_vdd_reg:
+ regulator_disable(data->vdd_reg);
+
+ return err;
+}
+
+static int mcp4728_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct mcp4728_data *data = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+ regulator_disable(data->vdd_reg);
+ return 0;
+}
+
+static const struct i2c_device_id mcp4728_id[] = { { "mcp4728", MCP4728 }, {} };
+MODULE_DEVICE_TABLE(i2c, mcp4728_id);
+
+static const struct of_device_id mcp4728_of_match[] = {
+ { .compatible = "microchip,mcp4728", .data = (void *)MCP4728 },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mcp4728_of_match);
+
+static struct i2c_driver mcp4728_driver = {
+ .driver = {
+ .name = MCP4728_DRV_NAME,
+ .of_match_table = mcp4728_of_match,
+ .pm = pm_sleep_ptr(&mcp4728_pm_ops),
+ },
+ .probe = mcp4728_probe,
+ .remove = mcp4728_remove,
+ .id_table = mcp4728_id,
+};
+module_i2c_driver(mcp4728_driver);
+
+MODULE_AUTHOR("Andrea Collamati <andrea.collamati@gmail.com>");
+MODULE_DESCRIPTION("MCP4728 12-bit DAC");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: add mcp4728 I2C DAC driver
2023-07-10 20:43 add mcp4728 I2C DAC driver Andrea Collamati
@ 2023-07-11 7:33 ` Krzysztof Kozlowski
0 siblings, 0 replies; 2+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-11 7:33 UTC (permalink / raw)
To: Andrea Collamati, Jonathan Cameron, Lars-Peter Clausen, Rob Herring
Cc: linux-iio, devicetree, linux-kernel
Thank you for your patch. There is something to discuss/improve.
On 10/07/2023 22:43, Andrea Collamati wrote:
> From 01b156ca1b27be83f4c74c288dbc0bcad178fe0b Mon Sep 17 00:00:00 2001
> From: Andrea Collamati <andrea.collamati@gmail.com>
> Date: Mon, 10 Jul 2023 16:20:40 +0200
> Subject: [PATCH] iio: add mcp4728 I2C DAC driver
1. That is not a proper patch header. I don't know how you got it, but
it's wrong. Use just b4 or git format-patch and git send-email.
2. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC (and consider --no-git-fallback argument). It
might happen, that command when run on an older kernel, gives you
outdated entries. Therefore please be sure you base your patches on
recent Linux kernel.
>
> Microchip MCP4728 is a 12-bit quad channel
> digital-to-analog converter (DAC) with I2C interface.
>
> This patch adds support for per-channel gain, power state and power down mode control.
3. Please do not use "This commit/patch", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> Current state could be saved to on-chip EEPROM.
> Internal voltage reference and external vdd ref are supported.
>
> ---
> .../bindings/iio/dac/microchip,mcp4728.yaml | 42 ++
4. Bindings are always separate patches.
5. Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
> drivers/iio/dac/Kconfig | 12 +
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/mcp4728.c | 641 ++++++++++++++++++
> 4 files changed, 696 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> create mode 100644 drivers/iio/dac/mcp4728.c
>
> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> new file mode 100644
> index 000000000000..68f4e359a921
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip mcp4728
6. mcp or MCP? What is this? Proper title is missing... also no
description.
> +
> +maintainers:
> + - Andrea Collamati <andrea.collamati@gmail.com>
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,mcp4728
7. Blank line
8. Whitespace errors
> + reg:
> + maxItems: 1
> +
> + vdd-supply:
> + description: |
> + Provides both power and acts as the reference supply on the mcp4728
> + when Internal Vref is not selected.
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + mcp4728_dac@64 {
9. Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Also, underscores are not allowed in node names.
Shouldn't this binding be just merged with existing mcp4725? Are you
sure it's not similar device, IOW, are you sure you do not have vref supply?
> + compatible = "microchip,mcp4728";
> + reg = <0x60>;
> + vdd-supply = <&vdac_vdd>;
> + };
> + };
...
> +
> +static int mcp4728_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct mcp4728_data *data;
> + struct iio_dev *indio_dev;
> + int err;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (indio_dev == NULL)
> + return -ENOMEM;
Missing blank line
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> + if (dev_fwnode(&client->dev))
> + data->id = (uintptr_t)device_get_match_data(&client->dev);
> + else
> + data->id = id->driver_data;
Dead code, drop.
> +
> + data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> + if (IS_ERR(data->vdd_reg))
> + return PTR_ERR(data->vdd_reg);
> +
> + err = regulator_enable(data->vdd_reg);
> + if (err)
> + goto err_disable_vdd_reg;
> +
> + err = mcp4728_init_channels_data(data);
> + if (err) {
> + dev_err(&client->dev,
> + "failed to read mcp4728 current configuration\n");
None of your statements look properly aligned. What's more everything
has incorrect indentation.
Run checkpatch --strict and fix ALL warnings and errors.
> + goto err_disable_vdd_reg;
> + }
> +
> + indio_dev->name = id->name;
> + indio_dev->info = &mcp4728_info;
> + indio_dev->channels = mcp4728_channels;
> + indio_dev->num_channels = MCP4728_N_CHANNELS;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + err = iio_device_register(indio_dev);
> + if (err)
> + goto err_disable_vdd_reg;
> +
> + return 0;
> +
> +err_disable_vdd_reg:
> + regulator_disable(data->vdd_reg);
> +
> + return err;
> +}
> +
> +static int mcp4728_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct mcp4728_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + regulator_disable(data->vdd_reg);
> + return 0;
> +}
> +
> +static const struct i2c_device_id mcp4728_id[] = { { "mcp4728", MCP4728 }, {} };
That's some odd formatting. Look at existing files:
git grep i2c_device_id
> +MODULE_DEVICE_TABLE(i2c, mcp4728_id);
> +
> +static const struct of_device_id mcp4728_of_match[] = {
> + { .compatible = "microchip,mcp4728", .data = (void *)MCP4728 },
Drop unused MCP4728.
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mcp4728_of_match);
> +
> +static struct i2c_driver mcp4728_driver = {
> + .driver = {
> + .name = MCP4728_DRV_NAME,
> + .of_match_table = mcp4728_of_match,
> + .pm = pm_sleep_ptr(&mcp4728_pm_ops),
> + },
> + .probe = mcp4728_probe,
> + .remove = mcp4728_remove,
> + .id_table = mcp4728_id,
> +};
> +module_i2c_driver(mcp4728_driver);
> +
> +MODULE_AUTHOR("Andrea Collamati <andrea.collamati@gmail.com>");
> +MODULE_DESCRIPTION("MCP4728 12-bit DAC");
> +MODULE_LICENSE("GPL");
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-07-11 7:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-10 20:43 add mcp4728 I2C DAC driver Andrea Collamati
2023-07-11 7:33 ` Krzysztof Kozlowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.