linux-kernel - Re: [PATCH 1/2] iio: adc: ad799x: add reference voltage capability to chip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250806155853.00003f0b@huawei.com>
Date: Wed, 6 Aug 2025 15:58:53 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Stefano Manni <stefano.manni@...il.com>
CC: <lars@...afoo.de>, <Michael.Hennerich@...log.com>, <jic23@...nel.org>,
	<dlechner@...libre.com>, <nuno.sa@...log.com>, <andy@...nel.org>,
	<linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] iio: adc: ad799x: add reference voltage capability
 to chip_info

On Wed,  6 Aug 2025 11:01:57 +0200
Stefano Manni <stefano.manni@...il.com> wrote:

> If the chip supports an external reference voltage
> on REFIN pin then the "vref-supply" regulator may be used.
> 
> This commit partially refactors 6b104e7895ab16b9b7f466c5f2ca282b87f661e8
> to add the capability of the chip to have an external
> voltage reference and then remove the ugly conditional check
> on chip id.
> 
> Signed-off-by: Stefano Manni <stefano.manni@...il.com>

Hi Stefano,
`
A few minor things inline.

Mostly looks good to me. 

Jonathan

> ---
>  drivers/iio/adc/ad799x.c | 45 +++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index 9c02f9199139..955d845407b9 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -114,11 +114,13 @@ struct ad799x_chip_config {
>   * @num_channels:	number of channels
>   * @noirq_config:	device configuration w/o IRQ
>   * @irq_config:		device configuration w/IRQ
> + * @has_vref:		device supports external reference voltage
>   */
>  struct ad799x_chip_info {
>  	int				num_channels;
>  	const struct ad799x_chip_config	noirq_config;
>  	const struct ad799x_chip_config	irq_config;
> +	bool has_vref;
>  };
>  
>  struct ad799x_state {
> @@ -604,6 +606,7 @@ static const struct iio_event_spec ad799x_events[] = {
>  static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	[ad7991] = {
>  		.num_channels = 5,
> +		.has_vref = true,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 12),
> @@ -617,6 +620,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7995] = {
>  		.num_channels = 5,
> +		.has_vref = true,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 10),
> @@ -630,6 +634,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7999] = {
>  		.num_channels = 5,
> +		.has_vref = true,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 8),
> @@ -643,6 +648,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7992] = {
>  		.num_channels = 3,
> +		.has_vref = false,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 12),
> @@ -663,6 +669,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7993] = {
>  		.num_channels = 5,
> +		.has_vref = false,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 10),
> @@ -687,6 +694,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7994] = {
>  		.num_channels = 5,
> +		.has_vref = false,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 12),
> @@ -711,6 +719,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7997] = {
>  		.num_channels = 9,
> +		.has_vref = false,

Seems like 'not' having a vref is a reasonable default, so you could just not
set it at all in those cases and rely on the default being false.

>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 10),
> @@ -743,6 +752,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
>  	},
>  	[ad7998] = {
>  		.num_channels = 9,
> +		.has_vref = false,
>  		.noirq_config = {
>  			.channel = {
>  				AD799X_CHANNEL(0, 12),
> @@ -809,34 +819,31 @@ static int ad799x_probe(struct i2c_client *client)
>  		return ret;
>  
>  	/* check if an external reference is supplied */
> -	st->vref = devm_regulator_get_optional(&client->dev, "vref");
> -
> -	if (IS_ERR(st->vref)) {
> -		if (PTR_ERR(st->vref) == -ENODEV) {
> -			st->vref = NULL;
> -			dev_info(&client->dev, "Using VCC reference voltage\n");
> -		} else {
> -			ret = PTR_ERR(st->vref);
> -			goto error_disable_reg;
> +	if (chip_info->has_vref) {
> +		st->vref = devm_regulator_get_optional(&client->dev, "vref");
> +
> +		if (IS_ERR(st->vref)) {
> +			if (PTR_ERR(st->vref) == -ENODEV) {
> +				st->vref = NULL;
> +				dev_info(&client->dev, "Using VCC reference voltage\n");
> +			} else {
> +				ret = PTR_ERR(st->vref);
> +				goto error_disable_reg;
> +			}
>  		}
> -	}
>  
> -	if (st->vref) {
> -		/*
> -		 * Use external reference voltage if supported by hardware.
> -		 * This is optional if voltage / regulator present, use VCC otherwise.
> -		 */
> -		if ((st->id == ad7991) || (st->id == ad7995) || (st->id == ad7999)) {
> +		if (st->vref) {
>  			dev_info(&client->dev, "Using external reference voltage\n");
>  			extra_config |= AD7991_REF_SEL;
>  			ret = regulator_enable(st->vref);
>  			if (ret)
>  				goto error_disable_reg;
> -		} else {
> -			st->vref = NULL;
> -			dev_warn(&client->dev, "Supplied reference not supported\n");
>  		}
>  	}
> +	else {

	} else {

> +		st->vref = NULL;
> +		dev_dbg(&client->dev, "Supplied reference not supported\n");
> +	}
>  
>  	st->client = client;
>