Hi Jonathan, On Sat, 15 Oct 2016 15:14:19 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On 11/10/16 14:57, Tomas Novotny wrote: > > Signed-off-by: Tomas Novotny <tomas@xxxxxxxxxx> > 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: > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html