Re: [PATCH v2 1/2] iio: adc: ad7291: convert to device tree

[Ardelean, Alexandru]

On Tue, 2020-03-17 at 09:51 -0500, Michael Auchter wrote:
> [External]
> 
> There are no in-tree users of the platform data for this driver, so
> remove it and convert the driver to use device tree instead.
> 

A few comments inline for this.
Sorry if this is a bit late, but since there will be a V3 based on the DT
bindings patch, it's still not too late.

> Signed-off-by: Michael Auchter <michael.auchter@xxxxxx>
> ---
> 
> Changes since v1:
> - Fix regulator handling as suggested by Lars
> 
>  drivers/iio/adc/ad7291.c             | 33 ++++++++++++++++------------
>  include/linux/platform_data/ad7291.h | 13 -----------
>  2 files changed, 19 insertions(+), 27 deletions(-)
>  delete mode 100644 include/linux/platform_data/ad7291.h
> 
> diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
> index b2b137fed246..43a201fb4f34 100644
> --- a/drivers/iio/adc/ad7291.c
> +++ b/drivers/iio/adc/ad7291.c
> @@ -20,8 +20,6 @@
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
>  
> -#include <linux/platform_data/ad7291.h>
> -
>  /*
>   * Simplified handling
>   *
> @@ -465,7 +463,6 @@ static const struct iio_info ad7291_info = {
>  static int ad7291_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> -	struct ad7291_platform_data *pdata = client->dev.platform_data;
>  	struct ad7291_chip_info *chip;
>  	struct iio_dev *indio_dev;
>  	int ret;
> @@ -475,16 +472,6 @@ static int ad7291_probe(struct i2c_client *client,
>  		return -ENOMEM;
>  	chip = iio_priv(indio_dev);
>  
> -	if (pdata && pdata->use_external_ref) {
> -		chip->reg = devm_regulator_get(&client->dev, "vref");
> -		if (IS_ERR(chip->reg))
> -			return PTR_ERR(chip->reg);
> -
> -		ret = regulator_enable(chip->reg);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	mutex_init(&chip->state_lock);
>  	/* this is only used for device removal purposes */
>  	i2c_set_clientdata(client, indio_dev);
> @@ -495,8 +482,19 @@ static int ad7291_probe(struct i2c_client *client,
>  			AD7291_T_SENSE_MASK | /* Tsense always enabled */
>  			AD7291_ALERT_POLARITY; /* set irq polarity low level */
>  
> -	if (pdata && pdata->use_external_ref)
> +	chip->reg = devm_regulator_get_optional(&client->dev, "vref");
> +	if (!IS_ERR(chip->reg)) {
> +		ret = regulator_enable(chip->reg);
> +		if (ret)
> +			return ret;
> +
>  		chip->command |= AD7291_EXT_REF;
> +	} else {
> +		if (PTR_ERR(chip->reg) != -ENODEV)
> +			return PTR_ERR(chip->reg);
> +
> +		chip->reg = NULL;
> +	}
>  
>  	indio_dev->name = id->name;
>  	indio_dev->channels = ad7291_channels;
> @@ -569,9 +567,16 @@ static const struct i2c_device_id ad7291_id[] = {
>  
>  MODULE_DEVICE_TABLE(i2c, ad7291_id);
>  
> +static const struct of_device_id ad7291_of_match[] = {
> +	{ .compatible = "adi,ad7291", },
> +	{},

[2]
if updating [1], maybe remove the trailing comma for the null-terminator;
so,  {},  ->  {}
not a biggy, but there are chances that someone else might complain about it
before Jonathan gets a chance to look over this;

> +};
> +MODULE_DEVICE_TABLE(of, ad7291_of_match);
> +
>  static struct i2c_driver ad7291_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
> +		.of_match_table = of_match_ptr(ad7291_of_match),

[1]
not sure if there was a comment about this, but I'd remove the of_match_ptr()
and just assign this directly;
i.e.  .of_match_table = ad7297_of_match,

there is some code that can also handle this OF-table for ACPI as well; I don't
know if this is sufficient for ACPI [with this patch], but it's a good idea to
prepare for that;


>  	},
>  	.probe = ad7291_probe,
>  	.remove = ad7291_remove,
> diff --git a/include/linux/platform_data/ad7291.h
> b/include/linux/platform_data/ad7291.h
> deleted file mode 100644
> index b1fd1530c9a5..000000000000
> --- a/include/linux/platform_data/ad7291.h
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef __IIO_AD7291_H__
> -#define __IIO_AD7291_H__
> -
> -/**
> - * struct ad7291_platform_data - AD7291 platform data
> - * @use_external_ref: Whether to use an external or internal reference
> voltage
> - */
> -struct ad7291_platform_data {
> -	bool use_external_ref;
> -};
> -
> -#endif