| Message ID | 20250303122151.91557-3-clamor95@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | thermal: thermal-generic-adc: add temp sensor function | expand |
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[] = {
ср, 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;
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).
ср, 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).
ср, 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).
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.
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[] = {
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(-)