[PATCH v3 0/2] thermal: thermal-generic-adc: add temp sensor function

[PATCH v3 0/2] thermal: thermal-generic-adc: add temp sensor function

[Svyatoslav Ryhel]

Add IIO sensor cell to thermal-generic-adc, which would benefit
devices that use adc sensors to detect temperature and need a
custom conversion table.

---
Changes on switching from v2 to v3:
- rephrased commit headers

Changes on switching from v1 to v2:
- documented #iio-channel-cells property
- switched to IIO_CHAN_INFO_PROCESSED
---

Svyatoslav Ryhel (2):
  dt-bindings: thermal: generic-adc: Add optional io-channel-cells
    property
  thermal: thermal-generic-adc: add temperature sensor channel

 .../bindings/thermal/generic-adc-thermal.yaml |  4 ++
 drivers/thermal/thermal-generic-adc.c         | 54 ++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v3 1/2] dt-bindings: thermal: generic-adc: Add optional io-channel-cells property

[Svyatoslav Ryhel]

This implements a mechanism to derive temperature values from an existing ADC IIO
channel, effectively creating a temperature IIO channel. This approach avoids adding
a new sensor and its associated conversion table, while providing IIO-based temperature
data for devices that may not utilize hwmon.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../devicetree/bindings/thermal/generic-adc-thermal.yaml      | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
index 12e6418dc24d..4bc2cff0593c 100644
--- a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
@@ -30,6 +30,9 @@ properties:
   io-channel-names:
     const: sensor-channel
 
+  '#io-channel-cells':
+    const: 1
+
   temperature-lookup-table:
     description: |
       Lookup table to map the relation between ADC value and temperature.
@@ -60,6 +63,7 @@ examples:
         #thermal-sensor-cells = <0>;
         io-channels = <&ads1015 1>;
         io-channel-names = "sensor-channel";
+        #io-channel-cells = <1>;
         temperature-lookup-table = <
               (-40000) 2578
               (-39000) 2577
-- 
2.43.0

Re: [PATCH v3 1/2] dt-bindings: thermal: generic-adc: Add optional io-channel-cells property

[Lukasz Luba]

On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> This implements a mechanism to derive temperature values from an existing ADC IIO
> channel, effectively creating a temperature IIO channel. This approach avoids adding
> a new sensor and its associated conversion table, while providing IIO-based temperature
> data for devices that may not utilize hwmon.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>   .../devicetree/bindings/thermal/generic-adc-thermal.yaml      | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> index 12e6418dc24d..4bc2cff0593c 100644
> --- a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> @@ -30,6 +30,9 @@ properties:
>     io-channel-names:
>       const: sensor-channel
>   
> +  '#io-channel-cells':
> +    const: 1
> +
>     temperature-lookup-table:
>       description: |
>         Lookup table to map the relation between ADC value and temperature.
> @@ -60,6 +63,7 @@ examples:
>           #thermal-sensor-cells = <0>;
>           io-channels = <&ads1015 1>;
>           io-channel-names = "sensor-channel";
> +        #io-channel-cells = <1>;
>           temperature-lookup-table = <
>                 (-40000) 2578
>                 (-39000) 2577

Do we really need this change in the DT?
Won't the code in the thermal driver that registers a new iio device
would just be enough?

I agree with Rob that it looks odd.

Re: [PATCH v3 1/2] dt-bindings: thermal: generic-adc: Add optional io-channel-cells property

[Svyatoslav Ryhel]

ср, 5 бер. 2025 р. о 12:00 Lukasz Luba <lukasz.luba@arm.com> пише:
>
>
>
> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> > This implements a mechanism to derive temperature values from an existing ADC IIO
> > channel, effectively creating a temperature IIO channel. This approach avoids adding
> > a new sensor and its associated conversion table, while providing IIO-based temperature
> > data for devices that may not utilize hwmon.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >   .../devicetree/bindings/thermal/generic-adc-thermal.yaml      | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> > index 12e6418dc24d..4bc2cff0593c 100644
> > --- a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> > +++ b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> > @@ -30,6 +30,9 @@ properties:
> >     io-channel-names:
> >       const: sensor-channel
> >
> > +  '#io-channel-cells':
> > +    const: 1
> > +
> >     temperature-lookup-table:
> >       description: |
> >         Lookup table to map the relation between ADC value and temperature.
> > @@ -60,6 +63,7 @@ examples:
> >           #thermal-sensor-cells = <0>;
> >           io-channels = <&ads1015 1>;
> >           io-channel-names = "sensor-channel";
> > +        #io-channel-cells = <1>;
> >           temperature-lookup-table = <
> >                 (-40000) 2578
> >                 (-39000) 2577
>
> Do we really need this change in the DT?
> Won't the code in the thermal driver that registers a new iio device
> would just be enough?
>
> I agree with Rob that it looks odd.

