add MCP4728 I2C DAC driver​

Add documentation for MCP4728

Signed-off-by Andrea Collamati <[email protected]>

---
 .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +11 -0

new file mode 100644
index 000000000000..6fd9be076245
@@ -0,0 +1,48 @@
+# 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 DAC
+
+description:
+  MCP4728 is a quad channel, 12-bit voltage output
+  Digital-to-Analog Converter with non-volatile

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +27 -0

+  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@60 {
+            compatible = "microchip,mcp4728";
+            reg = <0x60>;
+            vdd-supply = <&vdac_vdd>;
+        };
+    };

Signature · 1 line

2.34.1

Quoted 31 lines

Add documentation for MCP4728

Signed-off-by Andrea Collamati <[email protected]>

---
 .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +17 -0

new file mode 100644
index 000000000000..6fd9be076245
@@ -0,0 +1,48 @@
+# 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 DAC
+
+description:
+  MCP4728 is a quad channel, 12-bit voltage output
+  Digital-to-Analog Converter with non-volatile
+  memory and I2C compatible Serial Interface.
+  https://www.microchip.com/en-us/product/mcp4728
+
+maintainers:
+  - Andrea Collamati <[email protected]>
+

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21

Quoted 4 lines

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +4 -0

+properties:
+  compatible:
+    enum:
+      - microchip,mcp4728

This can just be
compatible:
const: microchip,mcp47288
since you only have a single item in your enum.

Add documentation for MCP4728

Signed-off-by Andrea Collamati <[email protected]>

---
 .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +11 -0

new file mode 100644
index 000000000000..6fd9be076245
@@ -0,0 +1,48 @@
+# 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 DAC
+
+description:
+  MCP4728 is a quad channel, 12-bit voltage output
+  Digital-to-Analog Converter with non-volatile

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +12 -0

+  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

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 43

Quoted 6 lines

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +6 -0

+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mcp4728@60 {

The same... Probably more comments were ignored, so:

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.

Quoted 6 lines · Krzysztof Kozlowski

Quoted 6 lines

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +6 -0

+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mcp4728@60 {

Quoted 8 lines

The same... Probably more comments were ignored, so:

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them.

Quoted 28 lines

Add documentation for MCP4728

Signed-off-by Andrea Collamati <[email protected]>

---
 .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +14 -0

new file mode 100644
index 000000000000..6fd9be076245
@@ -0,0 +1,48 @@
+# 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 DAC
+
+description:
+  MCP4728 is a quad channel, 12-bit voltage output
+  Digital-to-Analog Converter with non-volatile
+  memory and I2C compatible Serial Interface.
+  https://www.microchip.com/en-us/product/mcp4728
+

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21

Quoted 7 lines

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +7 -0

+maintainers:
+  - Andrea Collamati <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - microchip,mcp4728

This is a friendly reminder during the review process.

Hi Krzysztof,

Quoted 28 lines · Krzysztof Kozlowski

Quoted 28 lines

Add documentation for MCP4728

Signed-off-by Andrea Collamati <[email protected]>

---
 .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +14 -0

new file mode 100644
index 000000000000..6fd9be076245
@@ -0,0 +1,48 @@
+# 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 DAC
+
+description:
+  MCP4728 is a quad channel, 12-bit voltage output
+  Digital-to-Analog Converter with non-volatile
+  memory and I2C compatible Serial Interface.
+  https://www.microchip.com/en-us/product/mcp4728
+

Quoted 8 lines

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml line 21

Quoted 7 lines

Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml new file +7 -0

+maintainers:
+  - Andrea Collamati <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - microchip,mcp4728

This is a friendly reminder during the review process.

Sorry but I didn't understand all your requests:

- I changed in the title mcp4728 with MCP4728

- I added description

but I don't know which blank line or whitespaces should be removed.

Can you tell me please?

MCP4728 is a 12-bit quad channel DAC with I2C interface.

support for:
* per-channel gain
* per-channel power state
* per-channel power down mode control
* per-channel vref selection internal/vdd
* store current state to on-chip EEPROM

Signed-off-by Andrea Collamati <[email protected]>

---
 drivers/iio/dac/Kconfig   |  11 +
 drivers/iio/dac/Makefile  |   1 +
 drivers/iio/dac/mcp4728.c | 638 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 650 insertions(+)
 create mode 100644 drivers/iio/dac/mcp4728.c

drivers/iio/dac/mcp4728.c new file +12 -0

new file mode 100644
index 000000000000..39237dba27ba
@@ -0,0 +1,638 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for Microchip MCP4728
+ *
+ * Copyright (C) 2023 Andrea Collamati <[email protected]>
+ *
+ * Based on mcp4725 by Peter Meerwald <[email protected]>
+ *
+ * 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

drivers/iio/dac/mcp4728.c new file +6 -0

+
+#define MCP4728_RESOLUTION 12
+#define MCP4728_N_CHANNELS 4
+
+#define MCP4728_CMD_POS 3
+#define MCP4728_CMD_UDAC_POS 0

drivers/iio/dac/mcp4728.c new file +15 -0

+
+#define MCP4728_READ_RESPONSE_LEN (MCP4728_N_CHANNELS * 3 * 2)
+#define MCP4728_WRITE_EEPROM_LEN (1 + MCP4728_N_CHANNELS * 2)
 
 
+
+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;

drivers/iio/dac/mcp4728.c new file +42 -0

+		if (data->powerdown) {
+			u8 mcp4728_pd_mode = ch->pd_mode + 1;
+
+			outbuf[offset] |= 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[] = {

drivers/iio/dac/mcp4728.c new file +49 -0

+		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;

drivers/iio/dac/mcp4728.c new file +17 -0

+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)
+{

drivers/iio/dac/mcp4728.c new file +46 -0

+};
+
+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,

drivers/iio/dac/mcp4728.c new file +30 -0

+	{},
+};
+
+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;
+}
+

drivers/iio/dac/mcp4728.c new file +69 -0

+}
+
+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);

