[PATCH v3 0/2] add MCP4728 I2C DAC driver​

[PATCH v3 0/2] add MCP4728 I2C DAC driver​

[Andrea Collamati]

Changes v2->v3:
- fix wrong i2c_device_id array indentation
- removed double blank line in Kconfig
- added description in dt-bindings
- use uppercase letters for device name

Changes v1->v2:
- fix mcp4728_remove prototype
- improve indentation
- various fixes suggested by checkscript.pl
- removed unused of_device_id.data field
- removed unuseful mcp4728_data.id field
- various fixes suggested by dt_binding_check

Andrea Collamati (2):
  dt-bindings: iio: dac: add mcp4728.yaml
  iio: add MCP4728 I2C DAC driver

 .../bindings/iio/dac/microchip,mcp4728.yaml   |  48 ++
 drivers/iio/dac/Kconfig                       |  11 +
 drivers/iio/dac/Makefile                      |   1 +
 drivers/iio/dac/mcp4728.c                     | 638 ++++++++++++++++++
 4 files changed, 698 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
 create mode 100644 drivers/iio/dac/mcp4728.c

-- 
2.34.1

[PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Andrea Collamati]

Add documentation for MCP4728

Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
---
 .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml

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..6fd9be076245
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
@@ -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 <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@60 {
+            compatible = "microchip,mcp4728";
+            reg = <0x60>;
+            vdd-supply = <&vdac_vdd>;
+        };
+    };
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

Hey Andrea,

On Thu, Jul 20, 2023 at 05:40:02PM +0200, Andrea Collamati wrote:
> Add documentation for MCP4728
> 
> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
> ---
>  .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> 
> 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..6fd9be076245
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> @@ -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 <andrea.collamati@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp4728

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

Otherwise, this looks good to me.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Despite the email address, I have no knowledge of the hardware in
question, I'm just reviewing the binding.

Thanks,
Conor.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Andrea Collamati]

Hi Conor,

On 7/20/23 19:01, Conor Dooley wrote:
>> Add documentation for MCP4728
>>
>> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
>> ---
>>  .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>>
>> 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..6fd9be076245
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>> @@ -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 <andrea.collamati@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - microchip,mcp4728
> This can just be
> compatible:
>   const: microchip,mcp47288
> since you only have a single item in your enum.

I will in include in v4.

Thank you.

               Andrea

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Krzysztof Kozlowski]

On 20/07/2023 17:40, Andrea Collamati wrote:
> Add documentation for MCP4728
> 
> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
> ---
>  .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> 
> 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..6fd9be076245
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> @@ -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 <andrea.collamati@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp4728

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.

Thank you.

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

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.

Thank you.


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Krzysztof Kozlowski]

On 21/07/2023 10:21, Krzysztof Kozlowski wrote:
>> +  - |
>> +    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.

Damn, this is my third comment about the same. Here was second:
https://lore.kernel.org/all/5e5d1a1e-f106-9dd6-c19e-f933e8e70dd4@kernel.org/

so you nicely ignore feedback. NAK.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Andrea Collamati]

On 7/21/23 10:22, Krzysztof Kozlowski wrote:
> On 21/07/2023 10:21, Krzysztof Kozlowski wrote:
>>> +  - |
>>> +    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.

Sorry, you are right. I missed to change the node name.