Building tree will complain on missing cells property if you try to
bind it. It is not in required category anyway.

Re: [PATCH v3 1/2] dt-bindings: thermal: generic-adc: Add optional io-channel-cells property

[Lukasz Luba]

On 3/5/25 10:03, Svyatoslav Ryhel wrote:
> ср, 5 бер. 2025 р. о 12:00 Lukasz Luba <lukasz.luba@arm.com> пише:
>>
>>
>>
>> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
>>> This implements a mechanism to derive temperature values from an existing ADC IIO
>>> channel, effectively creating a temperature IIO channel. This approach avoids adding
>>> a new sensor and its associated conversion table, while providing IIO-based temperature
>>> data for devices that may not utilize hwmon.
>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>    .../devicetree/bindings/thermal/generic-adc-thermal.yaml      | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
>>> index 12e6418dc24d..4bc2cff0593c 100644
>>> --- a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
>>> @@ -30,6 +30,9 @@ properties:
>>>      io-channel-names:
>>>        const: sensor-channel
>>>
>>> +  '#io-channel-cells':
>>> +    const: 1
>>> +
>>>      temperature-lookup-table:
>>>        description: |
>>>          Lookup table to map the relation between ADC value and temperature.
>>> @@ -60,6 +63,7 @@ examples:
>>>            #thermal-sensor-cells = <0>;
>>>            io-channels = <&ads1015 1>;
>>>            io-channel-names = "sensor-channel";
>>> +        #io-channel-cells = <1>;
>>>            temperature-lookup-table = <
>>>                  (-40000) 2578
>>>                  (-39000) 2577
>>
>> Do we really need this change in the DT?
>> Won't the code in the thermal driver that registers a new iio device
>> would just be enough?
>>
>> I agree with Rob that it looks odd.
> 
> Building tree will complain on missing cells property if you try to
> bind it. It is not in required category anyway.

Fair enough, then I will leave it to DT folks.

For me it's looks OK-ish, assuming you add this property with the
comment describing it like Rob asked.

UUIC it will always be only 1 channel - with that temperature,
so shouldn't harm much the design in DT.

Re: [PATCH v3 1/2] dt-bindings: thermal: generic-adc: Add optional io-channel-cells property

[Rob Herring]

On Wed, Mar 05, 2025 at 12:03:20PM +0200, Svyatoslav Ryhel wrote:
> ср, 5 бер. 2025 р. о 12:00 Lukasz Luba <lukasz.luba@arm.com> пише:
> >
> >
> >
> > On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> > > This implements a mechanism to derive temperature values from an existing ADC IIO
> > > channel, effectively creating a temperature IIO channel. This approach avoids adding
> > > a new sensor and its associated conversion table, while providing IIO-based temperature
> > > data for devices that may not utilize hwmon.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >   .../devicetree/bindings/thermal/generic-adc-thermal.yaml      | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> > > index 12e6418dc24d..4bc2cff0593c 100644
> > > --- a/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> > > +++ b/Documentation/devicetree/bindings/thermal/generic-adc-thermal.yaml
> > > @@ -30,6 +30,9 @@ properties:
> > >     io-channel-names:
> > >       const: sensor-channel
> > >
> > > +  '#io-channel-cells':
> > > +    const: 1
> > > +
> > >     temperature-lookup-table:
> > >       description: |
> > >         Lookup table to map the relation between ADC value and temperature.
> > > @@ -60,6 +63,7 @@ examples:
> > >           #thermal-sensor-cells = <0>;
> > >           io-channels = <&ads1015 1>;
> > >           io-channel-names = "sensor-channel";
> > > +        #io-channel-cells = <1>;
> > >           temperature-lookup-table = <
> > >                 (-40000) 2578
> > >                 (-39000) 2577
> >
> > Do we really need this change in the DT?
> > Won't the code in the thermal driver that registers a new iio device
> > would just be enough?
> >
> > I agree with Rob that it looks odd.
> 
> Building tree will complain on missing cells property if you try to
> bind it. It is not in required category anyway.

