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

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

[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.

The temperature data, while technically a product of conversion and thus
categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
(milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.

---
Changes on switching from v3 to v4:
- switch to use of RAW and SCALED channels to provide more accurate data

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         | 62 ++++++++++++++++++-
 2 files changed, 65 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v4 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

[PATCH v4 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.

The temperature data, while technically a product of conversion and thus
categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
(milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.

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

diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
index ee3d0aa31406..7dcc2e1168a4 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,65 @@ 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_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	}
+};
+
+static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan,
+				 int *val, int *val2, long mask)
+{
+	struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = gadc_thermal_get_temp(gtinfo->tz_dev, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		/* scale to a degree centigrade */
+		*val = 1;
+		*val2 = 1000;
+		return IIO_VAL_FRACTIONAL;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+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 +213,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 v4 1/2] dt-bindings: thermal: generic-adc: Add optional io-channel-cells property

[Krzysztof Kozlowski]

On 10/03/2025 08:56, 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.
> 

You got comments from Rob few days ago to which you did not respond.
Instead you decided to send next version, which hides the previous
unresolved discussion.

This patch does not look necessary based on earlier discussion.

Best regards,
Krzysztof

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

[Svyatoslav Ryhel]

вт, 11 бер. 2025 р. о 10:20 Krzysztof Kozlowski <krzk@kernel.org> пише:
>
> On 10/03/2025 08:56, 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.
> >
>
> You got comments from Rob few days ago to which you did not respond.
> Instead you decided to send next version, which hides the previous
> unresolved discussion.
>
> This patch does not look necessary based on earlier discussion.
>

ok, I will remove this schema change

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

[Svyatoslav Ryhel]

пн, 10 бер. 2025 р. о 09:57 Svyatoslav Ryhel <clamor95@gmail.com> пише:
>
> 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.
>
> The temperature data, while technically a product of conversion and thus
> categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
> (milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
> IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/thermal/thermal-generic-adc.c | 62 ++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> index ee3d0aa31406..7dcc2e1168a4 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,65 @@ 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_RAW) |
> +                                     BIT(IIO_CHAN_INFO_SCALE),
> +       }
> +};
> +
> +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> +                                struct iio_chan_spec const *chan,
> +                                int *val, int *val2, long mask)
> +{
> +       struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = gadc_thermal_get_temp(gtinfo->tz_dev, val);
> +               if (ret)
> +                       return ret;
> +
> +               return IIO_VAL_INT;
> +
> +       case IIO_CHAN_INFO_SCALE:
> +               /* scale to a degree centigrade */
> +               *val = 1;
> +               *val2 = 1000;
> +               return IIO_VAL_FRACTIONAL;
> +
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +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 +213,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
>

Added Jonathan Cameron and linux-iio@vger.kernel.org to list.

Jonathan, this is newer version of the thermal-generic-adc you have
reviewed recently with channels adjusted like proposed in v3.

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

[Jonathan Cameron]

On Sat, 5 Apr 2025 18:23:25 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> пн, 10 бер. 2025 р. о 09:57 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> >
> > 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.
> >
> > The temperature data, while technically a product of conversion and thus
> > categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
> > (milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
> > IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.

You've lost me in this description.  The base units of an IIO temperature channel
are milli-degrees so if the scaling is already right for that you would
be fine using a IIO_CHAN_INFO_PROCESSED channel.

A few other minor things inline.

> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  drivers/thermal/thermal-generic-adc.c | 62 ++++++++++++++++++++++++++-
> >  1 file changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> > index ee3d0aa31406..7dcc2e1168a4 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,65 @@ 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[] = {
Even though there is only one. If it is an array use channels.

or stop it being an array and just take a pointer to a single channel
instance.

> > +       {
> > +               .type = IIO_TEMP,
> > +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +                                     BIT(IIO_CHAN_INFO_SCALE),
> > +       }
> > +};
> > +
> > +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> > +                                struct iio_chan_spec const *chan,
> > +                                int *val, int *val2, long mask)
> > +{
> > +       struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> > +       int ret;
> > +
> > +       switch (mask) {
> > +       case IIO_CHAN_INFO_RAW:
> > +               ret = gadc_thermal_get_temp(gtinfo->tz_dev, val);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               return IIO_VAL_INT;
> > +
> > +       case IIO_CHAN_INFO_SCALE:
> > +               /* scale to a degree centigrade */

As above.  See Documentation/ABI/testing/sysfs-bus-iio
entries from temperature. Scaling of a temperature channel is milli-degrees

This is a bit of a historical artefact. Way back at the start of IIO
when we had relatively few channel types, where possible I matched the
scaling to hwmon.  With hindsight that made things a bit inconsistent
but we are stuck with it as ABI :(

Jonathan

> > +               *val = 1;
> > +               *val2 = 1000;
> > +               return IIO_VAL_FRACTIONAL;
> > +
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +}
> > +
> > +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));

sizeof(*gtinfo) probably slightly better.

> > +
> > +       indio_dev->name = dev_name(dev);

What does this end up as?  The convention in IIO is to name after
a part number.  If you have duplicates this isn't how you tell them
apart.  So I'd kind of expect thermal-generic-temp or
something like that.

> > +       indio_dev->info = &gadc_thermal_iio_info;
> > +       indio_dev->modes = INDIO_DIRECT_MODE;
> > +       indio_dev->channels = gadc_thermal_iio_channel;
As above, I'd stop that being an array and use
	indio_dev->channels = &gadc_thermal_iio_chanel;
	indio_dev->channels = 1;

Unless you think maybe we will get more channels in future, in which case
just rename it channels (which happens to have one element this time)

> > +       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 +213,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
> >  
> 
> Added Jonathan Cameron and linux-iio@vger.kernel.org to list.
> 
> Jonathan, this is newer version of the thermal-generic-adc you have
> reviewed recently with channels adjusted like proposed in v3.

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

[Svyatoslav Ryhel]

сб, 12 квіт. 2025 р. о 13:54 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Sat, 5 Apr 2025 18:23:25 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > пн, 10 бер. 2025 р. о 09:57 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > >
> > > 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.
> > >
> > > The temperature data, while technically a product of conversion and thus
> > > categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
> > > (milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
> > > IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.
>
> You've lost me in this description.  The base units of an IIO temperature channel
> are milli-degrees so if the scaling is already right for that you would
> be fine using a IIO_CHAN_INFO_PROCESSED channel.
>
> A few other minor things inline.
>
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/thermal/thermal-generic-adc.c | 62 ++++++++++++++++++++++++++-
> > >  1 file changed, 61 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> > > index ee3d0aa31406..7dcc2e1168a4 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,65 @@ 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[] = {
> Even though there is only one. If it is an array use channels.
>
> or stop it being an array and just take a pointer to a single channel
> instance.
>

There should not be more channels, but tbh, you never know, I will
rename to channels.

> > > +       {
> > > +               .type = IIO_TEMP,
> > > +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > +                                     BIT(IIO_CHAN_INFO_SCALE),
> > > +       }
> > > +};
> > > +
> > > +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> > > +                                struct iio_chan_spec const *chan,
> > > +                                int *val, int *val2, long mask)
> > > +{
> > > +       struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> > > +       int ret;
> > > +
> > > +       switch (mask) {
> > > +       case IIO_CHAN_INFO_RAW:
> > > +               ret = gadc_thermal_get_temp(gtinfo->tz_dev, val);
> > > +               if (ret)
> > > +                       return ret;
> > > +
> > > +               return IIO_VAL_INT;
> > > +
> > > +       case IIO_CHAN_INFO_SCALE:
> > > +               /* scale to a degree centigrade */
>
> As above.  See Documentation/ABI/testing/sysfs-bus-iio
> entries from temperature. Scaling of a temperature channel is milli-degrees
>
> This is a bit of a historical artefact. Way back at the start of IIO
> when we had relatively few channel types, where possible I matched the
> scaling to hwmon.  With hindsight that made things a bit inconsistent
> but we are stuck with it as ABI :(
>

RAW channel is in milli-degrees, or this is not enough? I don't get
your point here tbh. What is wrong? Battery driver requires
temperature in degree centigrade hence it is scaled to it.

> Jonathan
>
> > > +               *val = 1;
> > > +               *val2 = 1000;
> > > +               return IIO_VAL_FRACTIONAL;
> > > +
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > > +}
> > > +
> > > +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));
>
> sizeof(*gtinfo) probably slightly better.
>
> > > +
> > > +       indio_dev->name = dev_name(dev);
>
> What does this end up as?  The convention in IIO is to name after
> a part number.  If you have duplicates this isn't how you tell them
> apart.  So I'd kind of expect thermal-generic-temp or
> something like that.
>

it is "generic-adc-thermal" with this name, it is not present anywhere in IIO.

> > > +       indio_dev->info = &gadc_thermal_iio_info;
> > > +       indio_dev->modes = INDIO_DIRECT_MODE;
> > > +       indio_dev->channels = gadc_thermal_iio_channel;
> As above, I'd stop that being an array and use
>         indio_dev->channels = &gadc_thermal_iio_chanel;
>         indio_dev->channels = 1;
>
> Unless you think maybe we will get more channels in future, in which case
> just rename it channels (which happens to have one element this time)
>
> > > +       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 +213,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
> > >
> >
> > Added Jonathan Cameron and linux-iio@vger.kernel.org to list.
> >
> > Jonathan, this is newer version of the thermal-generic-adc you have
> > reviewed recently with channels adjusted like proposed in v3.
>

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

[Jonathan Cameron]

On Sat, 12 Apr 2025 14:06:06 +0300
Svyatoslav Ryhel <clamor95@gmail.com> wrote:

> сб, 12 квіт. 2025 р. о 13:54 Jonathan Cameron <jic23@kernel.org> пише:
> >
> > On Sat, 5 Apr 2025 18:23:25 +0300
> > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >  
> > > пн, 10 бер. 2025 р. о 09:57 Svyatoslav Ryhel <clamor95@gmail.com> пише:  
> > > >
> > > > 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.
> > > >
> > > > The temperature data, while technically a product of conversion and thus
> > > > categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
> > > > (milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
> > > > IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.  
> >
> > You've lost me in this description.  The base units of an IIO temperature channel
> > are milli-degrees so if the scaling is already right for that you would
> > be fine using a IIO_CHAN_INFO_PROCESSED channel.
> >
> > A few other minor things inline.
> >  
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > ---
> > > >  drivers/thermal/thermal-generic-adc.c | 62 ++++++++++++++++++++++++++-
> > > >  1 file changed, 61 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> > > > index ee3d0aa31406..7dcc2e1168a4 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,65 @@ 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[] = {  
> > Even though there is only one. If it is an array use channels.
> >
> > or stop it being an array and just take a pointer to a single channel
> > instance.
> >  
> 
> There should not be more channels, but tbh, you never know, I will
> rename to channels.
> 
> > > > +       {
> > > > +               .type = IIO_TEMP,
> > > > +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > > +                                     BIT(IIO_CHAN_INFO_SCALE),
> > > > +       }
> > > > +};
> > > > +
> > > > +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> > > > +                                struct iio_chan_spec const *chan,
> > > > +                                int *val, int *val2, long mask)
> > > > +{
> > > > +       struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> > > > +       int ret;
> > > > +
> > > > +       switch (mask) {
> > > > +       case IIO_CHAN_INFO_RAW:
> > > > +               ret = gadc_thermal_get_temp(gtinfo->tz_dev, val);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +
> > > > +               return IIO_VAL_INT;
> > > > +
> > > > +       case IIO_CHAN_INFO_SCALE:
> > > > +               /* scale to a degree centigrade */  
> >
> > As above.  See Documentation/ABI/testing/sysfs-bus-iio
> > entries from temperature. Scaling of a temperature channel is milli-degrees
> >
> > This is a bit of a historical artefact. Way back at the start of IIO
> > when we had relatively few channel types, where possible I matched the
> > scaling to hwmon.  With hindsight that made things a bit inconsistent
> > but we are stuck with it as ABI :(
> >  
> 
> RAW channel is in milli-degrees, or this is not enough? I don't get
> your point here tbh. What is wrong? Battery driver requires
> temperature in degree centigrade hence it is scaled to it.

If the battery driver is assuming the temperature from a query to an IIO
provide (to get the processed value) is in degrees centigrade then that
is a bug that needs fixing and it probably implies a scaling problem
in some other driver given I assume someone is using that driver!

The scale here should be 1.  Simplest being to just make a processed
channel and not provide scale at all.

We never explicitly document the internal ABI but the use of
scale and raw must match the userspace ABI which is documented
in Documentation/ABI/testing/sysfs-bus-iio

Jonathan


> 
> > Jonathan
> >  
> > > > +               *val = 1;
> > > > +               *val2 = 1000;
> > > > +               return IIO_VAL_FRACTIONAL;
> > > > +
> > > > +       default:
> > > > +               return -EINVAL;
> > > > +       }
> > > > +}
> > > > +
> > > > +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));  
> >
> > sizeof(*gtinfo) probably slightly better.
> >  
> > > > +
> > > > +       indio_dev->name = dev_name(dev);  
> >
> > What does this end up as?  The convention in IIO is to name after
> > a part number.  If you have duplicates this isn't how you tell them
> > apart.  So I'd kind of expect thermal-generic-temp or
> > something like that.
> >  
> 
> it is "generic-adc-thermal" with this name, it is not present anywhere in IIO.
> 
> > > > +       indio_dev->info = &gadc_thermal_iio_info;
> > > > +       indio_dev->modes = INDIO_DIRECT_MODE;
> > > > +       indio_dev->channels = gadc_thermal_iio_channel;  
> > As above, I'd stop that being an array and use
> >         indio_dev->channels = &gadc_thermal_iio_chanel;
> >         indio_dev->channels = 1;
> >
> > Unless you think maybe we will get more channels in future, in which case
> > just rename it channels (which happens to have one element this time)
> >  
> > > > +       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 +213,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
> > > >  
> > >
> > > Added Jonathan Cameron and linux-iio@vger.kernel.org to list.
> > >
> > > Jonathan, this is newer version of the thermal-generic-adc you have
> > > reviewed recently with channels adjusted like proposed in v3.  
> >  

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

