[PATCH v2 0/4] iio: dac: mcp4725: use regulator framework, add vref and dt support

[PATCH v2 0/4] iio: dac: mcp4725: use regulator framework, add vref and dt support

[Tomas Novotny]

Hi all,

here is a second version of a series to use regulator framework, add an
option to set external reference and to add devicetree support for
mcp4725/6.

I wasn't using the regulator framework in the previous version. That is the
reason why is the second iteration very different. I took ad7303.c as a
reference.

Changes since v1:
- add the first patch
- use regulator framework
- fix printf argument (%u instead of %d)
- add parenthesis to make expression more clear
- fix typo in documentation (Microchpip)
- move vref configuration from probe function to new mcp4726_set_cfg

Tomas Novotny (4):
  iio: dac: mcp4725: use regulator framework
  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        |  35 ++++
 drivers/iio/dac/mcp4725.c                          | 182 +++++++++++++++++++--
 include/linux/iio/dac/mcp4725.h                    |   3 +-
 3 files changed, 202 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt

-- 
2.1.4

[PATCH v2 1/4] iio: dac: mcp4725: use regulator framework

[Tomas Novotny]

Use a standard framework to get the reference voltage. It is done that way
in the iio subsystem and it will simplify extending of the driver.

Structure mcp4725_platform_data is left undeleted because it used in the
next patch.

This change breaks the current users of the driver, but there is no
mainline user of struct mcp4725_platform_data.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/dac/mcp4725.c       | 46 +++++++++++++++++++++++++++++++++--------
 include/linux/iio/dac/mcp4725.h |  1 -
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
index cca935c..2b28b1f 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/regulator/consumer.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -28,10 +29,10 @@
 
 struct mcp4725_data {
 	struct i2c_client *client;
-	u16 vref_mv;
 	u16 dac_value;
 	bool powerdown;
 	unsigned powerdown_mode;
+	struct regulator *vdd_reg;
 };
 
 static int mcp4725_suspend(struct device *dev)
@@ -283,13 +284,18 @@ static int mcp4725_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct mcp4725_data *data = iio_priv(indio_dev);
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		*val = data->dac_value;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		*val = data->vref_mv;
+		ret = regulator_get_voltage(data->vdd_reg);
+		if (ret < 0)
+			return ret;
+
+		*val = ret / 1000;
 		*val2 = 12;
 		return IIO_VAL_FRACTIONAL_LOG2;
 	}
@@ -328,12 +334,12 @@ 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;
+	struct mcp4725_platform_data *pdata = dev_get_platdata(&client->dev);
 	u8 inbuf[3];
 	u8 pd;
 	int err;
 