Sorry, I don't follow nor see why you need the property if there are no 
DT consumers.

Rob

[PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Svyatoslav Ryhel]

To avoid duplicating sensor functionality and conversion tables, this design
allows converting an ADC IIO channel's output directly into a temperature IIO
channel. This is particularly useful for devices where hwmon isn't suitable
or where temperature data must be accessible through IIO.

One such device is, for example, the MAX17040 fuel gauge.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
index ee3d0aa31406..a8f3b965b39b 100644
--- a/drivers/thermal/thermal-generic-adc.c
+++ b/drivers/thermal/thermal-generic-adc.c
@@ -7,6 +7,7 @@
  * Author: Laxman Dewangan <ldewangan@nvidia.com>
  */
 #include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -73,6 +74,57 @@ static const struct thermal_zone_device_ops gadc_thermal_ops = {
 	.get_temp = gadc_thermal_get_temp,
 };
 
+static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+	}
+};
+
+static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *temp, int *val2, long mask)
+{
+	struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_PROCESSED)
+		return -EINVAL;
+
+	ret = gadc_thermal_get_temp(gtinfo->tz_dev, temp);
+	if (ret < 0)
+		return ret;
+
+	*temp /= 1000;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info gadc_thermal_iio_info = {
+	.read_raw = gadc_thermal_read_raw,
+};
+
+static int gadc_iio_register(struct device *dev, struct gadc_thermal_info *gti)
+{
+	struct gadc_thermal_info *gtinfo;
+	struct iio_dev *indio_dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct gadc_thermal_info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	gtinfo = iio_priv(indio_dev);
+	memcpy(gtinfo, gti, sizeof(struct gadc_thermal_info));
+
+	indio_dev->name = dev_name(dev);
+	indio_dev->info = &gadc_thermal_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = gadc_thermal_iio_channel;
+	indio_dev->num_channels = ARRAY_SIZE(gadc_thermal_iio_channel);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
 static int gadc_thermal_read_linear_lookup_table(struct device *dev,
 						 struct gadc_thermal_info *gti)
 {
@@ -153,7 +205,7 @@ static int gadc_thermal_probe(struct platform_device *pdev)
 
 	devm_thermal_add_hwmon_sysfs(dev, gti->tz_dev);
 
-	return 0;
+	return gadc_iio_register(&pdev->dev, gti);
 }
 
 static const struct of_device_id of_adc_thermal_match[] = {
-- 
2.43.0

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Lukasz Luba]

On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> To avoid duplicating sensor functionality and conversion tables, this design
> allows converting an ADC IIO channel's output directly into a temperature IIO
> channel. This is particularly useful for devices where hwmon isn't suitable
> or where temperature data must be accessible through IIO.
> 
> One such device is, for example, the MAX17040 fuel gauge.
> 
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>   drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
>   1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> index ee3d0aa31406..a8f3b965b39b 100644
> --- a/drivers/thermal/thermal-generic-adc.c
> +++ b/drivers/thermal/thermal-generic-adc.c
> @@ -7,6 +7,7 @@
>    * Author: Laxman Dewangan <ldewangan@nvidia.com>
>    */
>   #include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> @@ -73,6 +74,57 @@ static const struct thermal_zone_device_ops gadc_thermal_ops = {
>   	.get_temp = gadc_thermal_get_temp,
>   };
>   
> +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),

I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.

> +	}
> +};
> +
> +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan,
> +				 int *temp, int *val2, long mask)
> +{
> +	struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_PROCESSED)
> +		return -EINVAL;

Therefore, here it would need to handle such case as well, when
a client is asking about scale.

> +
> +	ret = gadc_thermal_get_temp(gtinfo->tz_dev, temp);
> +	if (ret < 0)
> +		return ret;
> +
> +	*temp /= 1000;

