diff mbox series

[v3,10/10] iio: adc: ad7124: Implement temperature measurement

Message ID 20241122113322.242875-22-u.kleine-koenig@baylibre.com (mailing list archive)
State New
Headers show
Series iio: adc: ad7124: Various fixes | expand

Commit Message

Uwe Kleine-König Nov. 22, 2024, 11:33 a.m. UTC 
If the maximal count of channels the driver supports isn't fully
utilized, add an attribute providing the internal temperature.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 112 +++++++++++++++++++++++++++++++--------
 1 file changed, 91 insertions(+), 21 deletions(-)

Comments

David Lechner Nov. 22, 2024, 3:46 p.m. UTC | #1 
On 11/22/24 5:33 AM, Uwe Kleine-König wrote:
> If the maximal count of channels the driver supports isn't fully
> utilized, add an attribute providing the internal temperature.
> 

...

> @@ -901,6 +945,32 @@ static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
>  		chan[channel].channel2 = ain[1];
>  	}
>  
> +	if (num_channels < AD7124_MAX_CHANNELS) {
> +		st->channels[num_channels] = (struct ad7124_channel) {
> +			.nr = num_channels,
> +			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
> +				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
> +			.cfg = {
> +				.bipolar = true,
> +			},
> +		};
> +
> +		chan[num_channels] = (struct iio_chan_spec) {
> +			.type = IIO_TEMP,
> +			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +			.scan_type = {
> +				.sign = 'u',

The combination of .bipolar = true and .sign = 'u' looks a bit suspicions.

> +				.realbits = 24,
> +				.storagebits = 32,
> +				.endianness = IIO_BE,
> +			},
> +			.address = num_channels,
> +			.scan_index = num_channels,
> +		};
> +	};
> +
>  	return 0;
>  }
>
Andy Shevchenko Nov. 22, 2024, 8:31 p.m. UTC | #2 
On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> If the maximal count of channels the driver supports isn't fully
> utilized, add an attribute providing the internal temperature.

...