-	if (!platform_data || !platform_data->vref_mv) {
+	if (!pdata) {
 		dev_err(&client->dev, "invalid platform data");
 		return -EINVAL;
 	}
@@ -345,6 +351,14 @@ static int mcp4725_probe(struct i2c_client *client,
 	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)
+		return err;
+
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = id->name;
 	indio_dev->info = &mcp4725_info;
@@ -352,25 +366,39 @@ 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;
-
 	/* read current DAC value */
 	err = i2c_master_recv(client, inbuf, 3);
 	if (err < 0) {
 		dev_err(&client->dev, "failed to read DAC value");
-		return err;
+		goto err_disable_vdd_reg;
 	}
 	pd = (inbuf[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);
 
-	return iio_device_register(indio_dev);
+	err = iio_device_register(indio_dev);
+	if (err)
+		goto err_disable_vdd_reg;
+
+	return 0;
+
+
+err_disable_vdd_reg:
+	regulator_disable(data->vdd_reg);
+
+	return err;
 }
 
 static int mcp4725_remove(struct i2c_client *client)
 {
-	iio_device_unregister(i2c_get_clientdata(client));
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct mcp4725_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	regulator_disable(data->vdd_reg);
+
 	return 0;
 }
 
diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
index 91530e6..7c062e8 100644
--- a/include/linux/iio/dac/mcp4725.h
+++ b/include/linux/iio/dac/mcp4725.h
@@ -10,7 +10,6 @@
 #define IIO_DAC_MCP4725_H_
 
 struct mcp4725_platform_data {
-	u16 vref_mv;
 };
 
 #endif /* IIO_DAC_MCP4725_H_ */
-- 
2.1.4

Re: [PATCH v2 1/4] iio: dac: mcp4725: use regulator framework

[Lars-Peter Clausen]

On 10/11/2016 03:57 PM, Tomas Novotny wrote:

Hi,

Looks mostly good. One small thing that should be addressed.

> diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
> index 91530e6..7c062e8 100644
> --- a/include/linux/iio/dac/mcp4725.h
> +++ b/include/linux/iio/dac/mcp4725.h
> @@ -10,7 +10,6 @@
>  #define IIO_DAC_MCP4725_H_
>  
>  struct mcp4725_platform_data {
> -	u16 vref_mv;
>  };

Might as well remove the whole struct and file and drop the if (!pdata)
check in the driver probe function. Having to declare a empty platform_data
for the driver to work is not that sensible.

Re: [PATCH v2 1/4] iio: dac: mcp4725: use regulator framework

[Lars-Peter Clausen]

On 10/14/2016 01:58 PM, Lars-Peter Clausen wrote:
> On 10/11/2016 03:57 PM, Tomas Novotny wrote:
> 
> Hi,
> 
> Looks mostly good. One small thing that should be addressed.
> 
>> diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
>> index 91530e6..7c062e8 100644
>> --- a/include/linux/iio/dac/mcp4725.h
>> +++ b/include/linux/iio/dac/mcp4725.h
>> @@ -10,7 +10,6 @@
>>  #define IIO_DAC_MCP4725_H_
>>  
>>  struct mcp4725_platform_data {
>> -	u16 vref_mv;
>>  };
> 
> Might as well remove the whole struct and file and drop the if (!pdata)
> check in the driver probe function. Having to declare a empty platform_data
> for the driver to work is not that sensible.

Ok, I see, new fields are added in the next commit. So ignore this.

Re: [PATCH v2 1/4] iio: dac: mcp4725: use regulator framework

[Tomas Novotny]

Hi,

On Fri, 14 Oct 2016 13:58:24 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 10/11/2016 03:57 PM, Tomas Novotny wrote:
> 
> Hi,
> 
> Looks mostly good. One small thing that should be addressed.
> 
> > diff --git a/include/linux/iio/dac/mcp4725.h
> > b/include/linux/iio/dac/mcp4725.h index 91530e6..7c062e8 100644
> > --- a/include/linux/iio/dac/mcp4725.h
> > +++ b/include/linux/iio/dac/mcp4725.h
> > @@ -10,7 +10,6 @@
> >  #define IIO_DAC_MCP4725_H_
> >  
> >  struct mcp4725_platform_data {
> > -	u16 vref_mv;
> >  };
> 
> Might as well remove the whole struct and file and drop the if (!pdata)
> check in the driver probe function. Having to declare a empty platform_data
> for the driver to work is not that sensible.

Yes, I was thinking of it but I left the empty structure in the driver
finally (I briefly comment it also in the commit message of this patch). The
reason is that the structure is used again in the next commit. So removing
the file and associated checks and putting it back again would make
unnecessary noise in the patches. Anyway, if you think that it is really
needed I will do it.

Thanks,

Tomas

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

Re: [PATCH v2 1/4] iio: dac: mcp4725: use regulator framework

[Jonathan Cameron]

On 14/10/16 13:24, Tomas Novotny wrote:
> Hi,
> 
> On Fri, 14 Oct 2016 13:58:24 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
>> On 10/11/2016 03:57 PM, Tomas Novotny wrote:
>>
>> Hi,
>>
>> Looks mostly good. One small thing that should be addressed.
>>
>>> diff --git a/include/linux/iio/dac/mcp4725.h
>>> b/include/linux/iio/dac/mcp4725.h index 91530e6..7c062e8 100644
>>> --- a/include/linux/iio/dac/mcp4725.h
>>> +++ b/include/linux/iio/dac/mcp4725.h
>>> @@ -10,7 +10,6 @@
>>>  #define IIO_DAC_MCP4725_H_
>>>  
>>>  struct mcp4725_platform_data {
>>> -	u16 vref_mv;
>>>  };
>>
>> Might as well remove the whole struct and file and drop the if (!pdata)
>> check in the driver probe function. Having to declare a empty platform_data
>> for the driver to work is not that sensible.
> 
> Yes, I was thinking of it but I left the empty structure in the driver
> finally (I briefly comment it also in the commit message of this patch). The
> reason is that the structure is used again in the next commit. So removing
> the file and associated checks and putting it back again would make
> unnecessary noise in the patches. Anyway, if you think that it is really
> needed I will do it.
It's fine.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> 
> Thanks,
> 
> Tomas
> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v2 2/4] 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 eventual setting
is ignored and user is warned.

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@novotny.cz>
---
 drivers/iio/dac/mcp4725.c       | 98 ++++++++++++++++++++++++++++++++++++++---
 include/linux/iio/dac/mcp4725.h |  2 +
 2 files changed, 93 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
index 2b28b1f..29cf85d 100644
--- a/drivers/iio/dac/mcp4725.c
+++ b/drivers/iio/dac/mcp4725.c
@@ -27,12 +27,20 @@
 
 #define MCP4725_DRV_NAME "mcp4725"
 
+#define MCP472X_REF_VDD			0x00
+#define MCP472X_REF_VREF_UNBUFFERED	0x02
+#define MCP472X_REF_VREF_BUFFERED	0x03
+
 struct mcp4725_data {
 	struct i2c_client *client;
+	int id;
+	unsigned ref_mode;
+	bool vref_buffered;
 	u16 dac_value;
 	bool powerdown;
 	unsigned powerdown_mode;
 	struct regulator *vdd_reg;
+	struct regulator *vref_reg;
 };
 
 static int mcp4725_suspend(struct device *dev)
@@ -87,6 +95,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
 		return 0;
 
 	inoutbuf[0] = 0x60; /* write EEPROM */
+	inoutbuf[0] |= data->ref_mode << 3;
 	inoutbuf[1] = data->dac_value >> 4;
 	inoutbuf[2] = (data->dac_value & 0xf) << 4;
 
@@ -279,6 +288,28 @@ static int mcp4725_set_value(struct iio_dev *indio_dev, int val)
 		return 0;
 }
 
+static int mcp4726_set_cfg(struct iio_dev *indio_dev)
+{
+	struct mcp4725_data *data = iio_priv(indio_dev);
+	u8 outbuf[3];
+	int ret;
+
+	outbuf[0] = 0x40;
+	outbuf[0] |= data->ref_mode << 3;
+	if (data->powerdown)
+		outbuf[0] |= data->powerdown << 1;
+	outbuf[1] = data->dac_value >> 4;
+	outbuf[2] = (data->dac_value & 0xf) << 4;
+
+	ret = i2c_master_send(data->client, outbuf, 3);
+	if (ret < 0)
+		return ret;
+	else if (ret != 3)
+		return -EIO;
+	else
+		return 0;
+}
+
 static int mcp4725_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