IMO we shouldn't cut the precision if it's provided.
The user of this would know what to do with the value (when
the proper information about scale is also available).

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info gadc_thermal_iio_info = {
> +	.read_raw = gadc_thermal_read_raw,
> +};
> +
> +static int gadc_iio_register(struct device *dev, struct gadc_thermal_info *gti)
> +{
> +	struct gadc_thermal_info *gtinfo;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct gadc_thermal_info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	gtinfo = iio_priv(indio_dev);
> +	memcpy(gtinfo, gti, sizeof(struct gadc_thermal_info));
> +
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->info = &gadc_thermal_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = gadc_thermal_iio_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(gadc_thermal_iio_channel);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
>   static int gadc_thermal_read_linear_lookup_table(struct device *dev,
>   						 struct gadc_thermal_info *gti)
>   {
> @@ -153,7 +205,7 @@ static int gadc_thermal_probe(struct platform_device *pdev)
>   
>   	devm_thermal_add_hwmon_sysfs(dev, gti->tz_dev);
>   
> -	return 0;
> +	return gadc_iio_register(&pdev->dev, gti);
>   }
>   
>   static const struct of_device_id of_adc_thermal_match[] = {

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Svyatoslav Ryhel]

ср, 5 бер. 2025 р. о 11:52 Lukasz Luba <lukasz.luba@arm.com> пише:
>
>
>
> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> > To avoid duplicating sensor functionality and conversion tables, this design
> > allows converting an ADC IIO channel's output directly into a temperature IIO
> > channel. This is particularly useful for devices where hwmon isn't suitable
> > or where temperature data must be accessible through IIO.
> >
> > One such device is, for example, the MAX17040 fuel gauge.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >   drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
> >   1 file changed, 53 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
...
> >
> > +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
> > +     {
> > +             .type = IIO_TEMP,
> > +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>
> I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.
>

I have hit this issue already with als sensor. This should definitely
be a IIO_CHAN_INFO_PROCESSED since there is no raw temp data we have,
it gets processed into temp data via conversion table. I will add
Jonathan Cameron to list if you don't mind, he might give some good
advice.

> > +     }
> > +};
> > +
> > +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> > +                              struct iio_chan_spec const *chan,
> > +                              int *temp, int *val2, long mask)
> > +{
> > +     struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     if (mask != IIO_CHAN_INFO_PROCESSED)
> > +             return -EINVAL;
>
> Therefore, here it would need to handle such case as well, when
> a client is asking about scale.
>
> > +
> > +     ret = gadc_thermal_get_temp(gtinfo->tz_dev, temp);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     *temp /= 1000;
>
> IMO we shouldn't cut the precision if it's provided.
> The user of this would know what to do with the value (when
> the proper information about scale is also available).
>

The it will not fit existing IIO framework and thermal readings will
be 1000 off. I have had to adjust this since my battery suddenly got
temperature reading of 23200C which obviously was not true. With
adjustment temperature will be in 10th of C (yes, odd, I know but it
is what it is).

> > +
> > +     return IIO_VAL_INT;

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Lukasz Luba]

