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

[Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On 11/10/16 14:57, Tomas Novotny wrote:
> Signed-off-by: Tomas Novotny <tomas-P46umIhNmdHrBKCeMvbIDA@public.gmane.org>
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:
>