[Svyatoslav Ryhel]

нд, 13 квіт. 2025 р. о 12:28 Jonathan Cameron <jic23@kernel.org> пише:
>
> On Sat, 12 Apr 2025 14:06:06 +0300
> Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > сб, 12 квіт. 2025 р. о 13:54 Jonathan Cameron <jic23@kernel.org> пише:
> > >
> > > On Sat, 5 Apr 2025 18:23:25 +0300
> > > Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > пн, 10 бер. 2025 р. о 09:57 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > > > >
> > > > > 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.
> > > > >
> > > > > The temperature data, while technically a product of conversion and thus
> > > > > categorized as IIO_CHAN_INFO_PROCESSED, maintains its unscaled state
> > > > > (milli-degree). To account for this, IIO_CHAN_INFO_RAW is used along with
> > > > > IIO_CHAN_INFO_SCALE to provide different degrees of accuracy.
> > >
> > > You've lost me in this description.  The base units of an IIO temperature channel
> > > are milli-degrees so if the scaling is already right for that you would
> > > be fine using a IIO_CHAN_INFO_PROCESSED channel.
> > >
> > > A few other minor things inline.
> > >
> > > > >
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > > ---
> > > > >  drivers/thermal/thermal-generic-adc.c | 62 ++++++++++++++++++++++++++-
> > > > >  1 file changed, 61 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/thermal/thermal-generic-adc.c b/drivers/thermal/thermal-generic-adc.c
> > > > > index ee3d0aa31406..7dcc2e1168a4 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,65 @@ 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[] = {
> > > Even though there is only one. If it is an array use channels.
> > >
> > > or stop it being an array and just take a pointer to a single channel
> > > instance.
> > >
> >
> > There should not be more channels, but tbh, you never know, I will
> > rename to channels.
> >
> > > > > +       {
> > > > > +               .type = IIO_TEMP,
> > > > > +               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > > > +                                     BIT(IIO_CHAN_INFO_SCALE),
> > > > > +       }
> > > > > +};
> > > > > +
> > > > > +static int gadc_thermal_read_raw(struct iio_dev *indio_dev,
> > > > > +                                struct iio_chan_spec const *chan,
> > > > > +                                int *val, int *val2, long mask)
> > > > > +{
> > > > > +       struct gadc_thermal_info *gtinfo = iio_priv(indio_dev);
> > > > > +       int ret;
> > > > > +
> > > > > +       switch (mask) {
> > > > > +       case IIO_CHAN_INFO_RAW:
> > > > > +               ret = gadc_thermal_get_temp(gtinfo->tz_dev, val);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +
> > > > > +               return IIO_VAL_INT;
> > > > > +
> > > > > +       case IIO_CHAN_INFO_SCALE:
> > > > > +               /* scale to a degree centigrade */
> > >
> > > As above.  See Documentation/ABI/testing/sysfs-bus-iio
> > > entries from temperature. Scaling of a temperature channel is milli-degrees
> > >
> > > This is a bit of a historical artefact. Way back at the start of IIO
> > > when we had relatively few channel types, where possible I matched the
> > > scaling to hwmon.  With hindsight that made things a bit inconsistent
> > > but we are stuck with it as ABI :(
> > >
> >
> > RAW channel is in milli-degrees, or this is not enough? I don't get
> > your point here tbh. What is wrong? Battery driver requires
> > temperature in degree centigrade hence it is scaled to it.
>
> If the battery driver is assuming the temperature from a query to an IIO
> provide (to get the processed value) is in degrees centigrade then that
> is a bug that needs fixing and it probably implies a scaling problem
> in some other driver given I assume someone is using that driver!
>
> The scale here should be 1.  Simplest being to just make a processed
> channel and not provide scale at all.
>
> We never explicitly document the internal ABI but the use of
> scale and raw must match the userspace ABI which is documented
> in Documentation/ABI/testing/sysfs-bus-iio
>
> Jonathan
>

Acknowledged. I will return back to processed and send a fix regards
the battery.

>
> >
> > > Jonathan
> > >
> > > > > +               *val = 1;
> > > > > +               *val2 = 1000;
> > > > > +               return IIO_VAL_FRACTIONAL;
> > > > > +
> > > > > +       default:
> > > > > +               return -EINVAL;
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > +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));
> > >
> > > sizeof(*gtinfo) probably slightly better.
> > >
> > > > > +
> > > > > +       indio_dev->name = dev_name(dev);
> > >
> > > What does this end up as?  The convention in IIO is to name after
> > > a part number.  If you have duplicates this isn't how you tell them
> > > apart.  So I'd kind of expect thermal-generic-temp or
> > > something like that.
> > >
> >
> > it is "generic-adc-thermal" with this name, it is not present anywhere in IIO.
> >

Then this is acceptable?

> > > > > +       indio_dev->info = &gadc_thermal_iio_info;
> > > > > +       indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > +       indio_dev->channels = gadc_thermal_iio_channel;
> > > As above, I'd stop that being an array and use
> > >         indio_dev->channels = &gadc_thermal_iio_chanel;
> > >         indio_dev->channels = 1;
> > >
> > > Unless you think maybe we will get more channels in future, in which case
> > > just rename it channels (which happens to have one element this time)
> > >
> > > > > +       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 +213,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
> > > > >
> > > >
> > > > Added Jonathan Cameron and linux-iio@vger.kernel.org to list.
> > > >
> > > > Jonathan, this is newer version of the thermal-generic-adc you have
> > > > reviewed recently with channels adjusted like proposed in v3.
> > >
>

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

[Jonathan Cameron]

> > > > > > +
> > > > > > +       indio_dev->name = dev_name(dev);  
> > > >
> > > > What does this end up as?  The convention in IIO is to name after
> > > > a part number.  If you have duplicates this isn't how you tell them
> > > > apart.  So I'd kind of expect thermal-generic-temp or
> > > > something like that.
> > > >  
> > >
> > > it is "generic-adc-thermal" with this name, it is not present anywhere in IIO.
> > >  
> 
> Then this is acceptable?

That string is fine.  I'd still be tempted to hard code it here so the
value is obvious to anyone looking at the driver.

Thanks,

Jonathan