{

        #address-cells = <1>;
        #size-cells = <0>;

        dac@60 {

could be ok?


Thank you

       Andrea

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Andrea Collamati]

Hi Krzysztof,

On 7/21/23 10:21, Krzysztof Kozlowski wrote:
>> Add documentation for MCP4728
>>
>> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
>> ---
>>  .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>>
>> 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..6fd9be076245
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>> @@ -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 <andrea.collamati@gmail.com>
>> +
>> +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?

Re: [PATCH v3 1/2] dt-bindings: iio: dac: add mcp4728.yaml

[Krzysztof Kozlowski]

On 21/07/2023 13:58, Andrea Collamati wrote:
> Hi Krzysztof,
> 
> On 7/21/23 10:21, Krzysztof Kozlowski wrote:
>>> Add documentation for MCP4728
>>>
>>> Signed-off-by: Andrea Collamati <andrea.collamati@gmail.com>
>>> ---
>>>  .../bindings/iio/dac/microchip,mcp4728.yaml   | 48 +++++++++++++++++++
>>>  1 file changed, 48 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>>>
>>> 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..6fd9be076245
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>>> @@ -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 <andrea.collamati@gmail.com>
>>> +
>>> +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?

You forgot to add blank line. Open example-schema and compare.

Also, you had white-space errors. Editors should show it to you. Git
maybe as well.

Best regards,
Krzysztof

[PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Andrea Collamati]

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 <andrea.collamati@gmail.com>
---
 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

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 3acd9c3f388e..93b8be183de6 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -389,6 +389,17 @@ 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 addd97a78838..5b2bac900d5a 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MAX5522) += max5522.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..39237dba27ba
--- /dev/null
+++ b/drivers/iio/dac/mcp4728.c
@@ -0,0 +1,638 @@
+// 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)
+#define MCP4728_WRITE_EEPROM_LEN (1 + MCP4728_N_CHANNELS * 2)
+
+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;
+	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[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[] = {
+	&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;
+	}
+	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;
+	}
+	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 = 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);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	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 void 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);
+}
+
+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 <andrea.collamati@gmail.com>");
+MODULE_DESCRIPTION("MCP4728 12-bit DAC");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Jonathan Cameron]

On Thu, 20 Jul 2023 17:40:03 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:

> 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 <andrea.collamati@gmail.com>
Hi Andrea,

Given the question was raised about similar parts, I took a quick look
and to me this look different enough from parts already supported that
a new driver is reasonable.

Biggest thing to address here is the custom ABI.
1) I tend to resist that wherever possible - it will be little used (because it's custom)
   and rather defeats the point of a unfied subsystem.
2) It's not documented ;) Without docs, very low chance of acceptance or even
   good review.

Comments inline.

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


> diff --git a/drivers/iio/dac/mcp4728.c b/drivers/iio/dac/mcp4728.c
> new file mode 100644
> index 000000000000..39237dba27ba
> --- /dev/null
> +++ b/drivers/iio/dac/mcp4728.c
> @@ -0,0 +1,638 @@
> +// 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>

Prefer alphabetical order.  The groups you have are fine.


> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define MCP4728_DRV_NAME "mcp4728"

Normally better to just put this inline.  It's not repeated much.

> +
> +#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
All these should be just the masks then use FIELD_PREP() / FIELD_GET()
> +
> +#define MCP4728_MW_CMD 0x08 // Multiwrite Command
> +#define MCP4728_SW_CMD 0x0A // Sequential Write Command (include eeprom)

/* */ comments only for consistency with rest of the drivers in IIO

> +
> +#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;
> +	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
FIELD_PREP()
> +
> +	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;

FIELD_PREP() etc in similar places throughout.

> +		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[] = {
> +	&iio_dev_attr_store_eeprom.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group mcp4728_attribute_group = {
> +	.attrs = mcp4728_attributes,
> +};
> +
> +enum chip_id {
> +	MCP4728,

Don't introduce any infrastructure for multiple support devices until
you add support for a second device.  Right now it is just nose.

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

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

FIELD_PREP()

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

put_unaligned_be16()

> +
> +	ret = i2c_master_send(data->client, outbuf, 3);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret != 3)
> +		return -EIO;
> +	else

Drop the else - this is the non error condition.

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

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).

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

No c++ comments in IIO (other than for the spdx thing)

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

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

> +		data->channel_data[chan->channel].g_mode = MCP4728_GAIN_X1;
> +	}
> +
> +	ret = mcp4728_program_channel_cfg(chan->channel, indio_dev);
> +
> +	return ret;

return mcp...

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

Map this to _SCALE probably as a set of values combined with the
reference select.

Also, use numbers for ABI where possible. x2 is a pain for software
library to mess around with.

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

Custom ABI is basically ABI no software will use.  Also to even start attempting
to get that accepted for a driver, we need documentation in
Documentation/ABI/testing/sysfs-bus-iio-*

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

This looks like a linear transformation.  If so that's a job for userspace and
you should expose the raw channel values + the scaling.

> +}
> +
> +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;
> +	}
> +	return err;
> +}
> +
> +static int mcp4728_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

