linux-kernel - Re: [PATCH v1 6/7] iio: adc: ad4170: Add support for internal temperature sensor

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: <8c785dcda6d22a4a50a2d3994248c33fee69031f.camel@gmail.com>
Date: Thu, 10 Apr 2025 11:03:41 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, linux-iio@...r.kernel.org,
 	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: jic23@...nel.org, lars@...afoo.de, Michael.Hennerich@...log.com, 
	dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 robh@...nel.org, 	krzk+dt@...nel.org, conor+dt@...nel.org,
 marcelo.schmitt1@...il.com
Subject: Re: [PATCH v1 6/7] iio: adc: ad4170: Add support for internal
 temperature sensor

On Wed, 2025-04-09 at 09:26 -0300, Marcelo Schmitt wrote:
> The AD4170 has an internal temperature sensor that can be read using the
> ADC. Whenever possible, configure an IIO channel to provide the chip's
> temperature.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---

One minor nit... Otherwise looks good:

Reviewed-by: Nuno Sá <nuno.sa@...log.com>

>  drivers/iio/adc/ad4170.c | 72 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index b382e7f3dbe0..d204f8ca840f 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -922,6 +922,27 @@ static const struct iio_chan_spec ad4170_channel_template
> = {
>  	},
>  };
>  
> +static const struct iio_chan_spec ad4170_temp_channel_template = {
> +	.type = IIO_TEMP,
> +	.indexed = 0,
> +	.channel = 17,
> +	.channel2 = 17,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			      BIT(IIO_CHAN_INFO_SCALE) |
> +			      BIT(IIO_CHAN_INFO_OFFSET) |
> +			      BIT(IIO_CHAN_INFO_CALIBSCALE) |
> +			      BIT(IIO_CHAN_INFO_CALIBBIAS) |
> +			      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	.scan_type = {
> +		.sign = 's',
> +		.realbits = 24,
> +		.storagebits = 32,
> +		.shift = 8,
> +		.endianness = IIO_BE,
> +	},
> +};
> +
>  /*
>   * Receives the number of a multiplexed AD4170 input (ain_n), and stores the
>   * voltage (in µV) of the specified input into ain_voltage. If the input
> number
> @@ -1209,9 +1230,27 @@ static int ad4170_read_raw(struct iio_dev *indio_dev,
>  		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		pga = FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);
> -		*val = chan_info->scale_tbl[pga][0];
> -		*val2 = chan_info->scale_tbl[pga][1];
> -		return IIO_VAL_INT_PLUS_NANO;
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = chan_info->scale_tbl[pga][0];
> +			*val2 = chan_info->scale_tbl[pga][1];
> +			return IIO_VAL_INT_PLUS_NANO;
> +
> +		case IIO_TEMP:
> +			/*
> +			 * The scale_tbl converts output codes to mV units so
> +			 * multiply by MILLI to make the factor convert to
> µV.
> +			 * Then, apply the temperature sensor change
> sensitivity
> +			 * of 477 μV/K. Finally, multiply the result by MILLI
> +			 * again to comply with milli degrees Celsius IIO
> ABI.
> +			 */
> +			*val = 0; /* The scale integer part is always 0. */

Hmm this comment does not add much...

> +			*val2 = DIV_ROUND_CLOSEST(chan_info-
> >scale_tbl[pga][1] * MILLI,
> +						  477) * MILLI;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_OFFSET:
>  		pga = FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);
>  		*val = chan_info->offset_tbl[pga];
> @@ -1855,12 +1894,39 @@ static int ad4170_parse_channels(struct iio_dev
> *indio_dev)
>  	if (num_channels > AD4170_MAX_CHANNELS)
>  		return dev_err_probe(dev, -EINVAL, "Too many channels\n");
>  
> +	/* Add one for temperature */
> +	num_channels = min(num_channels + 1, AD4170_MAX_CHANNELS);
> +
>  	device_for_each_child_node_scoped(dev, child) {
>  		ret = ad4170_parse_channel_node(indio_dev, child,
> chan_num++);
>  		if (ret)
>  			return ret;
>  	}
>  
> +	/*
> +	 * Add internal temperature sensor channel if the maximum number of
> +	 * channels has not been reached.
> +	 */
> +	if (num_channels < AD4170_MAX_CHANNELS) {
> +		struct ad4170_setup *setup = &st->chan_infos[chan_num].setup;
> +
> +		st->chans[chan_num] = ad4170_temp_channel_template;
> +		st->chans[chan_num].address = chan_num;
> +		st->chans[chan_num].scan_index = chan_num;
> +
> +		st->chan_infos[chan_num].setup_num = AD4170_INVALID_SETUP;
> +		st->chan_infos[chan_num].initialized = true;
> +
> +		setup->afe |= FIELD_PREP(AD4170_AFE_REF_SELECT_MSK,
> +					 AD4170_REF_AVDD);
> +
> +		ret = ad4170_get_input_range(st, &st->chans[chan_num],
> chan_num,
> +					     AD4170_REF_AVDD);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Invalid input
> config\n");
> +
> +		st->chan_infos[chan_num].input_range_uv = ret;
> +	}
>  	indio_dev->num_channels = num_channels;
>  	indio_dev->channels = st->chans;
>  	return 0;