[PATCH 0/3] iio: dac: mcp4725: add vref selection and devicetree support

[PATCH 0/3] iio: dac: mcp4725: add vref selection and devicetree support

[Tomas Novotny]

Hi all,

here is a small series to add voltage reference selection and devicetree support
to mcp4725/6. This series was tested on mcp4726 with devicetree boot.

Tomas Novotny (3):
  iio: dac: mcp4725: support voltage reference selection
  Documentation: dt: iio: add mcp4725/6 dac device binding
  iio: dac: mcp4725: add devicetree support

 .../devicetree/bindings/iio/dac/mcp4725.txt        |  28 ++++++
 drivers/iio/dac/mcp4725.c                          | 108 ++++++++++++++++++---
 include/linux/iio/dac/mcp4725.h                    |   1 +
 3 files changed, 126 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] iio: dac: mcp4725: support voltage reference selection

[Tomas Novotny]

MCP47x6 chip supports selection of a voltage reference (VDD, VREF buffered
or unbuffered). MCP4725 doesn't have this feature thus the setting is
ignored and user is warned if there is an attempt to set a value other than
zero.

The setting is stored only in the volatile memory of the chip. You need to
manually store it to the EEPROM of the chip via 'store_eeprom' sysfs entry.

Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
---
 drivers/iio/dac/mcp4725.c       | 45 +++++++++++++++++++++++++++++++++++++----
 include/linux/iio/dac/mcp4725.h |  1 +
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
index cca935c..f8e54f1 100644
--- a/drivers/iio/dac/mcp4725.c
+++ b/drivers/iio/dac/mcp4725.c
@@ -28,7 +28,9 @@
 
 struct mcp4725_data {
 	struct i2c_client *client;
+	int id;
 	u16 vref_mv;
+	unsigned vref_mode;
 	u16 dac_value;
 	bool powerdown;
 	unsigned powerdown_mode;
@@ -86,6 +88,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
 		return 0;
 
 	inoutbuf[0] = 0x60; /* write EEPROM */
+	inoutbuf[0] |= data->vref_mode << 3;
 	inoutbuf[1] = data->dac_value >> 4;
 	inoutbuf[2] = (data->dac_value & 0xf) << 4;
 
@@ -329,8 +332,10 @@ static int mcp4725_probe(struct i2c_client *client,
 	struct mcp4725_data *data;
 	struct iio_dev *indio_dev;
 	struct mcp4725_platform_data *platform_data = client->dev.platform_data;
-	u8 inbuf[3];
+	u8 inoutbuf[3];
 	u8 pd;
+	u8 vref;
+	int ret;
 	int err;
 
 	if (!platform_data || !platform_data->vref_mv) {
@@ -344,6 +349,17 @@ static int mcp4725_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	data->id = id->driver_data;
+
+	if (data->id == MCP4725 && platform_data->vref_mode > 0) {
+		dev_warn(&client->dev, "vref mode is unavailable on MCP4725, ignoring");
+		platform_data->vref_mode = 0;
+	}
+
+	if (data->id == MCP4726 && platform_data->vref_mode > 3) {
+		dev_err(&client->dev, "vref mode is out of range");
+		return -EINVAL;
+	}
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = id->name;
@@ -353,17 +369,38 @@ static int mcp4725_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	data->vref_mv = platform_data->vref_mv;
+	data->vref_mode = platform_data->vref_mode;
 
 	/* read current DAC value */
-	err = i2c_master_recv(client, inbuf, 3);
+	err = i2c_master_recv(client, inoutbuf, 3);
 	if (err < 0) {
 		dev_err(&client->dev, "failed to read DAC value");
 		return err;
 	}
-	pd = (inbuf[0] >> 1) & 0x3;
+	pd = (inoutbuf[0] >> 1) & 0x3;
 	data->powerdown = pd > 0 ? true : false;
 	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
-	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
+	data->dac_value = (inoutbuf[1] << 4) | (inoutbuf[2] >> 4);
+	vref = inoutbuf[0] >> 3 & 0x3;
+
+	if (data->id == MCP4726 && vref != data->vref_mode) {
+		dev_info(&client->dev,
+			"vref_mode differs (conf: %d, eeprom: %d), setting %d",
+			data->vref_mode, vref, data->vref_mode);
+
+		inoutbuf[0] = 0x40;
+		inoutbuf[0] |= data->vref_mode << 3;
+		if (data->powerdown)
+			inoutbuf[0] |= data->powerdown << 1;
+		inoutbuf[1] = data->dac_value >> 4;
+		inoutbuf[2] = (data->dac_value & 0xf) << 4;
+
+		ret = i2c_master_send(data->client, inoutbuf, 3);
+		if (ret < 0)
+			return ret;
+		else if (ret != 3)
+			return -EIO;
+	}
 
 	return iio_device_register(indio_dev);
 }
diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
index 91530e6..370022b 100644
--- a/include/linux/iio/dac/mcp4725.h
+++ b/include/linux/iio/dac/mcp4725.h
@@ -11,6 +11,7 @@
 
 struct mcp4725_platform_data {
 	u16 vref_mv;
+	unsigned vref_mode;
 };
 
 #endif /* IIO_DAC_MCP4725_H_ */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] Documentation: dt: iio: add mcp4725/6 dac device binding

[Tomas Novotny]

Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
---
 .../devicetree/bindings/iio/dac/mcp4725.txt        | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt

diff --git a/Documentation/devicetree/bindings/iio/dac/mcp4725.txt b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
new file mode 100644
index 0000000..69c9462
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
@@ -0,0 +1,28 @@
+Microchpip mcp4725 and mcp4726 DAC device driver
+
+Required properties:
+	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
+	- reg: Should contain the DAC I2C address
+	- vref-millivolt: Value of the reference voltage
+
+Optional properties:
+	- vref-mode: Reference voltage selection. It is available only on
+	  mcp4726. Valid values are:
+		- 0, 1: Vdd pin voltage (unbuffered)
+		- 2: Vref pin voltage unbuffered
+		- 3: Vref pin voltage internally buffered
+
+Examples:
+
+        mcp4725@60 {
+                compatible = "microchip,mcp4725";
+                reg = <0x60>;
+                vref-millivolt = <3300>;
+        };
+
+        mcp4726@60 {
+                compatible = "microchip,mcp4726";
+                reg = <0x60>;
+                vref-mode = <2>;
+                vref-millivolt = <2500>;
+        };
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] iio: dac: mcp4725: add devicetree support

[Tomas Novotny]

Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
---
 drivers/iio/dac/mcp4725.c | 71 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
index f8e54f1..74ffe56 100644
--- a/drivers/iio/dac/mcp4725.c
+++ b/drivers/iio/dac/mcp4725.c
@@ -18,6 +18,7 @@
 #include <linux/i2c.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -326,23 +327,57 @@ static const struct iio_info mcp4725_info = {
 	.driver_module = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static int mcp472x_probe_dt(struct device *dev,
+			    struct mcp4725_platform_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+	struct mcp4725_data *data = iio_priv(i2c_get_clientdata(
+		to_i2c_client(dev)));
+	int err;
+	unsigned vref_mv;
+	unsigned vref_mode = 0;
+
+	if (!np)
+		return -ENODEV;
+
+	err = of_property_read_u32(np, "vref-millivolt", &vref_mv);
+	if (err) {
+		dev_err(dev, "missing or invalid vref-millivolt devicetree data");
+		return err;
+	}
+
+	err = of_property_read_u32(np, "vref-mode", &vref_mode);
+	if (data->id == MCP4725 && !err) {
+		dev_warn(dev, "vref-mode is unavailable on MCP4725");
+		vref_mode = 0;
+	}
+
+	pdata->vref_mv = vref_mv;
+	pdata->vref_mode = vref_mode;
+
+	return 0;
+}
+#else
+static int mcp472x_probe_dt(struct device *dev,
+			    struct mcp4725_platform_data *platform_data)
+{
+	return -ENODEV;
+}
+#endif
+
 static int mcp4725_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
 	struct mcp4725_data *data;
 	struct iio_dev *indio_dev;
-	struct mcp4725_platform_data *platform_data = client->dev.platform_data;
+	struct mcp4725_platform_data *pdata = dev_get_platdata(&client->dev);
 	u8 inoutbuf[3];
 	u8 pd;
 	u8 vref;
 	int ret;
 	int err;
 
-	if (!platform_data || !platform_data->vref_mv) {
-		dev_err(&client->dev, "invalid platform data");
-		return -EINVAL;
-	}
-
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (indio_dev == NULL)
 		return -ENOMEM;
@@ -351,12 +386,23 @@ static int mcp4725_probe(struct i2c_client *client,
 	data->client = client;
 	data->id = id->driver_data;
 
-	if (data->id == MCP4725 && platform_data->vref_mode > 0) {
+	if (!dev_get_platdata(&client->dev)) {
+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+		err = mcp472x_probe_dt(&client->dev, pdata);
+		if (err) {
+			dev_err(&client->dev, "invalid platform or devicetree data");
+			return err;
+		}
+	}
+
+	if (data->id == MCP4725 && pdata->vref_mode > 0) {
 		dev_warn(&client->dev, "vref mode is unavailable on MCP4725, ignoring");
-		platform_data->vref_mode = 0;
+		pdata->vref_mode = 0;
 	}
 
-	if (data->id == MCP4726 && platform_data->vref_mode > 3) {
+	if (data->id == MCP4726 && pdata->vref_mode > 3) {
 		dev_err(&client->dev, "vref mode is out of range");
 		return -EINVAL;
 	}
@@ -368,8 +414,8 @@ static int mcp4725_probe(struct i2c_client *client,
 	indio_dev->num_channels = 1;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	data->vref_mv = platform_data->vref_mv;
-	data->vref_mode = platform_data->vref_mode;
+	data->vref_mv = pdata->vref_mv;
+	data->vref_mode = pdata->vref_mode;
 
 	/* read current DAC value */
 	err = i2c_master_recv(client, inoutbuf, 3);
@@ -402,6 +448,9 @@ static int mcp4725_probe(struct i2c_client *client,
 			return -EIO;
 	}
 
+	if (!dev_get_platdata(&client->dev))
+		devm_kfree(&client->dev, pdata);
+
 	return iio_device_register(indio_dev);
 }
 
-- 
2.1.4

Re: [PATCH 2/3] Documentation: dt: iio: add mcp4725/6 dac device binding

[Jonathan Cameron]

On 30/09/16 14:19, Tomas Novotny wrote:
> Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/dac/mcp4725.txt        | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/mcp4725.txt b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> new file mode 100644
> index 0000000..69c9462
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> @@ -0,0 +1,28 @@
> +Microchpip mcp4725 and mcp4726 DAC device driver
> +
> +Required properties:
> +	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
> +	- reg: Should contain the DAC I2C address
> +	- vref-millivolt: Value of the reference voltage
> +
> +Optional properties:
> +	- vref-mode: Reference voltage selection. It is available only on
Not a generic attribute, so wants to have a vendor prefix on it.
> +	  mcp4726. Valid values are:
> +		- 0, 1: Vdd pin voltage (unbuffered)
This should not be a direct mapping of the register values but rather
a means to control the setting.  Having two values mapping to the
same thing makes no sense.

Would also be odd to specify a vref and then not use it.  So you could
infer this first option.
> +		- 2: Vref pin voltage unbuffered
> +		- 3: Vref pin voltage internally buffered
Having inferred the first option then this becomes control of whether it
is buffered  or not. So could be named appropriately to cover that.
Might be nice to have a little note here on why you might want the
buffer or not..
> +
> +Examples:
> +
> +        mcp4725@60 {
> +                compatible = "microchip,mcp4725";
> +                reg = <0x60>;
> +                vref-millivolt = <3300>;
> +        };
> +
> +        mcp4726@60 {
> +                compatible = "microchip,mcp4726";
> +                reg = <0x60>;
> +                vref-mode = <2>;
> +                vref-millivolt = <2500>;
> +        };
> 

Re: [PATCH 2/3] Documentation: dt: iio: add mcp4725/6 dac device binding

[Jonathan Cameron]

On 01/10/16 12:15, Jonathan Cameron wrote:
> On 30/09/16 14:19, Tomas Novotny wrote:
>> Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
>> ---
>>  .../devicetree/bindings/iio/dac/mcp4725.txt        | 28 ++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/dac/mcp4725.txt b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
>> new file mode 100644
>> index 0000000..69c9462
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
>> @@ -0,0 +1,28 @@
>> +Microchpip mcp4725 and mcp4726 DAC device driver
>> +
>> +Required properties:
>> +	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
>> +	- reg: Should contain the DAC I2C address
>> +	- vref-millivolt: Value of the reference voltage
Sorry dozing this morning so missed this until looking at the code.

Use the regulator framework to supply two regulators:

vref-suppy and vdd-supply
first one optional.

Then query them in the driver.  Much more flexible than just hard coding
a value here.  Entirely possible to have a variable regulator used for
the reference.
>> +
>> +Optional properties:
>> +	- vref-mode: Reference voltage selection. It is available only on
> Not a generic attribute, so wants to have a vendor prefix on it.
>> +	  mcp4726. Valid values are:
>> +		- 0, 1: Vdd pin voltage (unbuffered)
> This should not be a direct mapping of the register values but rather
> a means to control the setting.  Having two values mapping to the
> same thing makes no sense.
> 
> Would also be odd to specify a vref and then not use it.  So you could
> infer this first option.
>> +		- 2: Vref pin voltage unbuffered
>> +		- 3: Vref pin voltage internally buffered
> Having inferred the first option then this becomes control of whether it
> is buffered  or not. So could be named appropriately to cover that.
> Might be nice to have a little note here on why you might want the
> buffer or not..
>> +
>> +Examples:
>> +
>> +        mcp4725@60 {
>> +                compatible = "microchip,mcp4725";
>> +                reg = <0x60>;
>> +                vref-millivolt = <3300>;
>> +        };
>> +
>> +        mcp4726@60 {
>> +                compatible = "microchip,mcp4726";
>> +                reg = <0x60>;
>> +                vref-mode = <2>;
>> +                vref-millivolt = <2500>;

>> +        };
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/3] iio: dac: mcp4725: support voltage reference selection

[Jonathan Cameron]

On 30/09/16 14:19, Tomas Novotny wrote:
> MCP47x6 chip supports selection of a voltage reference (VDD, VREF buffered
> or unbuffered). MCP4725 doesn't have this feature thus the setting is
> ignored and user is warned if there is an attempt to set a value other than
> zero.
> 
> The setting is stored only in the volatile memory of the chip. You need to
> manually store it to the EEPROM of the chip via 'store_eeprom' sysfs entry.
> 
> Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
> ---
>  drivers/iio/dac/mcp4725.c       | 45 +++++++++++++++++++++++++++++++++++++----
>  include/linux/iio/dac/mcp4725.h |  1 +
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> index cca935c..f8e54f1 100644
> --- a/drivers/iio/dac/mcp4725.c
> +++ b/drivers/iio/dac/mcp4725.c
> @@ -28,7 +28,9 @@
>  
>  struct mcp4725_data {
>  	struct i2c_client *client;
> +	int id;
>  	u16 vref_mv;
> +	unsigned vref_mode;
>  	u16 dac_value;
>  	bool powerdown;
>  	unsigned powerdown_mode;
> @@ -86,6 +88,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
>  		return 0;
>  
>  	inoutbuf[0] = 0x60; /* write EEPROM */
> +	inoutbuf[0] |= data->vref_mode << 3;
>  	inoutbuf[1] = data->dac_value >> 4;
>  	inoutbuf[2] = (data->dac_value & 0xf) << 4;
>  
> @@ -329,8 +332,10 @@ static int mcp4725_probe(struct i2c_client *client,
>  	struct mcp4725_data *data;
>  	struct iio_dev *indio_dev;
>  	struct mcp4725_platform_data *platform_data = client->dev.platform_data;
> -	u8 inbuf[3];
> +	u8 inoutbuf[3];
>  	u8 pd;
> +	u8 vref;
> +	int ret;
>  	int err;
>  
>  	if (!platform_data || !platform_data->vref_mv) {
> @@ -344,6 +349,17 @@ static int mcp4725_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> +	data->id = id->driver_data;
> +
> +	if (data->id == MCP4725 && platform_data->vref_mode > 0) {
> +		dev_warn(&client->dev, "vref mode is unavailable on MCP4725, ignoring");
> +		platform_data->vref_mode = 0;
> +	}
> +
> +	if (data->id == MCP4726 && platform_data->vref_mode > 3) {
> +		dev_err(&client->dev, "vref mode is out of range");
> +		return -EINVAL;
> +	}
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->name = id->name;
> @@ -353,17 +369,38 @@ static int mcp4725_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	data->vref_mv = platform_data->vref_mv;
> +	data->vref_mode = platform_data->vref_mode;
>  
>  	/* read current DAC value */
> -	err = i2c_master_recv(client, inbuf, 3);
> +	err = i2c_master_recv(client, inoutbuf, 3);
>  	if (err < 0) {
>  		dev_err(&client->dev, "failed to read DAC value");
>  		return err;
>  	}
> -	pd = (inbuf[0] >> 1) & 0x3;
> +	pd = (inoutbuf[0] >> 1) & 0x3;
>  	data->powerdown = pd > 0 ? true : false;
>  	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
> -	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
> +	data->dac_value = (inoutbuf[1] << 4) | (inoutbuf[2] >> 4);
> +	vref = inoutbuf[0] >> 3 & 0x3;
> +
> +	if (data->id == MCP4726 && vref != data->vref_mode) {
> +		dev_info(&client->dev,
> +			"vref_mode differs (conf: %d, eeprom: %d), setting %d",
> +			data->vref_mode, vref, data->vref_mode);
> +
> +		inoutbuf[0] = 0x40;
> +		inoutbuf[0] |= data->vref_mode << 3;
> +		if (data->powerdown)
> +			inoutbuf[0] |= data->powerdown << 1;
> +		inoutbuf[1] = data->dac_value >> 4;
> +		inoutbuf[2] = (data->dac_value & 0xf) << 4;
> +
> +		ret = i2c_master_send(data->client, inoutbuf, 3);
> +		if (ret < 0)
> +			return ret;
> +		else if (ret != 3)
> +			return -EIO;
> +	}
>  
>  	return iio_device_register(indio_dev);
>  }
> diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
> index 91530e6..370022b 100644
> --- a/include/linux/iio/dac/mcp4725.h
> +++ b/include/linux/iio/dac/mcp4725.h
> @@ -11,6 +11,7 @@
>  
>  struct mcp4725_platform_data {
>  	u16 vref_mv;
> +	unsigned vref_mode;
Hmm. Should probably move the platform support over to using regulators.
This is a horible bit of legacy code.

Jonathan
>  };
>  
>  #endif /* IIO_DAC_MCP4725_H_ */
> 

Re: [PATCH 2/3] Documentation: dt: iio: add mcp4725/6 dac device binding

[Peter Meerwald-Stadler]

On Sat, 1 Oct 2016, Jonathan Cameron wrote:

some more comments

> On 30/09/16 14:19, Tomas Novotny wrote:
> > Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
> > ---
> >  .../devicetree/bindings/iio/dac/mcp4725.txt        | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/dac/mcp4725.txt b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> > new file mode 100644
> > index 0000000..69c9462
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> > @@ -0,0 +1,28 @@
> > +Microchpip mcp4725 and mcp4726 DAC device driver

Microchip

> > +
> > +Required properties:
> > +	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
> > +	- reg: Should contain the DAC I2C address
> > +	- vref-millivolt: Value of the reference voltage
> > +
> > +Optional properties:
> > +	- vref-mode: Reference voltage selection. It is available only on
> Not a generic attribute, so wants to have a vendor prefix on it.
> > +	  mcp4726. Valid values are:
> > +		- 0, 1: Vdd pin voltage (unbuffered)
> This should not be a direct mapping of the register values but rather
> a means to control the setting.  Having two values mapping to the
> same thing makes no sense.
> 
> Would also be odd to specify a vref and then not use it.  So you could
> infer this first option.
> > +		- 2: Vref pin voltage unbuffered
> > +		- 3: Vref pin voltage internally buffered
> Having inferred the first option then this becomes control of whether it
> is buffered  or not. So could be named appropriately to cover that.
> Might be nice to have a little note here on why you might want the
> buffer or not..
> > +
> > +Examples:
> > +
> > +        mcp4725@60 {
> > +                compatible = "microchip,mcp4725";
> > +                reg = <0x60>;
> > +                vref-millivolt = <3300>;
> > +        };
> > +
> > +        mcp4726@60 {
> > +                compatible = "microchip,mcp4726";
> > +                reg = <0x60>;
> > +                vref-mode = <2>;
> > +                vref-millivolt = <2500>;
> > +        };
> > 
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 1/3] iio: dac: mcp4725: support voltage reference selection

[Peter Meerwald-Stadler]

some additional comments

On Sat, 1 Oct 2016, Jonathan Cameron wrote:

> On 30/09/16 14:19, Tomas Novotny wrote:
> > MCP47x6 chip supports selection of a voltage reference (VDD, VREF buffered
> > or unbuffered). MCP4725 doesn't have this feature thus the setting is
> > ignored and user is warned if there is an attempt to set a value other than
> > zero.
> > 
> > The setting is stored only in the volatile memory of the chip. You need to
> > manually store it to the EEPROM of the chip via 'store_eeprom' sysfs entry.
> > 
> > Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
> > ---
> >  drivers/iio/dac/mcp4725.c       | 45 +++++++++++++++++++++++++++++++++++++----
> >  include/linux/iio/dac/mcp4725.h |  1 +
> >  2 files changed, 42 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> > index cca935c..f8e54f1 100644
> > --- a/drivers/iio/dac/mcp4725.c
> > +++ b/drivers/iio/dac/mcp4725.c
> > @@ -28,7 +28,9 @@
> >  
> >  struct mcp4725_data {
> >  	struct i2c_client *client;
> > +	int id;
> >  	u16 vref_mv;
> > +	unsigned vref_mode;
> >  	u16 dac_value;
> >  	bool powerdown;
> >  	unsigned powerdown_mode;
> > @@ -86,6 +88,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
> >  		return 0;
> >  
> >  	inoutbuf[0] = 0x60; /* write EEPROM */
> > +	inoutbuf[0] |= data->vref_mode << 3;
> >  	inoutbuf[1] = data->dac_value >> 4;
> >  	inoutbuf[2] = (data->dac_value & 0xf) << 4;
> >  
> > @@ -329,8 +332,10 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	struct mcp4725_data *data;
> >  	struct iio_dev *indio_dev;
> >  	struct mcp4725_platform_data *platform_data = client->dev.platform_data;
> > -	u8 inbuf[3];
> > +	u8 inoutbuf[3];
> >  	u8 pd;
> > +	u8 vref;
> > +	int ret;
> >  	int err;
> >  
> >  	if (!platform_data || !platform_data->vref_mv) {
> > @@ -344,6 +349,17 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	data = iio_priv(indio_dev);
> >  	i2c_set_clientdata(client, indio_dev);
> >  	data->client = client;
> > +	data->id = id->driver_data;
> > +
> > +	if (data->id == MCP4725 && platform_data->vref_mode > 0) {
> > +		dev_warn(&client->dev, "vref mode is unavailable on MCP4725, ignoring");
> > +		platform_data->vref_mode = 0;
> > +	}
> > +
> > +	if (data->id == MCP4726 && platform_data->vref_mode > 3) {
> > +		dev_err(&client->dev, "vref mode is out of range");
> > +		return -EINVAL;
> > +	}
> >  
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->name = id->name;
> > @@ -353,17 +369,38 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >  	data->vref_mv = platform_data->vref_mv;
> > +	data->vref_mode = platform_data->vref_mode;
> >  
> >  	/* read current DAC value */
> > -	err = i2c_master_recv(client, inbuf, 3);
> > +	err = i2c_master_recv(client, inoutbuf, 3);
> >  	if (err < 0) {
> >  		dev_err(&client->dev, "failed to read DAC value");
> >  		return err;
> >  	}
> > -	pd = (inbuf[0] >> 1) & 0x3;
> > +	pd = (inoutbuf[0] >> 1) & 0x3;
> >  	data->powerdown = pd > 0 ? true : false;
> >  	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
> > -	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
> > +	data->dac_value = (inoutbuf[1] << 4) | (inoutbuf[2] >> 4);
> > +	vref = inoutbuf[0] >> 3 & 0x3;

parenthesis would be good to make the expression above more clear

> > +
> > +	if (data->id == MCP4726 && vref != data->vref_mode) {
> > +		dev_info(&client->dev,
> > +			"vref_mode differs (conf: %d, eeprom: %d), setting %d",
> > +			data->vref_mode, vref, data->vref_mode);

use %u, these are unsigned variables

> > +
> > +		inoutbuf[0] = 0x40;
> > +		inoutbuf[0] |= data->vref_mode << 3;
> > +		if (data->powerdown)
> > +			inoutbuf[0] |= data->powerdown << 1;
> > +		inoutbuf[1] = data->dac_value >> 4;
> > +		inoutbuf[2] = (data->dac_value & 0xf) << 4;
> > +
> > +		ret = i2c_master_send(data->client, inoutbuf, 3);
> > +		if (ret < 0)
> > +			return ret;
> > +		else if (ret != 3)
> > +			return -EIO;
> > +	}
> >  
> >  	return iio_device_register(indio_dev);
> >  }
> > diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
> > index 91530e6..370022b 100644
> > --- a/include/linux/iio/dac/mcp4725.h
> > +++ b/include/linux/iio/dac/mcp4725.h
> > @@ -11,6 +11,7 @@
> >  
> >  struct mcp4725_platform_data {
> >  	u16 vref_mv;
> > +	unsigned vref_mode;
> Hmm. Should probably move the platform support over to using regulators.
> This is a horible bit of legacy code.
> 
> Jonathan
> >  };
> >  
> >  #endif /* IIO_DAC_MCP4725_H_ */
> > 
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 2/3] Documentation: dt: iio: add mcp4725/6 dac device binding

[Tomas Novotny]

Hi Jonathan,

On Sat, 1 Oct 2016 12:21:57 +0100, Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On 01/10/16 12:15, Jonathan Cameron wrote:
> > On 30/09/16 14:19, Tomas Novotny wrote:
> >> Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
> >> ---
> >>  .../devicetree/bindings/iio/dac/mcp4725.txt        | 28 ++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/dac/mcp4725.txt b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> >> new file mode 100644
> >> index 0000000..69c9462
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> >> @@ -0,0 +1,28 @@
> >> +Microchpip mcp4725 and mcp4726 DAC device driver
> >> +
> >> +Required properties:
> >> +	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
> >> +	- reg: Should contain the DAC I2C address
> >> +	- vref-millivolt: Value of the reference voltage
> Sorry dozing this morning so missed this until looking at the code.
> 
> Use the regulator framework to supply two regulators:
> 
> vref-suppy and vdd-supply
> first one optional.
> 
> Then query them in the driver.  Much more flexible than just hard coding
> a value here.  Entirely possible to have a variable regulator used for
> the reference.

I see now that this is a pretty standard approach in the iio/dac. I will do
it in the regulator way. I will send v2 then.

Thanks for the review,

Tomas

> >> +
> >> +Optional properties:
> >> +	- vref-mode: Reference voltage selection. It is available only on
> > Not a generic attribute, so wants to have a vendor prefix on it.
> >> +	  mcp4726. Valid values are:
> >> +		- 0, 1: Vdd pin voltage (unbuffered)
> > This should not be a direct mapping of the register values but rather
> > a means to control the setting.  Having two values mapping to the
> > same thing makes no sense.
> > 
> > Would also be odd to specify a vref and then not use it.  So you could
> > infer this first option.
> >> +		- 2: Vref pin voltage unbuffered
> >> +		- 3: Vref pin voltage internally buffered
> > Having inferred the first option then this becomes control of whether it
> > is buffered  or not. So could be named appropriately to cover that.
> > Might be nice to have a little note here on why you might want the
> > buffer or not..
> >> +
> >> +Examples:
> >> +
> >> +        mcp4725@60 {
> >> +                compatible = "microchip,mcp4725";
> >> +                reg = <0x60>;
> >> +                vref-millivolt = <3300>;
> >> +        };
> >> +
> >> +        mcp4726@60 {
> >> +                compatible = "microchip,mcp4726";
> >> +                reg = <0x60>;
> >> +                vref-mode = <2>;
> >> +                vref-millivolt = <2500>;
> 
> >> +        };
> >>
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] iio: dac: mcp4725: support voltage reference selection

[Tomas Novotny]

Hi Jonathan, Peter,

On Sat, 1 Oct 2016 13:48:38 +0200 (CEST), Peter Meerwald-Stadler
<pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org> wrote:
> 
> some additional comments
> 
> On Sat, 1 Oct 2016, Jonathan Cameron wrote:
> 
> > On 30/09/16 14:19, Tomas Novotny wrote:
> > > MCP47x6 chip supports selection of a voltage reference (VDD, VREF buffered
> > > or unbuffered). MCP4725 doesn't have this feature thus the setting is
> > > ignored and user is warned if there is an attempt to set a value other than
> > > zero.
> > > 
> > > The setting is stored only in the volatile memory of the chip. You need to
> > > manually store it to the EEPROM of the chip via 'store_eeprom' sysfs entry.
> > > 
> > > Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
> > > ---
> > >  drivers/iio/dac/mcp4725.c       | 45 +++++++++++++++++++++++++++++++++++++----
> > >  include/linux/iio/dac/mcp4725.h |  1 +
> > >  2 files changed, 42 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> > > index cca935c..f8e54f1 100644
> > > --- a/drivers/iio/dac/mcp4725.c
> > > +++ b/drivers/iio/dac/mcp4725.c
> > > @@ -28,7 +28,9 @@
> > >  
> > >  struct mcp4725_data {
> > >  	struct i2c_client *client;
> > > +	int id;
> > >  	u16 vref_mv;
> > > +	unsigned vref_mode;
> > >  	u16 dac_value;
> > >  	bool powerdown;
> > >  	unsigned powerdown_mode;
> > > @@ -86,6 +88,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
> > >  		return 0;
> > >  
> > >  	inoutbuf[0] = 0x60; /* write EEPROM */
> > > +	inoutbuf[0] |= data->vref_mode << 3;
> > >  	inoutbuf[1] = data->dac_value >> 4;
> > >  	inoutbuf[2] = (data->dac_value & 0xf) << 4;
> > >  
> > > @@ -329,8 +332,10 @@ static int mcp4725_probe(struct i2c_client *client,
> > >  	struct mcp4725_data *data;
> > >  	struct iio_dev *indio_dev;
> > >  	struct mcp4725_platform_data *platform_data = client->dev.platform_data;
> > > -	u8 inbuf[3];
> > > +	u8 inoutbuf[3];
> > >  	u8 pd;
> > > +	u8 vref;
> > > +	int ret;
> > >  	int err;
> > >  
> > >  	if (!platform_data || !platform_data->vref_mv) {
> > > @@ -344,6 +349,17 @@ static int mcp4725_probe(struct i2c_client *client,
> > >  	data = iio_priv(indio_dev);
> > >  	i2c_set_clientdata(client, indio_dev);
> > >  	data->client = client;
> > > +	data->id = id->driver_data;
> > > +
> > > +	if (data->id == MCP4725 && platform_data->vref_mode > 0) {
> > > +		dev_warn(&client->dev, "vref mode is unavailable on MCP4725, ignoring");
> > > +		platform_data->vref_mode = 0;
> > > +	}
> > > +
> > > +	if (data->id == MCP4726 && platform_data->vref_mode > 3) {
> > > +		dev_err(&client->dev, "vref mode is out of range");
> > > +		return -EINVAL;
> > > +	}
> > >  
> > >  	indio_dev->dev.parent = &client->dev;
> > >  	indio_dev->name = id->name;
> > > @@ -353,17 +369,38 @@ static int mcp4725_probe(struct i2c_client *client,
> > >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > >  
> > >  	data->vref_mv = platform_data->vref_mv;
> > > +	data->vref_mode = platform_data->vref_mode;
> > >  
> > >  	/* read current DAC value */
> > > -	err = i2c_master_recv(client, inbuf, 3);
> > > +	err = i2c_master_recv(client, inoutbuf, 3);
> > >  	if (err < 0) {
> > >  		dev_err(&client->dev, "failed to read DAC value");
> > >  		return err;
> > >  	}
> > > -	pd = (inbuf[0] >> 1) & 0x3;
> > > +	pd = (inoutbuf[0] >> 1) & 0x3;
> > >  	data->powerdown = pd > 0 ? true : false;
> > >  	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
> > > -	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
> > > +	data->dac_value = (inoutbuf[1] << 4) | (inoutbuf[2] >> 4);
> > > +	vref = inoutbuf[0] >> 3 & 0x3;
> 
> parenthesis would be good to make the expression above more clear

ok.

> > > +
> > > +	if (data->id == MCP4726 && vref != data->vref_mode) {
> > > +		dev_info(&client->dev,
> > > +			"vref_mode differs (conf: %d, eeprom: %d), setting %d",
> > > +			data->vref_mode, vref, data->vref_mode);
> 
> use %u, these are unsigned variables

Yes, you are right.

> > > +
> > > +		inoutbuf[0] = 0x40;
> > > +		inoutbuf[0] |= data->vref_mode << 3;
> > > +		if (data->powerdown)
> > > +			inoutbuf[0] |= data->powerdown << 1;
> > > +		inoutbuf[1] = data->dac_value >> 4;
> > > +		inoutbuf[2] = (data->dac_value & 0xf) << 4;
> > > +
> > > +		ret = i2c_master_send(data->client, inoutbuf, 3);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		else if (ret != 3)
> > > +			return -EIO;
> > > +	}
> > >  
> > >  	return iio_device_register(indio_dev);
> > >  }
> > > diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
> > > index 91530e6..370022b 100644
> > > --- a/include/linux/iio/dac/mcp4725.h
> > > +++ b/include/linux/iio/dac/mcp4725.h
> > > @@ -11,6 +11,7 @@
> > >  
> > >  struct mcp4725_platform_data {
> > >  	u16 vref_mv;
> > > +	unsigned vref_mode;
> > Hmm. Should probably move the platform support over to using regulators.
> > This is a horible bit of legacy code.

ok, I will do.

Thanks for the review,

Tomas

> > Jonathan
> > >  };
> > >  
> > >  #endif /* IIO_DAC_MCP4725_H_ */
> > > 
> > 
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html