drivers/iio/dac/mcp4728.c new file +16 -0

+	}
+	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,

drivers/iio/dac/mcp4728.c new file +17 -0

+	}
+
+	return 0;
+}
+
+static int mcp4728_probe(struct i2c_client *client)
+{
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+	struct mcp4728_data *data;
+	struct iio_dev *indio_dev;
+	int err;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);

drivers/iio/dac/mcp4728.c new file +23 -0

+
+	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);
+

drivers/iio/dac/mcp4728.c new file +29 -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" },
+	{}
+};
+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 <[email protected]>");
+MODULE_DESCRIPTION("MCP4728 12-bit DAC");
+MODULE_LICENSE("GPL");

drivers/iio/dac/mcp4728.c line 194

Quoted 10 lines

drivers/iio/dac/mcp4728.c new file +10 -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;

FIELD_PREP()

drivers/iio/dac/mcp4728.c line 196

Quoted 2 lines

drivers/iio/dac/mcp4728.c new file +2 -0

+	outbuf[1] |= ch->dac_value >> 8;
+	outbuf[2] = ch->dac_value & 0xff;

put_unaligned_be16()

Quoted 53 lines

drivers/iio/dac/mcp4728.c new file +53 -0

+		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

drivers/iio/dac/mcp4728.c line 262

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+		ret = mcp4728_resume(&data->client->dev);
+
+	if (ret < 0)
+		return ret;
+
+	return len;

Don't support custom powering down. If this is needed it should be using the existing power management flows.

Might have been more interesting if it were per channel, but it's not. (and I have no idea off the top of my head on how we would deal with it if it were per channel).

Quoted 16 lines

drivers/iio/dac/mcp4728.c new file +15 -0

+				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,