I don't feel strongly about this, but chances are this will get
simplified to not bounce from dev to i2c client and back again.
dev_get_drvdata(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;

Generally if an error occured, stop poking the device.  So error out
immediately.  I can see the logic in trying to continue, but an error
on a device like this normally indicates something is going very wrong
so muddling on won't succeed.

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

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

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

Use FIELD_GET()

> +
> +		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);
That's noise that you should be able to get by directly reading the relevant attributes
or is detail that userspace should not need to know about.
You can rely on dynamic debug though and make that dev_dbg() so it is available if needed.

> +	}
> +
> +	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);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	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;

As mentioned below - a devm_add_action_or_reset() here to deal with
the regulator would be good.  Also, regulator_enable() is side effect
free on error so you should not be disabling in this error path.

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

Use devm_add_action_or_reset() + a suitable callback to turn of the
regulator, then you can use devm for the device register as well
and no need for a remove function or error handlers in probe()

> +}
> +
> +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 <andrea.collamati@gmail.com>");
> +MODULE_DESCRIPTION("MCP4728 12-bit DAC");
> +MODULE_LICENSE("GPL");

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Andrea Collamati]

Hi Jonathan,

thank you for your first review.

See below.

On 7/20/23 21:13, Jonathan Cameron wrote:

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


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

> 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

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

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Jonathan Cameron]

On Fri, 21 Jul 2023 21:47:37 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:

> Hi Jonathan,
> 
> thank you for your first review.
> 
> See below.
> 
> On 7/20/23 21:13, Jonathan Cameron wrote:
> 
> >> +
> >> +	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()
> >  
> >> +	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.

Ah. I'd missed the |= because you then didn't mask the dac_value.

> 
> Something like this could be the solution?
> 
> #defineMCP4728_DAC_H_FIELD GENMASK(3, 0)
> 
> #defineMCP4728_DAC_L_FIELD GENMASK(7, 0)

Call them _MASK or _MSK rather than field.
I think that is much more common naming.

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

That's better.

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

Ok. I'd missed that we had an existing driver doing this and indeed
we have documented it.  Fair enough - I must have been persuaded in the past
and then forgotten about it :)


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

Ok.  Good to support that rather than a full power down.

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

Ok - this is fine if it's reading back from EEPROM.
Please add a comment on that though because as demonstrated above
I'm forgetful and may wonder why it was done like this in the future.

Jonathan


> 
> 
> Thank you again.
> 
>         Andrea
> 
> 

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Andrea Collamati]

Hi Jonathan,

On 7/20/23 21:13, Jonathan Cameron wrote:
>> +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);
> 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

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Jonathan Cameron]

On Sun, 23 Jul 2023 07:09:11 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:

> Hi Jonathan,
> 
> On 7/20/23 21:13, Jonathan Cameron wrote:
> >> +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);  
> > 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.

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.

> 
> - 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.
> 
From a quick look those are not coming from DTS and are not in the dt-binding for that
device. The parameters only come from legacy platform data (or the wild west of early IIO).

Only the presence of vref and whether to buffer it are in DT binding
https://elixir.bootlin.com/linux/latest/source/drivers/iio/dac/mcp4725.c#L373

We should probably rip the remaining platform data based probe paths out of drivers
sometime soon. No one uses them any more (there are better paths even if people are
using non DT based board files) and they can mislead because the rules weren't
as tight as they are for what goes in a DT binding.

Jonathan


> 
> Best regards
> 
>                        Andrea
> 
> 

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Andrea Collamati]

On 7/23/23 13:41, Jonathan Cameron wrote:
>>>> +
>>>> +	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.
> 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.

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

Re: [PATCH v3 2/2] iio: add MCP4728 I2C DAC driver

[Jonathan Cameron]

On Sun, 23 Jul 2023 14:15:10 +0200
Andrea Collamati <andrea.collamati@gmail.com> wrote:

> On 7/23/23 13:41, Jonathan Cameron wrote:
> >>>> +
> >>>> +	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.  
> > 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.  
> 
> 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

That's long out of date wrt to best practice.  Instead follow.
drivers/iio/dac/ad7293.c or any of the other drivers that set
.info_mask_shared_by_*_available = BIT(IIO_CHAN_INFO_SCALE)
and provide the read_avail() callback.

The advantage that brings is that consumer drivers can then
access this information much more simply.

One day we will hopefully finish converting old drivers over to
newer interfaces, but it's slow progress!

Jonathan