Message ID | 20250304-ntc_min_max-v1-1-b08e70e56459@gocontroll.com (mailing list archive) |
---|---|
State | Rejected |
Headers | show |
Series | hwmon: (ntc_thermistor) Add min and max temperature attributes | expand |
On 3/4/25 00:24, Maud Spierings via B4 Relay wrote: > From: Maud Spierings <maudspierings@gocontroll.com> > > Add the min and max temperature attributes as it is trivial for this > driver. > > This can help with detecting implausible readings and indicates to users > which range they can actually measure, so they will not set a trip point > at a temperature higher than max or lower than min. > Unless I misunderstand the driver code, readings outside the table values are never reported. Also, min/max are supposed to be alarm temperatures. The reported values for min/max would be between -55 and +155 degrees C, which does not make sense and has zero value for trip point usage. NACK. Guenter > This implementation only works for ntcs, if a ptc gets added this will > have to be changed, but I'm not sure if that is ever the intention. > > Signed-off-by: Maud Spierings <maudspierings@gocontroll.com> > --- > drivers/hwmon/ntc_thermistor.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/ntc_thermistor.c b/drivers/hwmon/ntc_thermistor.c > index 0d29c8f97ba7c2f264588b6309b91ca494012ad6..fecee177be5daf304c0521a74f2733f6e42567e0 100644 > --- a/drivers/hwmon/ntc_thermistor.c > +++ b/drivers/hwmon/ntc_thermistor.c > @@ -522,6 +522,12 @@ static int ntc_read(struct device *dev, enum hwmon_sensor_types type, > case hwmon_temp_type: > *val = 4; > return 0; > + case hwmon_temp_min: > + *val = data->comp[0].temp_c*1000; > + return 0; > + case hwmon_temp_max: > + *val = data->comp[data->n_comp-1].temp_c*1000; > + return 0; > default: > break; > } > @@ -539,6 +545,8 @@ static umode_t ntc_is_visible(const void *data, enum hwmon_sensor_types type, > switch (attr) { > case hwmon_temp_input: > case hwmon_temp_type: > + case hwmon_temp_min: > + case hwmon_temp_max: > return 0444; > default: > break; > @@ -549,7 +557,8 @@ static umode_t ntc_is_visible(const void *data, enum hwmon_sensor_types type, > > static const struct hwmon_channel_info * const ntc_info[] = { > HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ), > - HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_TYPE), > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_TYPE | HWMON_T_MIN | > + HWMON_T_MAX), > NULL > }; > > > --- > base-commit: cd3215bbcb9d4321def93fea6cfad4d5b42b9d1d > change-id: 20250304-ntc_min_max-a925e87d0da7 > > Best regards,
From: Guenter Roeck <groeck7@gmail.com> on behalf of Guenter Roeck <linux@roeck-us.net> Sent: Tuesday, March 4, 2025 12:12 PM >On 3/4/25 00:24, Maud Spierings via B4 Relay wrote: >> From: Maud Spierings <maudspierings@gocontroll.com> >> >> Add the min and max temperature attributes as it is trivial for this >> driver. >> >> This can help with detecting implausible readings and indicates to users >> which range they can actually measure, so they will not set a trip point >> at a temperature higher than max or lower than min. >> >Unless I misunderstand the driver code, readings outside the table values >are never reported. Also, min/max are supposed to be alarm temperatures. >The reported values for min/max would be between -55 and +155 degrees C, >which does not make sense and has zero value for trip point usage. Regarding the driver not reporting values outside the table values: That does seem to be true and is good in my opinion, however currently 125 can mean 125 or something higher, with an indication of a max measurable temperature it can be determined that this is a max value and thus might have extra considerations. Regarding the meaning of attribues: It is difficult that the attributes do not have descriptions in include/linux/hwmon.h Is there an attribute that should be used to indicate this maximum measurable value to userspace? HWMON_T_HIGHEST/LOWEST? HWMON_T_RATED_MIN/MAX? Some extra ramblings: I want to have some indication of what the lowest and highest temperatures that the sensor can measure are. Imagine I set my trip point at 140 degrees, but the sensor can only measure up to 125, I would like there to be some feedback that this trip point can never be measured. Some kind of plausibility check may also be interesting. For example I have an ntc in an lvds display, if this display is disconnected it shuts down because the ADC reads zero, which means temp==temp_max. >NACK. > >Guenter Kind regards, Maud
On 3/4/25 03:33, Maud Spierings | GOcontroll wrote: > From: Guenter Roeck <groeck7@gmail.com> on behalf of Guenter Roeck <linux@roeck-us.net> > Sent: Tuesday, March 4, 2025 12:12 PM > >> On 3/4/25 00:24, Maud Spierings via B4 Relay wrote: >>> From: Maud Spierings <maudspierings@gocontroll.com> >>> >>> Add the min and max temperature attributes as it is trivial for this >>> driver. >>> >>> This can help with detecting implausible readings and indicates to users >>> which range they can actually measure, so they will not set a trip point >>> at a temperature higher than max or lower than min. >>> >> Unless I misunderstand the driver code, readings outside the table values >> are never reported. Also, min/max are supposed to be alarm temperatures. >> The reported values for min/max would be between -55 and +155 degrees C, >> which does not make sense and has zero value for trip point usage. > > Regarding the driver not reporting values outside the table values: > > That does seem to be true and is good in my opinion, however currently > 125 can mean 125 or something higher, with an indication of a max > measurable temperature it can be determined that this is a max value and > thus might have extra considerations. > > Regarding the meaning of attribues: > > It is difficult that the attributes do not have descriptions in > include/linux/hwmon.h > > Is there an attribute that should be used to indicate this maximum > measurable value to userspace? HWMON_T_HIGHEST/LOWEST? > HWMON_T_RATED_MIN/MAX? > There is no attribute providing the temperature (or any other) range supported by a chip. highest/lowest are highest/lowest measurements, and the rated attributes are rated min/max values for the system, not for the chip. This applies to all chips, not just this one. Misusing the ABI to report "limits supported by the chip" would be inappropriate and misleading. > Some extra ramblings: > > I want to have some indication of what the lowest and highest > temperatures that the sensor can measure are. Imagine I set my trip point > at 140 degrees, but the sensor can only measure up to 125, I would like > there to be some feedback that this trip point can never be measured. > That would be a problem which applies to every chip. Unfortunately, it is all but impossible to try to express that in sysfs attributes. Many chips have different temperature ratings based on the chip model/variant (commercial vs. car vs. mil), but the valid range can not be determined from register values. The same really applies to this driver: The tables in the driver are for specific thermistors, but the thermistor will still provide a reading if the temperature is out of range. It will just be out of spec. On top of that, thermal limits should be based on board limits, not on chip limits. A board which supports a temperature up to 140 degrees C but utilizes a temperature sensor which can only measure up to 125 degrees C would be a badly designed board. Trying to address that in a driver would not add any value. > Some kind of plausibility check may also be interesting. For example I > have an ntc in an lvds display, if this display is disconnected it shuts > down because the ADC reads zero, which means temp==temp_max. > The best we could do would be to return -ENODATA for temperature values if resistor values are out of range. Guenter
From: Guenter Roeck <groeck7@gmail.com> on behalf of Guenter Roeck <linux@roeck-us.net> Sent: Tuesday, March 4, 2025 12:54 PM >On 3/4/25 03:33, Maud Spierings | GOcontroll wrote: >> From: Guenter Roeck <groeck7@gmail.com> on behalf of Guenter Roeck <linux@roeck-us.net> >> Sent: Tuesday, March 4, 2025 12:12 PM >> >>> On 3/4/25 00:24, Maud Spierings via B4 Relay wrote: >>>> From: Maud Spierings <maudspierings@gocontroll.com> >>>> >>>> Add the min and max temperature attributes as it is trivial for this >>>> driver. >>>> >>>> This can help with detecting implausible readings and indicates to users >>>> which range they can actually measure, so they will not set a trip point >>>> at a temperature higher than max or lower than min. >>>> >>> Unless I misunderstand the driver code, readings outside the table values >>> are never reported. Also, min/max are supposed to be alarm temperatures. >>> The reported values for min/max would be between -55 and +155 degrees C, >>> which does not make sense and has zero value for trip point usage. >> >> Regarding the driver not reporting values outside the table values: >> >> That does seem to be true and is good in my opinion, however currently >> 125 can mean 125 or something higher, with an indication of a max >> measurable temperature it can be determined that this is a max value and >> thus might have extra considerations. >> >> Regarding the meaning of attribues: >> >> It is difficult that the attributes do not have descriptions in >> include/linux/hwmon.h >> >> Is there an attribute that should be used to indicate this maximum >> measurable value to userspace? HWMON_T_HIGHEST/LOWEST? >> HWMON_T_RATED_MIN/MAX? >> > >There is no attribute providing the temperature (or any other) range >supported by a chip. highest/lowest are highest/lowest measurements, and >the rated attributes are rated min/max values for the system, not for >the chip. This applies to all chips, not just this one. Misusing the >ABI to report "limits supported by the chip" would be inappropriate and >misleading. That is unfortunate that such an attribute does not exist >> Some extra ramblings: >> >> I want to have some indication of what the lowest and highest >> temperatures that the sensor can measure are. Imagine I set my trip point >> at 140 degrees, but the sensor can only measure up to 125, I would like >> there to be some feedback that this trip point can never be measured. >> >That would be a problem which applies to every chip. Unfortunately, it is >all but impossible to try to express that in sysfs attributes. Many chips >have different temperature ratings based on the chip model/variant >(commercial vs. car vs. mil), but the valid range can not be determined >from register values. > >The same really applies to this driver: The tables in the driver are for >specific thermistors, but the thermistor will still provide a reading if the >temperature is out of range. It will just be out of spec. > >On top of that, thermal limits should be based on board limits, not on chip >limits. A board which supports a temperature up to 140 degrees C but >utilizes a temperature sensor which can only measure up to 125 degrees C >would be a badly designed board. Trying to address that in a driver >would not add any value. > >> Some kind of plausibility check may also be interesting. For example I >> have an ntc in an lvds display, if this display is disconnected it shuts >> down because the ADC reads zero, which means temp==temp_max. >> > >The best we could do would be to return -ENODATA for temperature values >if resistor values are out of range. That sounds good to me, I'll give it a test and send a new patch if it works out. >Guenter
diff --git a/drivers/hwmon/ntc_thermistor.c b/drivers/hwmon/ntc_thermistor.c index 0d29c8f97ba7c2f264588b6309b91ca494012ad6..fecee177be5daf304c0521a74f2733f6e42567e0 100644 --- a/drivers/hwmon/ntc_thermistor.c +++ b/drivers/hwmon/ntc_thermistor.c @@ -522,6 +522,12 @@ static int ntc_read(struct device *dev, enum hwmon_sensor_types type, case hwmon_temp_type: *val = 4; return 0; + case hwmon_temp_min: + *val = data->comp[0].temp_c*1000; + return 0; + case hwmon_temp_max: + *val = data->comp[data->n_comp-1].temp_c*1000; + return 0; default: break; } @@ -539,6 +545,8 @@ static umode_t ntc_is_visible(const void *data, enum hwmon_sensor_types type, switch (attr) { case hwmon_temp_input: case hwmon_temp_type: + case hwmon_temp_min: + case hwmon_temp_max: return 0444; default: break; @@ -549,7 +557,8 @@ static umode_t ntc_is_visible(const void *data, enum hwmon_sensor_types type, static const struct hwmon_channel_info * const ntc_info[] = { HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ), - HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_TYPE), + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_TYPE | HWMON_T_MIN | + HWMON_T_MAX), NULL };