Re: [PATCH v5 2/2] staging: iio: ad2s1210: Add device tree support.

[Jonathan Cameron]

On Sat, 27 Oct 2018 17:49:03 +0200
Slawomir Stepien <sst@xxxxxxxxx> wrote:

> Hi
> 
> On paÅ 26, 2018 18:55, Nishad Kamdar wrote:
> > Add device tree table for matching vendor ID
> > and support for retrieving platform data
> > from device tree.  
> 
> So maybe you should make 2 commits?
> 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@xxxxxxxxx>
> > ---
> >  drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index 93c3c70ce62e..9fd5461c4ed0 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -669,6 +670,27 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_OF
> > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev)
> > +{
> > +	struct device_node *np = dev->of_node;
> > +	struct ad2s1210_platform_data *pdata;
> > +
> > +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return NULL;
> > +
> > +	pdata->gpioin = of_property_read_bool(np, "adi,gpioin");  
> 
> Why here you are adding "adi", but you are not adding it to the gpios in prev
> commit?
> 
> I've also seen this: https://patchwork.kernel.org/patch/10355839/. However I do
> not understand why adding vendor id to props is needed...
This is not standard ABI, hence we define it as being under the scope of
a particular manufacturer (and hope they use it consistently ;)

So this should have a the prefix, as should the gpios now you mention it.

We have gotten that bit wrong a lot in the past in IIO.  If the gpio is
standard, i.e. reset or similar it doesn't need it, but for very much device
specific gpios like these it should have the prefix
(similar reasons to any other property - different manufacturers might
use the same name for different things).

Jonathan

> 
> > +
> > +	return pdata;
> > +}
> > +#else
> > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev)
> > +{
> > +	return NULL;
> > +}
> > +#endif
> > +
> >  static int ad2s1210_probe(struct spi_device *spi)
> >  {
> >  	struct iio_dev *indio_dev;
> > @@ -682,7 +704,19 @@ static int ad2s1210_probe(struct spi_device *spi)
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  	st = iio_priv(indio_dev);
> > -	st->pdata = spi->dev.platform_data;
> > +	if (spi->dev.of_node) {
> > +		st->pdata = ad2s1210_parse_dt(&spi->dev);
> > +		if (!st->pdata)
> > +			return -EINVAL;
> > +	} else {
> > +		st->pdata = spi->dev.platform_data;
> > +	}
> > +
> > +	if (!st->pdata) {
> > +		dev_err(&spi->dev, "ad2s1210: no platform data supplied\n");
> > +		return -EINVAL;
> > +	}
> > +  
> 
> Why not just use only device-tree here? The ad2s1210_platform_data has now only
> one entry... In that case remember to add "depends on OF" in Kconfig.
> 
> >  	ret = ad2s1210_setup_gpios(st);
> >  	if (ret < 0)
> >  		return ret;
> > @@ -724,6 +758,12 @@ static int ad2s1210_remove(struct spi_device *spi)
> >  	return 0;
> >  }
> >  
> > +static const struct of_device_id ad2s1210_of_match[] = {
> > +	{ .compatible = "adi,ad2s1210", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
> > +
> >  static const struct spi_device_id ad2s1210_id[] = {
> >  	{ "ad2s1210" },
> >  	{}
> > @@ -733,6 +773,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id);
> >  static struct spi_driver ad2s1210_driver = {
> >  	.driver = {
> >  		.name = DRV_NAME,
> > +		.of_match_table = of_match_ptr(ad2s1210_of_match),
> >  	},
> >  	.probe = ad2s1210_probe,
> >  	.remove = ad2s1210_remove,  
>