On 3/5/25 10:06, Svyatoslav Ryhel wrote:
> ср, 5 бер. 2025 р. о 11:52 Lukasz Luba <lukasz.luba@arm.com> пише:
>>
>>
>>
>> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
>>> To avoid duplicating sensor functionality and conversion tables, this design
>>> allows converting an ADC IIO channel's output directly into a temperature IIO
>>> channel. This is particularly useful for devices where hwmon isn't suitable
>>> or where temperature data must be accessible through IIO.
>>>
>>> One such device is, for example, the MAX17040 fuel gauge.
>>>
>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>> ---
>>>    drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
>>>    1 file changed, 53 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> ...
>>>
>>> +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
>>> +     {
>>> +             .type = IIO_TEMP,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>
>> I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.
>>
> 
> I have hit this issue already with als sensor. This should definitely
> be a IIO_CHAN_INFO_PROCESSED since there is no raw temp data we have,
> it gets processed into temp data via conversion table. I will add
> Jonathan Cameron to list if you don't mind, he might give some good
> advice.

I'm not talking about 'PROCESSED' vs 'RAW'...
I'm asking if you can add the 'SCALE' case to handle and report
that this device will report 'processed' temp value in milli-degrees
of Celsius.

> 
>>> +     }
>>> +};
>>> +
>>> +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
>>> +                              struct iio_chan_spec const *chan,
>>> +                              int *temp, int *val2, long mask)
>>> +{
>>> +     struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     if (mask != IIO_CHAN_INFO_PROCESSED)
>>> +             return -EINVAL;
>>
>> Therefore, here it would need to handle such case as well, when
>> a client is asking about scale.
>>
>>> +
>>> +     ret = gadc_thermal_get_temp(gtinfo->tz_dev, temp);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     *temp /= 1000;
>>
>> IMO we shouldn't cut the precision if it's provided.
>> The user of this would know what to do with the value (when
>> the proper information about scale is also available).
>>
> 
> The it will not fit existing IIO framework and thermal readings will
> be 1000 off. I have had to adjust this since my battery suddenly got
> temperature reading of 23200C which obviously was not true. With
> adjustment temperature will be in 10th of C (yes, odd, I know but it
> is what it is).

Your battery driver should get and check the 'SCALE' info first, then
it will know that the value is in higher resolution than it needs.
Therefore, it can divide the value inside its code.
Your proposed division here is creating a limitation.

You shouldn't force all other drivers to ignore and drop the
available information about milli-degC (which is done in this patch).

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Svyatoslav Ryhel]

ср, 5 бер. 2025 р. о 16:37 Lukasz Luba <lukasz.luba@arm.com> пише:
>
>
>
> On 3/5/25 10:06, Svyatoslav Ryhel wrote:
> > ср, 5 бер. 2025 р. о 11:52 Lukasz Luba <lukasz.luba@arm.com> пише:
> >>
> >>
> >>
> >> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> >>> To avoid duplicating sensor functionality and conversion tables, this design
> >>> allows converting an ADC IIO channel's output directly into a temperature IIO
> >>> channel. This is particularly useful for devices where hwmon isn't suitable
> >>> or where temperature data must be accessible through IIO.
> >>>
> >>> One such device is, for example, the MAX17040 fuel gauge.
> >>>
> >>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >>> ---
> >>>    drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
> >>>    1 file changed, 53 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> > ...
> >>>
> >>> +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
> >>> +     {
> >>> +             .type = IIO_TEMP,
> >>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> >>
> >> I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.
> >>
> >
> > I have hit this issue already with als sensor. This should definitely
> > be a IIO_CHAN_INFO_PROCESSED since there is no raw temp data we have,
> > it gets processed into temp data via conversion table. I will add
> > Jonathan Cameron to list if you don't mind, he might give some good
> > advice.
>
> I'm not talking about 'PROCESSED' vs 'RAW'...
> I'm asking if you can add the 'SCALE' case to handle and report
> that this device will report 'processed' temp value in milli-degrees
> of Celsius.
>

Sure, I take this into account.

> >
> >>> +     }
> >>> +};
> >>> +
> >>> +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> >>> +                              struct iio_chan_spec const *chan,
> >>> +                              int *temp, int *val2, long mask)
> >>> +{
> >>> +     struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> >>> +     int ret;
> >>> +
> >>> +     if (mask != IIO_CHAN_INFO_PROCESSED)
> >>> +             return -EINVAL;
> >>
> >> Therefore, here it would need to handle such case as well, when
> >> a client is asking about scale.
> >>
> >>> +
> >>> +     ret = gadc_thermal_get_temp(gtinfo->tz_dev, temp);
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +
> >>> +     *temp /= 1000;
> >>
> >> IMO we shouldn't cut the precision if it's provided.
> >> The user of this would know what to do with the value (when
> >> the proper information about scale is also available).
> >>
> >
> > The it will not fit existing IIO framework and thermal readings will
> > be 1000 off. I have had to adjust this since my battery suddenly got
> > temperature reading of 23200C which obviously was not true. With
> > adjustment temperature will be in 10th of C (yes, odd, I know but it
> > is what it is).
>
> Your battery driver should get and check the 'SCALE' info first, then
> it will know that the value is in higher resolution than it needs.
> Therefore, it can divide the value inside its code.
> Your proposed division here is creating a limitation.
>
> You shouldn't force all other drivers to ignore and drop the
> available information about milli-degC (which is done in this patch).

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Svyatoslav Ryhel]