@@ -291,7 +322,11 @@ static int mcp4725_read_raw(struct iio_dev *indio_dev,
 		*val = data->dac_value;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(data->vdd_reg);
+		if (data->ref_mode == MCP472X_REF_VDD)
+			ret = regulator_get_voltage(data->vdd_reg);
+		else
+			ret = regulator_get_voltage(data->vref_reg);
+
 		if (ret < 0)
 			return ret;
 
@@ -335,8 +370,9 @@ static int mcp4725_probe(struct i2c_client *client,
 	struct mcp4725_data *data;
 	struct iio_dev *indio_dev;
 	struct mcp4725_platform_data *pdata = dev_get_platdata(&client->dev);
-	u8 inbuf[3];
+	u8 inbuf[4];
 	u8 pd;
+	u8 ref;
 	int err;
 
 	if (!pdata) {
@@ -350,6 +386,26 @@ 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 && pdata->use_vref) {
+		dev_warn(&client->dev,
+			"ignoring unavailable external reference on MCP4725");
+		pdata->use_vref = false;
+	}
+
+	if (!pdata->use_vref && pdata->vref_buffered) {
+		dev_warn(&client->dev,
+			"buffering is unavailable on the internal reference");
+		pdata->vref_buffered = false;
+	}
+
+	if (!pdata->use_vref)
+		data->ref_mode = MCP472X_REF_VDD;
+	else
+		data->ref_mode = pdata->vref_buffered ?
+			MCP472X_REF_VREF_BUFFERED :
+			MCP472X_REF_VREF_UNBUFFERED;
 
 	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
 	if (IS_ERR(data->vdd_reg))
@@ -359,6 +415,18 @@ static int mcp4725_probe(struct i2c_client *client,
 	if (err)
 		return err;
 
+	if (pdata->use_vref) {
+		data->vref_reg = devm_regulator_get(&client->dev, "vref");
+		if (IS_ERR(data->vref_reg)) {
+			err =  PTR_ERR(data->vdd_reg);
+			goto err_disable_vdd_reg;
+		}
+
+		err = regulator_enable(data->vref_reg);
+		if (err)
+			goto err_disable_vdd_reg;
+	}
+
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->name = id->name;
 	indio_dev->info = &mcp4725_info;
@@ -366,23 +434,37 @@ static int mcp4725_probe(struct i2c_client *client,
 	indio_dev->num_channels = 1;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	/* read current DAC value */
-	err = i2c_master_recv(client, inbuf, 3);
+	/* read current DAC value and settings */
+	err = i2c_master_recv(client, inbuf, data->id == MCP4725 ? 3 : 4);
 	if (err < 0) {
 		dev_err(&client->dev, "failed to read DAC value");
-		goto err_disable_vdd_reg;
+		goto err_disable_vref_reg;
 	}
 	pd = (inbuf[0] >> 1) & 0x3;
 	data->powerdown = pd > 0 ? true : false;
-	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
+	data->powerdown_mode = pd ? pd - 1 : 2; /* largest resistor to gnd */
 	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
+	if (data->id == MCP4726)
+		ref = (inbuf[3] >> 3) & 0x3;
+
+	if (data->id == MCP4726 && ref != data->ref_mode) {
+		dev_info(&client->dev,
+			"voltage reference mode differs (conf: %u, eeprom: %u), setting %u",
+			data->ref_mode, ref, data->ref_mode);
+		err = mcp4726_set_cfg(indio_dev);
+		if (err < 0)
+			goto err_disable_vref_reg;
+	}
 
 	err = iio_device_register(indio_dev);
 	if (err)
-		goto err_disable_vdd_reg;
+		goto err_disable_vref_reg;
 
 	return 0;
 
+err_disable_vref_reg:
+	if (data->vref_reg)
+		regulator_disable(data->vref_reg);
 
 err_disable_vdd_reg:
 	regulator_disable(data->vdd_reg);
@@ -397,6 +479,8 @@ static int mcp4725_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 
+	if (data->vref_reg)
+		regulator_disable(data->vref_reg);
 	regulator_disable(data->vdd_reg);
 
 	return 0;
diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
index 7c062e8..0684cf6 100644
--- a/include/linux/iio/dac/mcp4725.h
+++ b/include/linux/iio/dac/mcp4725.h
@@ -10,6 +10,8 @@
 #define IIO_DAC_MCP4725_H_
 
 struct mcp4725_platform_data {
+	bool use_vref;
+	bool vref_buffered;
 };
 
 #endif /* IIO_DAC_MCP4725_H_ */
-- 
2.1.4

Re: [PATCH v2 2/4] iio: dac: mcp4725: support voltage reference selection

[Jonathan Cameron]

On 11/10/16 14:57, Tomas Novotny wrote:
> MCP47x6 chip supports selection of a voltage reference (VDD, VREF buffered
> or unbuffered). MCP4725 doesn't have this feature thus the eventual setting
> is ignored and user is warned.
> 
> 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@novotny.cz>
A few minor bits inline.  I think we should error out on invalid settings
in the platform data.  Last thing we want to do is to have to have the driver
papering over these issues and get bitten years down the line by some refactoring
that suddenly doesn't paper over them any more.

Also, a sneaky unconnected error in a comment in here which needs to be broken
out to it's own patch as nothing much to do with this. (good spot though!)

Jonathan
> ---
>  drivers/iio/dac/mcp4725.c       | 98 ++++++++++++++++++++++++++++++++++++++---
>  include/linux/iio/dac/mcp4725.h |  2 +
>  2 files changed, 93 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> index 2b28b1f..29cf85d 100644
> --- a/drivers/iio/dac/mcp4725.c
> +++ b/drivers/iio/dac/mcp4725.c
> @@ -27,12 +27,20 @@
>  
>  #define MCP4725_DRV_NAME "mcp4725"
>  
> +#define MCP472X_REF_VDD			0x00
> +#define MCP472X_REF_VREF_UNBUFFERED	0x02
> +#define MCP472X_REF_VREF_BUFFERED	0x03
> +
>  struct mcp4725_data {
>  	struct i2c_client *client;
> +	int id;
> +	unsigned ref_mode;
> +	bool vref_buffered;
>  	u16 dac_value;
>  	bool powerdown;
>  	unsigned powerdown_mode;
>  	struct regulator *vdd_reg;
> +	struct regulator *vref_reg;
>  };
>  
>  static int mcp4725_suspend(struct device *dev)
> @@ -87,6 +95,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
>  		return 0;
>  
>  	inoutbuf[0] = 0x60; /* write EEPROM */
> +	inoutbuf[0] |= data->ref_mode << 3;
>  	inoutbuf[1] = data->dac_value >> 4;
>  	inoutbuf[2] = (data->dac_value & 0xf) << 4;
>  
> @@ -279,6 +288,28 @@ static int mcp4725_set_value(struct iio_dev *indio_dev, int val)
>  		return 0;
>  }
>  
> +static int mcp4726_set_cfg(struct iio_dev *indio_dev)
> +{
> +	struct mcp4725_data *data = iio_priv(indio_dev);
> +	u8 outbuf[3];
> +	int ret;
> +
> +	outbuf[0] = 0x40;
> +	outbuf[0] |= data->ref_mode << 3;
> +	if (data->powerdown)
> +		outbuf[0] |= data->powerdown << 1;
> +	outbuf[1] = data->dac_value >> 4;
> +	outbuf[2] = (data->dac_value & 0xf) << 4;
> +
> +	ret = i2c_master_send(data->client, outbuf, 3);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret != 3)
> +		return -EIO;
> +	else
> +		return 0;
> +}
> +
>  static int mcp4725_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
> @@ -291,7 +322,11 @@ static int mcp4725_read_raw(struct iio_dev *indio_dev,
>  		*val = data->dac_value;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(data->vdd_reg);
> +		if (data->ref_mode == MCP472X_REF_VDD)
> +			ret = regulator_get_voltage(data->vdd_reg);
> +		else
> +			ret = regulator_get_voltage(data->vref_reg);
> +
>  		if (ret < 0)
>  			return ret;
>  
> @@ -335,8 +370,9 @@ static int mcp4725_probe(struct i2c_client *client,
>  	struct mcp4725_data *data;
>  	struct iio_dev *indio_dev;
>  	struct mcp4725_platform_data *pdata = dev_get_platdata(&client->dev);
> -	u8 inbuf[3];
> +	u8 inbuf[4];
>  	u8 pd;
> +	u8 ref;
>  	int err;
>  
>  	if (!pdata) {
> @@ -350,6 +386,26 @@ 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 && pdata->use_vref) {
> +		dev_warn(&client->dev,
> +			"ignoring unavailable external reference on MCP4725");
Could make this an error case as something is horribly wrong if it occurs.
> +		pdata->use_vref = false;
> +	}
> +
> +	if (!pdata->use_vref && pdata->vref_buffered) {
> +		dev_warn(&client->dev,
> +			"buffering is unavailable on the internal reference");
> +		pdata->vref_buffered = false;
Likewise here. Invalid state so should really error out.
> +	}
> +
> +	if (!pdata->use_vref)
> +		data->ref_mode = MCP472X_REF_VDD;
> +	else
> +		data->ref_mode = pdata->vref_buffered ?
> +			MCP472X_REF_VREF_BUFFERED :
> +			MCP472X_REF_VREF_UNBUFFERED;
>  
>  	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
>  	if (IS_ERR(data->vdd_reg))
> @@ -359,6 +415,18 @@ static int mcp4725_probe(struct i2c_client *client,
>  	if (err)
>  		return err;
>  
> +	if (pdata->use_vref) {
> +		data->vref_reg = devm_regulator_get(&client->dev, "vref");
> +		if (IS_ERR(data->vref_reg)) {
> +			err =  PTR_ERR(data->vdd_reg);
Looks like you have a stray space in the line above.

> +			goto err_disable_vdd_reg;
> +		}
> +
> +		err = regulator_enable(data->vref_reg);
> +		if (err)
> +			goto err_disable_vdd_reg;
> +	}
> +
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->name = id->name;
>  	indio_dev->info = &mcp4725_info;
> @@ -366,23 +434,37 @@ static int mcp4725_probe(struct i2c_client *client,
>  	indio_dev->num_channels = 1;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	/* read current DAC value */
> -	err = i2c_master_recv(client, inbuf, 3);
> +	/* read current DAC value and settings */
> +	err = i2c_master_recv(client, inbuf, data->id == MCP4725 ? 3 : 4);
>  	if (err < 0) {
>  		dev_err(&client->dev, "failed to read DAC value");
> -		goto err_disable_vdd_reg;
> +		goto err_disable_vref_reg;
>  	}
>  	pd = (inbuf[0] >> 1) & 0x3;
>  	data->powerdown = pd > 0 ? true : false;
> -	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
> +	data->powerdown_mode = pd ? pd - 1 : 2; /* largest resistor to gnd */
This fix is clearly right, but not part of the change this patch is making.
Please break out to a separate patch.
>  	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
> +	if (data->id == MCP4726)
> +		ref = (inbuf[3] >> 3) & 0x3;
> +
> +	if (data->id == MCP4726 && ref != data->ref_mode) {
> +		dev_info(&client->dev,
> +			"voltage reference mode differs (conf: %u, eeprom: %u), setting %u",
> +			data->ref_mode, ref, data->ref_mode);
> +		err = mcp4726_set_cfg(indio_dev);
> +		if (err < 0)
> +			goto err_disable_vref_reg;
> +	}
>  
>  	err = iio_device_register(indio_dev);
>  	if (err)
> -		goto err_disable_vdd_reg;
> +		goto err_disable_vref_reg;
>  
>  	return 0;
>  
> +err_disable_vref_reg:
> +	if (data->vref_reg)
> +		regulator_disable(data->vref_reg);
>  
>  err_disable_vdd_reg:
>  	regulator_disable(data->vdd_reg);
> @@ -397,6 +479,8 @@ static int mcp4725_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(indio_dev);
>  
> +	if (data->vref_reg)
> +		regulator_disable(data->vref_reg);
>  	regulator_disable(data->vdd_reg);
>  
>  	return 0;
> diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
> index 7c062e8..0684cf6 100644
> --- a/include/linux/iio/dac/mcp4725.h
> +++ b/include/linux/iio/dac/mcp4725.h
> @@ -10,6 +10,8 @@
>  #define IIO_DAC_MCP4725_H_
>  
Some docs on this seems wise. Maybe state the restrictions (i.e. which parts
support which combinations).
>  struct mcp4725_platform_data {
> +	bool use_vref;
> +	bool vref_buffered;
>  };
>  
>  #endif /* IIO_DAC_MCP4725_H_ */
> 

Re: [PATCH v2 2/4] iio: dac: mcp4725: support voltage reference selection

[Tomas Novotny]

Hi Jonathan,

On Sat, 15 Oct 2016 15:07:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On 11/10/16 14:57, Tomas Novotny wrote:
> > MCP47x6 chip supports selection of a voltage reference (VDD, VREF buffered
> > or unbuffered). MCP4725 doesn't have this feature thus the eventual setting
> > is ignored and user is warned.
> > 
> > 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@novotny.cz>
> A few minor bits inline.  I think we should error out on invalid settings
> in the platform data.  Last thing we want to do is to have to have the driver
> papering over these issues and get bitten years down the line by some refactoring
> that suddenly doesn't paper over them any more.
> 
> Also, a sneaky unconnected error in a comment in here which needs to be broken
> out to it's own patch as nothing much to do with this. (good spot though!)
> 
> Jonathan
> > ---
> >  drivers/iio/dac/mcp4725.c       | 98 ++++++++++++++++++++++++++++++++++++++---
> >  include/linux/iio/dac/mcp4725.h |  2 +
> >  2 files changed, 93 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> > index 2b28b1f..29cf85d 100644
> > --- a/drivers/iio/dac/mcp4725.c
> > +++ b/drivers/iio/dac/mcp4725.c
> > @@ -27,12 +27,20 @@
> >  
> >  #define MCP4725_DRV_NAME "mcp4725"
> >  
> > +#define MCP472X_REF_VDD			0x00
> > +#define MCP472X_REF_VREF_UNBUFFERED	0x02
> > +#define MCP472X_REF_VREF_BUFFERED	0x03
> > +
> >  struct mcp4725_data {
> >  	struct i2c_client *client;
> > +	int id;
> > +	unsigned ref_mode;
> > +	bool vref_buffered;
> >  	u16 dac_value;
> >  	bool powerdown;
> >  	unsigned powerdown_mode;
> >  	struct regulator *vdd_reg;
> > +	struct regulator *vref_reg;
> >  };
> >  
> >  static int mcp4725_suspend(struct device *dev)
> > @@ -87,6 +95,7 @@ static ssize_t mcp4725_store_eeprom(struct device *dev,
> >  		return 0;
> >  
> >  	inoutbuf[0] = 0x60; /* write EEPROM */
> > +	inoutbuf[0] |= data->ref_mode << 3;
> >  	inoutbuf[1] = data->dac_value >> 4;
> >  	inoutbuf[2] = (data->dac_value & 0xf) << 4;
> >  
> > @@ -279,6 +288,28 @@ static int mcp4725_set_value(struct iio_dev *indio_dev, int val)
> >  		return 0;
> >  }
> >  
> > +static int mcp4726_set_cfg(struct iio_dev *indio_dev)
> > +{
> > +	struct mcp4725_data *data = iio_priv(indio_dev);
> > +	u8 outbuf[3];
> > +	int ret;
> > +
> > +	outbuf[0] = 0x40;
> > +	outbuf[0] |= data->ref_mode << 3;
> > +	if (data->powerdown)
> > +		outbuf[0] |= data->powerdown << 1;
> > +	outbuf[1] = data->dac_value >> 4;
> > +	outbuf[2] = (data->dac_value & 0xf) << 4;
> > +
> > +	ret = i2c_master_send(data->client, outbuf, 3);
> > +	if (ret < 0)
> > +		return ret;
> > +	else if (ret != 3)
> > +		return -EIO;
> > +	else
> > +		return 0;
> > +}
> > +
> >  static int mcp4725_read_raw(struct iio_dev *indio_dev,
> >  			   struct iio_chan_spec const *chan,
> >  			   int *val, int *val2, long mask)
> > @@ -291,7 +322,11 @@ static int mcp4725_read_raw(struct iio_dev *indio_dev,
> >  		*val = data->dac_value;
> >  		return IIO_VAL_INT;
> >  	case IIO_CHAN_INFO_SCALE:
> > -		ret = regulator_get_voltage(data->vdd_reg);
> > +		if (data->ref_mode == MCP472X_REF_VDD)
> > +			ret = regulator_get_voltage(data->vdd_reg);
> > +		else
> > +			ret = regulator_get_voltage(data->vref_reg);
> > +
> >  		if (ret < 0)
> >  			return ret;
> >  
> > @@ -335,8 +370,9 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	struct mcp4725_data *data;
> >  	struct iio_dev *indio_dev;
> >  	struct mcp4725_platform_data *pdata = dev_get_platdata(&client->dev);
> > -	u8 inbuf[3];
> > +	u8 inbuf[4];
> >  	u8 pd;
> > +	u8 ref;
> >  	int err;
> >  
> >  	if (!pdata) {
> > @@ -350,6 +386,26 @@ 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 && pdata->use_vref) {
> > +		dev_warn(&client->dev,
> > +			"ignoring unavailable external reference on MCP4725");
> Could make this an error case as something is horribly wrong if it occurs.

ok, I will scream a bit louder.

> > +		pdata->use_vref = false;
> > +	}
> > +
> > +	if (!pdata->use_vref && pdata->vref_buffered) {
> > +		dev_warn(&client->dev,
> > +			"buffering is unavailable on the internal reference");
> > +		pdata->vref_buffered = false;
> Likewise here. Invalid state so should really error out.

same here.

> > +	}
> > +
> > +	if (!pdata->use_vref)
> > +		data->ref_mode = MCP472X_REF_VDD;
> > +	else
> > +		data->ref_mode = pdata->vref_buffered ?
> > +			MCP472X_REF_VREF_BUFFERED :
> > +			MCP472X_REF_VREF_UNBUFFERED;
> >  
> >  	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> >  	if (IS_ERR(data->vdd_reg))
> > @@ -359,6 +415,18 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	if (err)
> >  		return err;
> >  
> > +	if (pdata->use_vref) {
> > +		data->vref_reg = devm_regulator_get(&client->dev, "vref");
> > +		if (IS_ERR(data->vref_reg)) {
> > +			err =  PTR_ERR(data->vdd_reg);
> Looks like you have a stray space in the line above.

oh, good catch.
 
> > +			goto err_disable_vdd_reg;
> > +		}
> > +
> > +		err = regulator_enable(data->vref_reg);
> > +		if (err)
> > +			goto err_disable_vdd_reg;
> > +	}
> > +
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->name = id->name;
> >  	indio_dev->info = &mcp4725_info;
> > @@ -366,23 +434,37 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	indio_dev->num_channels = 1;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> > -	/* read current DAC value */
> > -	err = i2c_master_recv(client, inbuf, 3);
> > +	/* read current DAC value and settings */
> > +	err = i2c_master_recv(client, inbuf, data->id == MCP4725 ? 3 : 4);
> >  	if (err < 0) {
> >  		dev_err(&client->dev, "failed to read DAC value");
> > -		goto err_disable_vdd_reg;
> > +		goto err_disable_vref_reg;
> >  	}
> >  	pd = (inbuf[0] >> 1) & 0x3;
> >  	data->powerdown = pd > 0 ? true : false;
> > -	data->powerdown_mode = pd ? pd - 1 : 2; /* largest register to gnd */
> > +	data->powerdown_mode = pd ? pd - 1 : 2; /* largest resistor to gnd */
> This fix is clearly right, but not part of the change this patch is making.
> Please break out to a separate patch.

ok, I will do.

> >  	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
> > +	if (data->id == MCP4726)
> > +		ref = (inbuf[3] >> 3) & 0x3;
> > +
> > +	if (data->id == MCP4726 && ref != data->ref_mode) {
> > +		dev_info(&client->dev,
> > +			"voltage reference mode differs (conf: %u, eeprom: %u), setting %u",
> > +			data->ref_mode, ref, data->ref_mode);
> > +		err = mcp4726_set_cfg(indio_dev);
> > +		if (err < 0)
> > +			goto err_disable_vref_reg;
> > +	}
> >  
> >  	err = iio_device_register(indio_dev);
> >  	if (err)
> > -		goto err_disable_vdd_reg;
> > +		goto err_disable_vref_reg;
> >  
> >  	return 0;
> >  
> > +err_disable_vref_reg:
> > +	if (data->vref_reg)
> > +		regulator_disable(data->vref_reg);
> >  
> >  err_disable_vdd_reg:
> >  	regulator_disable(data->vdd_reg);
> > @@ -397,6 +479,8 @@ static int mcp4725_remove(struct i2c_client *client)
> >  
> >  	iio_device_unregister(indio_dev);
> >  
> > +	if (data->vref_reg)
> > +		regulator_disable(data->vref_reg);
> >  	regulator_disable(data->vdd_reg);
> >  
> >  	return 0;
> > diff --git a/include/linux/iio/dac/mcp4725.h b/include/linux/iio/dac/mcp4725.h
> > index 7c062e8..0684cf6 100644
> > --- a/include/linux/iio/dac/mcp4725.h
> > +++ b/include/linux/iio/dac/mcp4725.h
> > @@ -10,6 +10,8 @@
> >  #define IIO_DAC_MCP4725_H_
> >  
> Some docs on this seems wise. Maybe state the restrictions (i.e. which parts
> support which combinations).

I will write a short info here with a link to the dt documentation.

Thanks for the review,

Tomas

> >  struct mcp4725_platform_data {
> > +	bool use_vref;
> > +	bool vref_buffered;
> >  };
> >  
> >  #endif /* IIO_DAC_MCP4725_H_ */
> > 
> 

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

[Tomas Novotny]

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 .../devicetree/bindings/iio/dac/mcp4725.txt        | 35 ++++++++++++++++++++++
 1 file changed, 35 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..1bc6c09
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
@@ -0,0 +1,35 @@
+Microchip mcp4725 and mcp4726 DAC device driver
+
+Required properties:
+	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
+	- reg: Should contain the DAC I2C address
+	- vdd-supply: Phandle to the Vdd power supply. This supply is used as a
+	  voltage reference on mcp4725. It is used as a voltage reference on
+	  mcp4726 if there is no vref-supply specified.
+
+Optional properties (valid only for mcp4726):
+	- vref-supply: Optional phandle to the Vref power supply. Vref pin is
+	  used as a voltage reference when this supply is specified.
+	- microchip,vref-buffered: Boolean to enable buffering of the external
+	  Vref pin. This boolean is not valid without the vref-supply. Quoting
+	  the datasheet: This is offered in cases where the reference voltage
+	  does not have the current capability not to drop its voltage when
+	  connected to the internal resistor ladder circuit.
+
+Examples:
+
+	/* simple mcp4725 */
+	mcp4725@60 {
+		compatible = "microchip,mcp4725";
+		reg = <0x60>;
+		vdd-supply = <&vdac_vdd>;
+	};
+
+	/* mcp4726 with the buffered external reference voltage */
+	mcp4726@60 {
+		compatible = "microchip,mcp4726";
+		reg = <0x60>;
+		vdd-supply = <&vdac_vdd>;
+		vref-supply = <&vdac_vref>;
+		microchip,vref-buffered;
+	};
-- 
2.1.4

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

[Jonathan Cameron]

On 11/10/16 14:57, Tomas Novotny wrote:
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
Nicely documented.  Just odd enough that I'll want a device tree Ack on this one
before I take it.

Thanks,

Jonathan
> ---
>  .../devicetree/bindings/iio/dac/mcp4725.txt        | 35 ++++++++++++++++++++++
>  1 file changed, 35 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..1bc6c09
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/mcp4725.txt
> @@ -0,0 +1,35 @@
> +Microchip mcp4725 and mcp4726 DAC device driver
> +
> +Required properties:
> +	- compatible: Must be "microchip,mcp4725" or "microchip,mcp4726"
> +	- reg: Should contain the DAC I2C address
> +	- vdd-supply: Phandle to the Vdd power supply. This supply is used as a
> +	  voltage reference on mcp4725. It is used as a voltage reference on
> +	  mcp4726 if there is no vref-supply specified.
> +
> +Optional properties (valid only for mcp4726):
> +	- vref-supply: Optional phandle to the Vref power supply. Vref pin is
> +	  used as a voltage reference when this supply is specified.
> +	- microchip,vref-buffered: Boolean to enable buffering of the external
> +	  Vref pin. This boolean is not valid without the vref-supply. Quoting
> +	  the datasheet: This is offered in cases where the reference voltage
> +	  does not have the current capability not to drop its voltage when
> +	  connected to the internal resistor ladder circuit.
It is particularly nice to have this description of the 'why' in here.
I wish more docs did this!
> +
> +Examples:
> +
> +	/* simple mcp4725 */
> +	mcp4725@60 {
> +		compatible = "microchip,mcp4725";
> +		reg = <0x60>;
> +		vdd-supply = <&vdac_vdd>;
> +	};
> +
> +	/* mcp4726 with the buffered external reference voltage */
> +	mcp4726@60 {
> +		compatible = "microchip,mcp4726";
> +		reg = <0x60>;
> +		vdd-supply = <&vdac_vdd>;
> +		vref-supply = <&vdac_vref>;
> +		microchip,vref-buffered;
> +	};
> 

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

[Rob Herring]

On Tue, Oct 11, 2016 at 03:57:42PM +0200, Tomas Novotny wrote:
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  .../devicetree/bindings/iio/dac/mcp4725.txt        | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/mcp4725.txt

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 4/4] iio: dac: mcp4725: add devicetree support

[Tomas Novotny]

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/dac/mcp4725.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
index 29cf85d..b8954f2 100644
--- a/drivers/iio/dac/mcp4725.c
+++ b/drivers/iio/dac/mcp4725.c
@@ -19,6 +19,7 @@
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/regulator/consumer.h>
+#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -364,6 +365,30 @@ static const struct iio_info mcp4725_info = {
 	.driver_module = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static int mcp4725_probe_dt(struct device *dev,
+			    struct mcp4725_platform_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+
+	if (!np)
+		return -ENODEV;
+
+	/* check if is the vref-supply defined */
+	pdata->use_vref = of_property_read_bool(np, "vref-supply");
+	pdata->vref_buffered =
+		of_property_read_bool(np, "microchip,vref-buffered");
+
+	return 0;
+}
+#else
+static int mcp4725_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)
 {
@@ -375,11 +400,6 @@ static int mcp4725_probe(struct i2c_client *client,
 	u8 ref;
 	int err;
 
-	if (!pdata) {
-		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;
@@ -388,6 +408,19 @@ static int mcp4725_probe(struct i2c_client *client,
 	data->client = client;
 	data->id = id->driver_data;
 
+	if (!pdata) {
+		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+
+		err = mcp4725_probe_dt(&client->dev, pdata);
+		if (err) {
+			dev_err(&client->dev,
+				"invalid platform or devicetree data");
+			return err;
+		}
+	}
+
 	if (data->id == MCP4725 && pdata->use_vref) {
 		dev_warn(&client->dev,
 			"ignoring unavailable external reference on MCP4725");
@@ -460,6 +493,9 @@ static int mcp4725_probe(struct i2c_client *client,
 	if (err)
 		goto err_disable_vref_reg;
 
+	if (!dev_get_platdata(&client->dev))
+		devm_kfree(&client->dev, pdata);
+
 	return 0;
 
 err_disable_vref_reg:
-- 
2.1.4

Re: [PATCH v2 4/4] iio: dac: mcp4725: add devicetree support

[Jonathan Cameron]

On 11/10/16 14:57, Tomas Novotny wrote:
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
Few small bits inline.

Jonathan
> ---
>  drivers/iio/dac/mcp4725.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> index 29cf85d..b8954f2 100644
> --- a/drivers/iio/dac/mcp4725.c
> +++ b/drivers/iio/dac/mcp4725.c
> @@ -19,6 +19,7 @@
>  #include <linux/err.h>
>  #include <linux/delay.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/of.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -364,6 +365,30 @@ static const struct iio_info mcp4725_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static int mcp4725_probe_dt(struct device *dev,
> +			    struct mcp4725_platform_data *pdata)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	/* check if is the vref-supply defined */
> +	pdata->use_vref = of_property_read_bool(np, "vref-supply");
> +	pdata->vref_buffered =
> +		of_property_read_bool(np, "microchip,vref-buffered");
> +
> +	return 0;
> +}
> +#else
> +static int mcp4725_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)
>  {
> @@ -375,11 +400,6 @@ static int mcp4725_probe(struct i2c_client *client,
>  	u8 ref;
>  	int err;
>  
> -	if (!pdata) {
> -		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;
> @@ -388,6 +408,19 @@ static int mcp4725_probe(struct i2c_client *client,
>  	data->client = client;
>  	data->id = id->driver_data;
>  
> +	if (!pdata) {
May seem an odd way to do it, but I would use
if (!dev_get_platdata(&client->dev)) so it's obvious what it matches
against when it comes to unwinding this down below.

> +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		err = mcp4725_probe_dt(&client->dev, pdata);
> +		if (err) {
> +			dev_err(&client->dev,
> +				"invalid platform or devicetree data");
> +			return err;
> +		}
> +	}
> +
>  	if (data->id == MCP4725 && pdata->use_vref) {
>  		dev_warn(&client->dev,
>  			"ignoring unavailable external reference on MCP4725");
> @@ -460,6 +493,9 @@ static int mcp4725_probe(struct i2c_client *client,
>  	if (err)
>  		goto err_disable_vref_reg;
>  
> +	if (!dev_get_platdata(&client->dev))
> +		devm_kfree(&client->dev, pdata);
Hmm. I wonder if it's worth freeing this explicitly rather than just letting
it clear up on driver remove.  It seems odd to use devm allocations for
what is a short term termporary variable.

Actually why not just use a local variable on the stack and set
pdata to point to that?  Then your cleanup is all nice an automatic
without having to do this slightly odd devm usage.
> +
>  	return 0;
>  
>  err_disable_vref_reg:
> 

Re: [PATCH v2 4/4] iio: dac: mcp4725: add devicetree support

[Tomas Novotny]

Hi Jonathan,

On Sat, 15 Oct 2016 15:14:19 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On 11/10/16 14:57, Tomas Novotny wrote:
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> Few small bits inline.
> 
> Jonathan
> > ---
> >  drivers/iio/dac/mcp4725.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 41 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> > index 29cf85d..b8954f2 100644
> > --- a/drivers/iio/dac/mcp4725.c
> > +++ b/drivers/iio/dac/mcp4725.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/err.h>
> >  #include <linux/delay.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -364,6 +365,30 @@ static const struct iio_info mcp4725_info = {
> >  	.driver_module = THIS_MODULE,
> >  };
> >  
> > +#ifdef CONFIG_OF
> > +static int mcp4725_probe_dt(struct device *dev,
> > +			    struct mcp4725_platform_data *pdata)
> > +{
> > +	struct device_node *np = dev->of_node;
> > +
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	/* check if is the vref-supply defined */
> > +	pdata->use_vref = of_property_read_bool(np, "vref-supply");
> > +	pdata->vref_buffered =
> > +		of_property_read_bool(np, "microchip,vref-buffered");
> > +
> > +	return 0;
> > +}
> > +#else
> > +static int mcp4725_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)
> >  {
> > @@ -375,11 +400,6 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	u8 ref;
> >  	int err;
> >  
> > -	if (!pdata) {
> > -		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;
> > @@ -388,6 +408,19 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	data->client = client;
> >  	data->id = id->driver_data;
> >  
> > +	if (!pdata) {
> May seem an odd way to do it, but I would use
> if (!dev_get_platdata(&client->dev)) so it's obvious what it matches
> against when it comes to unwinding this down below.

I will rewrite it completely, see below.

> > +		pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > +		if (!pdata)
> > +			return -ENOMEM;
> > +
> > +		err = mcp4725_probe_dt(&client->dev, pdata);
> > +		if (err) {
> > +			dev_err(&client->dev,
> > +				"invalid platform or devicetree data");
> > +			return err;
> > +		}
> > +	}
> > +
> >  	if (data->id == MCP4725 && pdata->use_vref) {
> >  		dev_warn(&client->dev,
> >  			"ignoring unavailable external reference on MCP4725");
> > @@ -460,6 +493,9 @@ static int mcp4725_probe(struct i2c_client *client,
> >  	if (err)
> >  		goto err_disable_vref_reg;
> >  
> > +	if (!dev_get_platdata(&client->dev))
> > +		devm_kfree(&client->dev, pdata);
> Hmm. I wonder if it's worth freeing this explicitly rather than just letting
> it clear up on driver remove.  It seems odd to use devm allocations for
> what is a short term termporary variable.
> 
> Actually why not just use a local variable on the stack and set
> pdata to point to that?  Then your cleanup is all nice an automatic
> without having to do this slightly odd devm usage.

Well, you are right. I saw this construction somewhere else (in another
subsystem) but the local variable would be better.

Thanks for the review,

Tomas

> > +
> >  	return 0;
> >  
> >  err_disable_vref_reg:
> > 
>