drivers/iio/dac/mcp4728.c line 530

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+			"failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
+			ret);
+		return -EIO;
+	}
+
+	for (i = 0; i < MCP4728_N_CHANNELS; i++) {

Unusual to read back existing values from the device rather than resetting it into a clean state. What is your reasoning?

Hi Jonathan,

thank you for your first review.

See below.

Quoted 12 lines · Jonathan Cameron

drivers/iio/dac/mcp4728.c line 194

Quoted 10 lines

drivers/iio/dac/mcp4728.c new file +10 -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;  

FIELD_PREP()

Quoted 4 lines

drivers/iio/dac/mcp4728.c line 196

Quoted 2 lines

drivers/iio/dac/mcp4728.c new file +2 -0

+	outbuf[1] |= ch->dac_value >> 8;
+	outbuf[2] = ch->dac_value & 0xff;  

put_unaligned_be16()

outbuf[1] contains other data (gain mode, powerdown  mode, etc) in addition to dac value. Using put_unaligned_be16 will probably reset them.

Something like this could be the solution?

#defineMCP4728_DAC_H_FIELD GENMASK(3, 0)

#defineMCP4728_DAC_L_FIELD GENMASK(7, 0)

...

outbuf[1] |= FIELD_PREP(MCP4728_DAC_H_FIELD, ch->dac_value>> 8);

outbuf[2] = FIELD_PREP(MCP4728_DAC_L_FIELD, ch->dac_value);

Quoted 55 lines

Quoted 55 lines

drivers/iio/dac/mcp4728.c new file +53 -0

+		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

Quoted 8 lines

drivers/iio/dac/mcp4728.c line 262

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+		ret = mcp4728_resume(&data->client->dev);
+
+	if (ret < 0)
+		return ret;
+
+	return len;  

Don't support custom powering down. If this is needed it should be using the existing power management flows.

Could you explain better? I have extended customized powering down because I took as reference  the driver mcp4725.c and  I extended to multichannel.

How should I change it?

Quoted 4 lines

Might have been more interesting if it were per channel, but it's not. (and I have no idea off the top of my head on how we would deal with it if it were per channel).

MCP4728 can handle power down per channel...

There are two bits per each channel the could be

00 => No Power Down

01 => 1kohm_to_gnd

10 => 100kohm_to_gnd

11 => 500kohm_to_gnd

Quoted 16 lines

Quoted 16 lines

drivers/iio/dac/mcp4728.c new file +15 -0

+				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,

Quoted 8 lines

drivers/iio/dac/mcp4728.c line 530

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+			"failed to read mcp4728 conf. Wrong Response Len ret=%d\n",
+			ret);
+		return -EIO;
+	}
+
+	for (i = 0; i < MCP4728_N_CHANNELS; i++) {  

Unusual to read back existing values from the device rather than resetting it into a clean state. What is your reasoning?

MCP4728 has an EEPROM memory.

Under the reset conditions, the device uploads the EEPROM data into both of the DAC input and output registers simultaneously.

My reasoning was that the driver syncs with device at probe time and let the user change each channel parameters via iio_chan_spec_ext_info.

Thank you again.

        Andrea

Quoted 21 lines

drivers/iio/dac/mcp4728.c new file +21 -0

+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;

drivers/iio/dac/mcp4728.c line 299

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+
+	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);

Even better if you don't present the option at all and wrap it up in the standard ABI of _scale

Hi Jonathan,

Quoted 21 lines · Jonathan Cameron

Quoted 21 lines

drivers/iio/dac/mcp4728.c new file +21 -0

+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;

Quoted 9 lines

drivers/iio/dac/mcp4728.c line 299

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+
+	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);  

Even better if you don't present the option at all and wrap it up in the standard ABI of _scale

I think that the solution could be:

- Removing custom ABI (vref/gain)

- Initialize them at device tree level using two 4-elements arrays.

- Finally using the same approach of https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/mcp4725.c#L462 where after having  synced current parameters stored in EEPROM they are updated with the ones specified in dts.

Best regards

                       Andrea

Quoted 11 lines

Quoted 9 lines

drivers/iio/dac/mcp4728.c line 299

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+
+	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);  

Even better if you don't present the option at all and wrap it up in the standard ABI of _scale

I think that the solution could be:

Quoted 3 lines

- Removing custom ABI (vref/gain)

- Initialize them at device tree level using two 4-elements arrays.

If doing with device tree, they should reflect something that is a characteristic of how the chips is wired up. So you would need to explain why that is the case here.

However, I'm still not understanding why _SCALE is not appropriate here. We have a small set of options with well defined scales.

Quoted 17 lines · Jonathan Cameron

Quoted 9 lines

Quoted 9 lines

drivers/iio/dac/mcp4728.c line 299

Quoted 6 lines

drivers/iio/dac/mcp4728.c new file +6 -0

+
+	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);    

Even better if you don't present the option at all and wrap it up in the standard ABI of _scale

Quoted 5 lines

I think that the solution could be:

- Removing custom ABI (vref/gain)

- Initialize them at device tree level using two 4-elements arrays.

If doing with device tree, they should reflect something that is a characteristic of how the chips is wired up. So you would need to explain why that is the case here.

Quoted 2 lines

However, I'm still not understanding why _SCALE is not appropriate here. We have a small set of options with well defined scales.

SCALE is appropriate. I didn't know that scale_available was a standard ABI.

I will follow  the implementation done n https://github.com/torvalds/linux/blob/c2782531397f5cb19ca3f8f9c17727f1cdf5bee8/drivers/iio/dac/ad5592r-base.c#L487