ср, 5 бер. 2025 р. о 16:37 Lukasz Luba <lukasz.luba@arm.com> пише:
>
>
>
> On 3/5/25 10:06, Svyatoslav Ryhel wrote:
> > ср, 5 бер. 2025 р. о 11:52 Lukasz Luba <lukasz.luba@arm.com> пише:
> >>
> >>
> >>
> >> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
> >>> To avoid duplicating sensor functionality and conversion tables, this design
> >>> allows converting an ADC IIO channel's output directly into a temperature IIO
> >>> channel. This is particularly useful for devices where hwmon isn't suitable
> >>> or where temperature data must be accessible through IIO.
> >>>
> >>> One such device is, for example, the MAX17040 fuel gauge.
> >>>
> >>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >>> ---
> >>>    drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
> >>>    1 file changed, 53 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> > ...
> >>>
> >>> +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
> >>> +     {
> >>> +             .type = IIO_TEMP,
> >>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> >>
> >> I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.
> >>
> >
> > I have hit this issue already with als sensor. This should definitely
> > be a IIO_CHAN_INFO_PROCESSED since there is no raw temp data we have,
> > it gets processed into temp data via conversion table. I will add
> > Jonathan Cameron to list if you don't mind, he might give some good
> > advice.
>
> I'm not talking about 'PROCESSED' vs 'RAW'...
> I'm asking if you can add the 'SCALE' case to handle and report
> that this device will report 'processed' temp value in milli-degrees
> of Celsius.
>

It seems that SCALE is not applied to PROCESSED channel. I can use RAW
which would work as intended and I will add a note in commit
description why I used RAW. Would that be acceptable?

> >
> >>> +     }
> >>> +};
> >>> +
> >>> +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> >>> +                              struct iio_chan_spec const *chan,
> >>> +                              int *temp, int *val2, long mask)
> >>> +{
> >>> +     struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> >>> +     int ret;
> >>> +
> >>> +     if (mask != IIO_CHAN_INFO_PROCESSED)
> >>> +             return -EINVAL;
> >>
> >> Therefore, here it would need to handle such case as well, when
> >> a client is asking about scale.
> >>
> >>> +
> >>> +     ret = gadc_thermal_get_temp(gtinfo->tz_dev, temp);
> >>> +     if (ret < 0)
> >>> +             return ret;
> >>> +
> >>> +     *temp /= 1000;
> >>
> >> IMO we shouldn't cut the precision if it's provided.
> >> The user of this would know what to do with the value (when
> >> the proper information about scale is also available).
> >>
> >
> > The it will not fit existing IIO framework and thermal readings will
> > be 1000 off. I have had to adjust this since my battery suddenly got
> > temperature reading of 23200C which obviously was not true. With
> > adjustment temperature will be in 10th of C (yes, odd, I know but it
> > is what it is).
>
> Your battery driver should get and check the 'SCALE' info first, then
> it will know that the value is in higher resolution than it needs.
> Therefore, it can divide the value inside its code.
> Your proposed division here is creating a limitation.
>
> You shouldn't force all other drivers to ignore and drop the
> available information about milli-degC (which is done in this patch).

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Lukasz Luba]