>         case IIO_CHAN_INFO_SCALE:
> -               mutex_lock(&st->cfgs_lock);
> +               switch (chan->type) {
> +               case IIO_VOLTAGE:
> +                       mutex_lock(&st->cfgs_lock);

Side note 1: cleanup.h at some point?

...

>         case IIO_CHAN_INFO_OFFSET:
> -               mutex_lock(&st->cfgs_lock);
> -               if (st->channels[chan->address].cfg.bipolar)
> -                       *val = -(1 << (chan->scan_type.realbits - 1));
> -               else
> -                       *val = 0;
> +               switch (chan->type) {
> +               case IIO_VOLTAGE:
> +                       mutex_lock(&st->cfgs_lock);
> +                       if (st->channels[chan->address].cfg.bipolar)
> +                               *val = -(1 << (chan->scan_type.realbits - 1));

Side note 2: BIT() ?

...

>         case IIO_CHAN_INFO_SAMP_FREQ:
>                 mutex_lock(&st->cfgs_lock);
>                 *val = st->channels[chan->address].cfg.odr;
>                 mutex_unlock(&st->cfgs_lock);
>
>                 return IIO_VAL_INT;
> +
>         case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>                 mutex_lock(&st->cfgs_lock);
>                 *val = ad7124_get_3db_filter_freq(st, chan->scan_index);
>                 mutex_unlock(&st->cfgs_lock);
>
>                 return IIO_VAL_INT;
> +

Seems like stray / unrelated changes. Do you really want to combine
this with style changing? Usually we either change style first
followed by featuring, or vice versa.

>         default:
>                 return -EINVAL;
>         }
> @@ -645,6 +685,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
>
>                 ad7124_set_channel_odr(st, chan->address, val);
>                 break;
> +
>         case IIO_CHAN_INFO_SCALE:
>                 if (val != 0) {
>                         ret = -EINVAL;
> @@ -666,6 +707,7 @@ static int ad7124_write_raw(struct iio_dev *indio_dev,
>
>                 st->channels[chan->address].cfg.pga_bits = res;
>                 break;
> +
>         case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
>                 if (val2 != 0) {
>                         ret = -EINVAL;

Ditto.

...

> +       /* Add one for temperature */
> +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);

Is the type of both arguments the same?
Uwe Kleine-König Nov. 25, 2024, 11:27 a.m. UTC | #3 
On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > +       /* Add one for temperature */
> > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> 
> Is the type of both arguments the same?

Hmm, my compiler is happy with it at least. I don't understand why
though. I'll do a few more tests ...

Best regards
Uwe
Andy Shevchenko Nov. 25, 2024, 1:47 p.m. UTC | #4 
On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > > +       /* Add one for temperature */
> > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> >
> > Is the type of both arguments the same?
>
> Hmm, my compiler is happy with it at least. I don't understand why
> though. I'll do a few more tests ...

If num_channels is signed int or shorter than (independently on the
sign) int, then it's obvious why. + 1 makes it int.
Uwe Kleine-König Nov. 25, 2024, 2:52 p.m. UTC | #5 
On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > <u.kleine-koenig@baylibre.com> wrote:
> > > > +       /* Add one for temperature */
> > > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> > >
> > > Is the type of both arguments the same?
> >
> > Hmm, my compiler is happy with it at least. I don't understand why
> > though. I'll do a few more tests ...
> 
> If num_channels is signed int or shorter than (independently on the
> sign) int, then it's obvious why. + 1 makes it int.

Ah indeed, I should have understood that without that explanation.
Thanks!

Uwe
Andy Shevchenko Nov. 25, 2024, 7:33 p.m. UTC | #6 
On Mon, Nov 25, 2024 at 4:52 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > +       /* Add one for temperature */
> > > > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> > > >
> > > > Is the type of both arguments the same?
> > >
> > > Hmm, my compiler is happy with it at least. I don't understand why
> > > though. I'll do a few more tests ...
> >
> > If num_channels is signed int or shorter than (independently on the
> > sign) int, then it's obvious why. + 1 makes it int.
>
> Ah indeed, I should have understood that without that explanation.

Yeah, but a closer look shows to me that num_channels is unsigned int
or did I look in the wrong place? If that's true, that should make a
warning appear since AD7124_MAX_CHANNELS is signed int...
Nuno Sá Nov. 27, 2024, 2:05 p.m. UTC | #7 
On Mon, 2024-11-25 at 21:33 +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 4:52 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> > > <u.kleine-koenig@baylibre.com> wrote:
> > > > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > > +       /* Add one for temperature */
> > > > > > +       st->num_channels = min(num_channels + 1,
> > > > > > AD7124_MAX_CHANNELS);
> > > > > 
> > > > > Is the type of both arguments the same?
> > > > 
> > > > Hmm, my compiler is happy with it at least. I don't understand why
> > > > though. I'll do a few more tests ...
> > > 
> > > If num_channels is signed int or shorter than (independently on the
> > > sign) int, then it's obvious why. + 1 makes it int.
> > 
> > Ah indeed, I should have understood that without that explanation.
> 
> Yeah, but a closer look shows to me that num_channels is unsigned int
> or did I look in the wrong place? If that's true, that should make a
> warning appear since AD7124_MAX_CHANNELS is signed int...
> 
> 

Hmm,

Weren't the min()/max() macros improved for things like this?

https://elixir.bootlin.com/linux/v6.12.1/source/include/linux/minmax.h#L22

- Nuno Sá
Uwe Kleine-König Nov. 27, 2024, 2:38 p.m. UTC | #8 
On Mon, Nov 25, 2024 at 09:33:12PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 4:52 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Mon, Nov 25, 2024 at 03:47:25PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 25, 2024 at 1:27 PM Uwe Kleine-König
> > > <u.kleine-koenig@baylibre.com> wrote:
> > > > On Fri, Nov 22, 2024 at 10:31:07PM +0200, Andy Shevchenko wrote:
> > > > > On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
> > > > > <u.kleine-koenig@baylibre.com> wrote:
> > > > > > +       /* Add one for temperature */
> > > > > > +       st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
> > > > >
> > > > > Is the type of both arguments the same?
> > > >
> > > > Hmm, my compiler is happy with it at least. I don't understand why
> > > > though. I'll do a few more tests ...
> > >
> > > If num_channels is signed int or shorter than (independently on the
> > > sign) int, then it's obvious why. + 1 makes it int.
> >
> > Ah indeed, I should have understood that without that explanation.
> 
> Yeah, but a closer look shows to me that num_channels is unsigned int
> or did I look in the wrong place? If that's true, that should make a
> warning appear since AD7124_MAX_CHANNELS is signed int...

The ideas in the definition of min are a bit hard to follow, but IIUC it
doesn't warn because AD7124_MAX_CHANNELS is non-negative and so there is
no danger for misinterpretation.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index fdbe2806bf11..3eff156b9c00 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -95,6 +95,10 @@ 
 #define AD7124_MAX_CONFIGS	8
 #define AD7124_MAX_CHANNELS	16
 
+/* AD7124 input sources */
+#define AD7124_INPUT_TEMPSENSOR	16
+#define AD7124_INPUT_AVSS	17
+
 enum ad7124_ids {
 	ID_AD7124_4,
 	ID_AD7124_8,
@@ -588,39 +592,75 @@  static int ad7124_read_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_SCALE:
-		mutex_lock(&st->cfgs_lock);
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			mutex_lock(&st->cfgs_lock);
 
-		idx = st->channels[chan->address].cfg.pga_bits;
-		*val = st->channels[chan->address].cfg.vref_mv;
-		if (st->channels[chan->address].cfg.bipolar)
-			*val2 = chan->scan_type.realbits - 1 + idx;
-		else
-			*val2 = chan->scan_type.realbits + idx;
+			idx = st->channels[chan->address].cfg.pga_bits;
+			*val = st->channels[chan->address].cfg.vref_mv;
+			if (st->channels[chan->address].cfg.bipolar)
+				*val2 = chan->scan_type.realbits - 1 + idx;
+			else
+				*val2 = chan->scan_type.realbits + idx;
+
+			mutex_unlock(&st->cfgs_lock);
+			return IIO_VAL_FRACTIONAL_LOG2;
+
+		case IIO_TEMP:
+			/*
+			 * According to the data sheet
+			 *   Temperature (°C)
+			 * = ((Conversion − 0x800000)/13584) − 272.5
+			 * = (Conversion − 0x800000 - 13584 * 272.5) / 13584
+			 * = (Conversion − 12090248) / 13584
+			 * So scale with 1000/13584 to yield °mC. Reduce by 8 to
+			 * 125/1698.
+			 */
+			*val = 125;
+			*val2 = 1698;
+			return IIO_VAL_FRACTIONAL;
+
+		default:
+			return -EINVAL;
+		}
 
-		mutex_unlock(&st->cfgs_lock);
-		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		mutex_lock(&st->cfgs_lock);
-		if (st->channels[chan->address].cfg.bipolar)
-			*val = -(1 << (chan->scan_type.realbits - 1));
-		else
-			*val = 0;
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			mutex_lock(&st->cfgs_lock);
+			if (st->channels[chan->address].cfg.bipolar)
+				*val = -(1 << (chan->scan_type.realbits - 1));
+			else
+				*val = 0;
+
+			mutex_unlock(&st->cfgs_lock);
+			return IIO_VAL_INT;
+
+		case IIO_TEMP:
+			/* see calculation above */
+			*val = -12090248;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
 
-		mutex_unlock(&st->cfgs_lock);
-		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		mutex_lock(&st->cfgs_lock);
 		*val = st->channels[chan->address].cfg.odr;
 		mutex_unlock(&st->cfgs_lock);
 
 		return IIO_VAL_INT;
+
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		mutex_lock(&st->cfgs_lock);
 		*val = ad7124_get_3db_filter_freq(st, chan->scan_index);
 		mutex_unlock(&st->cfgs_lock);
 
 		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -645,6 +685,7 @@  static int ad7124_write_raw(struct iio_dev *indio_dev,
 
 		ad7124_set_channel_odr(st, chan->address, val);
 		break;
+
 	case IIO_CHAN_INFO_SCALE:
 		if (val != 0) {
 			ret = -EINVAL;
@@ -666,6 +707,7 @@  static int ad7124_write_raw(struct iio_dev *indio_dev,
 
 		st->channels[chan->address].cfg.pga_bits = res;
 		break;
+
 	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
 		if (val2 != 0) {
 			ret = -EINVAL;
@@ -825,11 +867,10 @@  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	struct ad7124_channel *channels;
 	struct iio_chan_spec *chan;
 	unsigned int ain[2], channel = 0, tmp;
+	unsigned int num_channels;
 	int ret;
 
-	st->num_channels = device_get_child_node_count(dev);
-	if (!st->num_channels)
-		return dev_err_probe(dev, -ENODEV, "no channel children\n");
+	num_channels = device_get_child_node_count(dev);
 
 	/*
 	 * The driver assigns each logical channel defined in the device tree
@@ -838,9 +879,12 @@  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	 * CHANNEL_15) as an additional channel register. The driver could be
 	 * improved to lift this limitation.
 	 */
-	if (st->num_channels > AD7124_MAX_CHANNELS)
+	if (num_channels > AD7124_MAX_CHANNELS)
 		return dev_err_probe(dev, -EINVAL, "Too many channels defined\n");
 
+	/* Add one for temperature */
+	st->num_channels = min(num_channels + 1, AD7124_MAX_CHANNELS);
+
 	chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
 			    sizeof(*chan), GFP_KERNEL);
 	if (!chan)
@@ -861,7 +905,7 @@  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 			return dev_err_probe(dev, ret,
 					     "Failed to parse reg property of %pfwP\n", child);
 
-		if (channel >= indio_dev->num_channels)
+		if (channel >= num_channels)
 			return dev_err_probe(dev, -EINVAL,
 					     "Channel index >= number of channels in %pfwP\n", child);
 
@@ -901,6 +945,32 @@  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 		chan[channel].channel2 = ain[1];
 	}
 
+	if (num_channels < AD7124_MAX_CHANNELS) {
+		st->channels[num_channels] = (struct ad7124_channel) {
+			.nr = num_channels,
+			.ain = AD7124_CHANNEL_AINP(AD7124_INPUT_TEMPSENSOR) |
+				AD7124_CHANNEL_AINM(AD7124_INPUT_AVSS),
+			.cfg = {
+				.bipolar = true,
+			},
+		};
+
+		chan[num_channels] = (struct iio_chan_spec) {
+			.type = IIO_TEMP,
+			.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),
+			.scan_type = {
+				.sign = 'u',
+				.realbits = 24,
+				.storagebits = 32,
+				.endianness = IIO_BE,
+			},
+			.address = num_channels,
+			.scan_index = num_channels,
+		};
+	};
+
 	return 0;
 }