On 3/6/25 09:49, Svyatoslav Ryhel wrote:
> ср, 5 бер. 2025 р. о 16:37 Lukasz Luba <lukasz.luba@arm.com> пише:
>>
>>
>>
>> On 3/5/25 10:06, Svyatoslav Ryhel wrote:
>>> ср, 5 бер. 2025 р. о 11:52 Lukasz Luba <lukasz.luba@arm.com> пише:
>>>>
>>>>
>>>>
>>>> On 3/3/25 12:21, Svyatoslav Ryhel wrote:
>>>>> To avoid duplicating sensor functionality and conversion tables, this design
>>>>> allows converting an ADC IIO channel's output directly into a temperature IIO
>>>>> channel. This is particularly useful for devices where hwmon isn't suitable
>>>>> or where temperature data must be accessible through IIO.
>>>>>
>>>>> One such device is, for example, the MAX17040 fuel gauge.
>>>>>
>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
>>>>> ---
>>>>>     drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
>>>>>     1 file changed, 53 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
>>> ...
>>>>>
>>>>> +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
>>>>> +     {
>>>>> +             .type = IIO_TEMP,
>>>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>>
>>>> I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.
>>>>
>>>
>>> I have hit this issue already with als sensor. This should definitely
>>> be a IIO_CHAN_INFO_PROCESSED since there is no raw temp data we have,
>>> it gets processed into temp data via conversion table. I will add
>>> Jonathan Cameron to list if you don't mind, he might give some good
>>> advice.
>>
>> I'm not talking about 'PROCESSED' vs 'RAW'...
>> I'm asking if you can add the 'SCALE' case to handle and report
>> that this device will report 'processed' temp value in milli-degrees
>> of Celsius.
>>
> 
> It seems that SCALE is not applied to PROCESSED channel. I can use RAW
> which would work as intended and I will add a note in commit
> description why I used RAW. Would that be acceptable?
> 

In that case, yes that would be the preferred solution.

Re: [PATCH v3 2/2] thermal: thermal-generic-adc: add temperature sensor channel

[Jonathan Cameron]

On Thu, 6 Mar 2025 10:04:01 +0000
Lukasz Luba <lukasz.luba@arm.com> wrote:

> On 3/6/25 09:49, Svyatoslav Ryhel wrote:
> > ср, 5 бер. 2025 р. о 16:37 Lukasz Luba <lukasz.luba@arm.com> пише:  
> >>
> >>
> >>
> >> On 3/5/25 10:06, Svyatoslav Ryhel wrote:  
> >>> ср, 5 бер. 2025 р. о 11:52 Lukasz Luba <lukasz.luba@arm.com> пише:  
> >>>>
> >>>>
> >>>>
> >>>> On 3/3/25 12:21, Svyatoslav Ryhel wrote:  
> >>>>> To avoid duplicating sensor functionality and conversion tables, this design
> >>>>> allows converting an ADC IIO channel's output directly into a temperature IIO
> >>>>> channel. This is particularly useful for devices where hwmon isn't suitable
> >>>>> or where temperature data must be accessible through IIO.
> >>>>>
> >>>>> One such device is, for example, the MAX17040 fuel gauge.
> >>>>>
> >>>>> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> >>>>> ---
> >>>>>     drivers/thermal/thermal-generic-adc.c | 54 ++++++++++++++++++++++++++-
> >>>>>     1 file changed, 53 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c  
> >>> ...  
> >>>>>
> >>>>> +static const struct iio_chan_spec gadc_thermal_iio_channel[] = {
> >>>>> +     {
> >>>>> +             .type = IIO_TEMP,
> >>>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),  
> >>>>
> >>>> I would add the IIO_CHAN_INFO_SCALE and say it's in milli-degrees.
> >>>>  
> >>>
> >>> I have hit this issue already with als sensor. This should definitely
> >>> be a IIO_CHAN_INFO_PROCESSED since there is no raw temp data we have,
> >>> it gets processed into temp data via conversion table. I will add
> >>> Jonathan Cameron to list if you don't mind, he might give some good
> >>> advice.  
> >>
> >> I'm not talking about 'PROCESSED' vs 'RAW'...
> >> I'm asking if you can add the 'SCALE' case to handle and report
> >> that this device will report 'processed' temp value in milli-degrees
> >> of Celsius.
> >>  
> > 
> > It seems that SCALE is not applied to PROCESSED channel. I can use RAW
> > which would work as intended and I will add a note in commit
> > description why I used RAW. Would that be acceptable?

Indeed. SCALE is only about RAW channels because if they are processed
you have already applied the scale (typically because it wasn't linear)

> >   
> 
> In that case, yes that would be the preferred solution.

I nearly missed this entirely as it was buried in my unfiltered email.
Thanks for the +CC.

Given this is a IIO driver (be it in thermal)
please +CC linux-iio@vger.kernel.org to get review of that part of it.

Note, in general if you do a driver out of subsystem (and there
are good reasons to do that!) please +CC the other subsystem and
maintainers as well. We do that for IIO drivers that have a gpio
chip for instance. I specifically check they are +CC and wait for
an Ack before merging such drivers.

Thanks,

Jonathan