| Message ID | 20190506224109.9357-3-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: scmi: Scale values to target desired HWMON units | expand |
Hi Florian, On 5/6/19 3:41 PM, Florian Fainelli wrote: > If the SCMI firmware implementation is reporting values in a scale that > is different from the HWMON units, we need to scale up or down the value > according to how far appart they are. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/hwmon/scmi-hwmon.c | 55 +++++++++++++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 9 deletions(-) > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > index a80183a488c5..e9913509cb88 100644 > --- a/drivers/hwmon/scmi-hwmon.c > +++ b/drivers/hwmon/scmi-hwmon.c > @@ -18,6 +18,51 @@ struct scmi_sensors { > const struct scmi_sensor_info **info[hwmon_max]; > }; > > +static enum hwmon_sensor_types scmi_types[] = { > + [TEMPERATURE_C] = hwmon_temp, > + [VOLTAGE] = hwmon_in, > + [CURRENT] = hwmon_curr, > + [POWER] = hwmon_power, > + [ENERGY] = hwmon_energy, > +}; > + > +static u64 scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 value) > +{ > + u64 scaled_value = value; I don't think that variable is necessary. > + s8 desired_scale; Just scale ? Also, you could assign scale here directly, and subtract the offset below. Then "n" would not be necessary. Such as s8 scale = sensor->scale; // assuming scale is s8 ... case CURRENT: scale += 3; ... That would also be less confusing, since it would avoid the double negation. > + int n, p; > + > + switch (sensor->type) { > + case TEMPERATURE_C: > + case VOLTAGE: > + case CURRENT: > + /* fall through */ Unnecessary comment > + desired_scale = -3; > + break; > + case POWER: > + case ENERGY: > + /* fall through */ Unnecessary comment. > + desired_scale = -6; > + break; > + default: > + return scaled_value; Here we presumably want a scale of 0. However, if the scale passed from SCMI is, say, -5 or +5, we return the original (unadjusted) value. Seems to me we would still want to adjust the value to match hwmon expectations. Am I missing something ? > + } > + > + n = (s8)sensor->scale - desired_scale; > + if (n == 0) Indentation seems off here. > + return scaled_value; > + > + for (p = 0; p < abs(n); p++) { > + /* Need to scale up from sensor to HWMON */ > + if (n > 0) > + scaled_value *= 10; > + else > + do_div(scaled_value, 10); > + } Something like factor = pow10(abs(scale)); if (scale > 0) value *= factor; else do_div(value, factor); would avoid the repeated abs() and do_div(). Unfortunately there is no pow10() helper, so you would have to write that. Still, I think that would be much more efficient. Thanks, Guenter > + > + return scaled_value; > +} > + > static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > u32 attr, int channel, long *val) > { > @@ -30,7 +75,7 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > sensor = *(scmi_sensors->info[type] + channel); > ret = h->sensor_ops->reading_get(h, sensor->id, false, &value); > if (!ret) > - *val = value; > + *val = scmi_hwmon_scale(sensor, value); > > return ret; > } > @@ -91,14 +136,6 @@ static int scmi_hwmon_add_chan_info(struct hwmon_channel_info *scmi_hwmon_chan, > return 0; > } > > -static enum hwmon_sensor_types scmi_types[] = { > - [TEMPERATURE_C] = hwmon_temp, > - [VOLTAGE] = hwmon_in, > - [CURRENT] = hwmon_curr, > - [POWER] = hwmon_power, > - [ENERGY] = hwmon_energy, > -}; > - > static u32 hwmon_attributes[] = { > [hwmon_chip] = HWMON_C_REGISTER_TZ, > [hwmon_temp] = HWMON_T_INPUT | HWMON_T_LABEL, >
On 5/7/19 6:55 AM, Guenter Roeck wrote: > Hi Florian, > > On 5/6/19 3:41 PM, Florian Fainelli wrote: >> If the SCMI firmware implementation is reporting values in a scale that >> is different from the HWMON units, we need to scale up or down the value >> according to how far appart they are. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> drivers/hwmon/scmi-hwmon.c | 55 +++++++++++++++++++++++++++++++------- >> 1 file changed, 46 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c >> index a80183a488c5..e9913509cb88 100644 >> --- a/drivers/hwmon/scmi-hwmon.c >> +++ b/drivers/hwmon/scmi-hwmon.c >> @@ -18,6 +18,51 @@ struct scmi_sensors { >> const struct scmi_sensor_info **info[hwmon_max]; >> }; >> +static enum hwmon_sensor_types scmi_types[] = { >> + [TEMPERATURE_C] = hwmon_temp, >> + [VOLTAGE] = hwmon_in, >> + [CURRENT] = hwmon_curr, >> + [POWER] = hwmon_power, >> + [ENERGY] = hwmon_energy, >> +}; >> + >> +static u64 scmi_hwmon_scale(const struct scmi_sensor_info *sensor, >> u64 value) >> +{ >> + u64 scaled_value = value; > > I don't think that variable is necessary. > >> + s8 desired_scale; > > Just scale ? Also, you could assign scale here directly, and subtract > the offset below. Then "n" would not be necessary. > Such as > s8 scale = sensor->scale; // assuming scale is s8 > ... > case CURRENT: > scale += 3; > ... > > That would also be less confusing, since it would avoid the double > negation. > >> + int n, p; > >> + >> + switch (sensor->type) { >> + case TEMPERATURE_C: >> + case VOLTAGE: >> + case CURRENT: >> + /* fall through */ > Unnecessary comment Is not removing the comment going to upset gcc when using -Wimplicit-fallthrough? > >> + desired_scale = -3; >> + break; >> + case POWER: >> + case ENERGY: >> + /* fall through */ > Unnecessary comment. > >> + desired_scale = -6; >> + break; >> + default: >> + return scaled_value; > > Here we presumably want a scale of 0. However, if the scale passed > from SCMI is, say, -5 or +5, we return the original (unadjusted) > value. Seems to me we would still want to adjust the value to match > hwmon expectations. Am I missing something ? You raise a valid point, not that could happen today because if the sensor type has a value we don't recognize, we have not registered it, so we would not even try to read rom it, but let's be future proof. > >> + } >> + >> + n = (s8)sensor->scale - desired_scale; >> + if (n == 0) > > Indentation seems off here. > >> + return scaled_value; >> + >> + for (p = 0; p < abs(n); p++) { >> + /* Need to scale up from sensor to HWMON */ >> + if (n > 0) >> + scaled_value *= 10; >> + else >> + do_div(scaled_value, 10); >> + } > > Something like > > factor = pow10(abs(scale)); > if (scale > 0) > value *= factor; > else > do_div(value, factor); > > would avoid the repeated abs() and do_div(). Unfortunately there is > no pow10() helper, so you would have to write that. Still, I think > that would be much more efficient. Sounds reasonable. Thanks for your feedback!
Hi Florian, On Tue, May 07, 2019 at 10:44:00AM -0700, Florian Fainelli wrote: > On 5/7/19 6:55 AM, Guenter Roeck wrote: > > Hi Florian, > > > > On 5/6/19 3:41 PM, Florian Fainelli wrote: > >> If the SCMI firmware implementation is reporting values in a scale that > >> is different from the HWMON units, we need to scale up or down the value > >> according to how far appart they are. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> drivers/hwmon/scmi-hwmon.c | 55 +++++++++++++++++++++++++++++++------- > >> 1 file changed, 46 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > >> index a80183a488c5..e9913509cb88 100644 > >> --- a/drivers/hwmon/scmi-hwmon.c > >> +++ b/drivers/hwmon/scmi-hwmon.c > >> @@ -18,6 +18,51 @@ struct scmi_sensors { > >> const struct scmi_sensor_info **info[hwmon_max]; > >> }; > >> +static enum hwmon_sensor_types scmi_types[] = { > >> + [TEMPERATURE_C] = hwmon_temp, > >> + [VOLTAGE] = hwmon_in, > >> + [CURRENT] = hwmon_curr, > >> + [POWER] = hwmon_power, > >> + [ENERGY] = hwmon_energy, > >> +}; > >> + > >> +static u64 scmi_hwmon_scale(const struct scmi_sensor_info *sensor, > >> u64 value) > >> +{ > >> + u64 scaled_value = value; > > > > I don't think that variable is necessary. > > > >> + s8 desired_scale; > > > > Just scale ? Also, you could assign scale here directly, and subtract > > the offset below. Then "n" would not be necessary. > > Such as > > s8 scale = sensor->scale; // assuming scale is s8 > > ... > > case CURRENT: > > scale += 3; > > ... > > > > That would also be less confusing, since it would avoid the double > > negation. > > > >> + int n, p; > > > >> + > >> + switch (sensor->type) { > >> + case TEMPERATURE_C: > >> + case VOLTAGE: > >> + case CURRENT: > >> + /* fall through */ > > Unnecessary comment > > Is not removing the comment going to upset gcc when using > -Wimplicit-fallthrough? > There is no implicit fallthrough, and the comment would have to be ahead of the previous case statement. Such as: case VOLTAGE: scale++; /* fall through */ case CURRENT: scale++; break; ... Two case statements together don't count as fall through. Guenter > > > >> + desired_scale = -3; > >> + break; > >> + case POWER: > >> + case ENERGY: > >> + /* fall through */ > > Unnecessary comment. > > > >> + desired_scale = -6; > >> + break; > >> + default: > >> + return scaled_value; > > > > Here we presumably want a scale of 0. However, if the scale passed > > from SCMI is, say, -5 or +5, we return the original (unadjusted) > > value. Seems to me we would still want to adjust the value to match > > hwmon expectations. Am I missing something ? > > You raise a valid point, not that could happen today because if the > sensor type has a value we don't recognize, we have not registered it, > so we would not even try to read rom it, but let's be future proof. > > > > >> + } > >> + > >> + n = (s8)sensor->scale - desired_scale; > >> + if (n == 0) > > > > Indentation seems off here. > > > >> + return scaled_value; > >> + > >> + for (p = 0; p < abs(n); p++) { > >> + /* Need to scale up from sensor to HWMON */ > >> + if (n > 0) > >> + scaled_value *= 10; > >> + else > >> + do_div(scaled_value, 10); > >> + } > > > > Something like > > > > factor = pow10(abs(scale)); > > if (scale > 0) > > value *= factor; > > else > > do_div(value, factor); > > > > would avoid the repeated abs() and do_div(). Unfortunately there is > > no pow10() helper, so you would have to write that. Still, I think > > that would be much more efficient. > > Sounds reasonable. Thanks for your feedback! > -- > Florian
diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c index a80183a488c5..e9913509cb88 100644 --- a/drivers/hwmon/scmi-hwmon.c +++ b/drivers/hwmon/scmi-hwmon.c @@ -18,6 +18,51 @@ struct scmi_sensors { const struct scmi_sensor_info **info[hwmon_max]; }; +static enum hwmon_sensor_types scmi_types[] = { + [TEMPERATURE_C] = hwmon_temp, + [VOLTAGE] = hwmon_in, + [CURRENT] = hwmon_curr, + [POWER] = hwmon_power, + [ENERGY] = hwmon_energy, +}; + +static u64 scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 value) +{ + u64 scaled_value = value; + s8 desired_scale; + int n, p; + + switch (sensor->type) { + case TEMPERATURE_C: + case VOLTAGE: + case CURRENT: + /* fall through */ + desired_scale = -3; + break; + case POWER: + case ENERGY: + /* fall through */ + desired_scale = -6; + break; + default: + return scaled_value; + } + + n = (s8)sensor->scale - desired_scale; + if (n == 0) + return scaled_value; + + for (p = 0; p < abs(n); p++) { + /* Need to scale up from sensor to HWMON */ + if (n > 0) + scaled_value *= 10; + else + do_div(scaled_value, 10); + } + + return scaled_value; +} + static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, long *val) { @@ -30,7 +75,7 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, sensor = *(scmi_sensors->info[type] + channel); ret = h->sensor_ops->reading_get(h, sensor->id, false, &value); if (!ret) - *val = value; + *val = scmi_hwmon_scale(sensor, value); return ret; } @@ -91,14 +136,6 @@ static int scmi_hwmon_add_chan_info(struct hwmon_channel_info *scmi_hwmon_chan, return 0; } -static enum hwmon_sensor_types scmi_types[] = { - [TEMPERATURE_C] = hwmon_temp, - [VOLTAGE] = hwmon_in, - [CURRENT] = hwmon_curr, - [POWER] = hwmon_power, - [ENERGY] = hwmon_energy, -}; - static u32 hwmon_attributes[] = { [hwmon_chip] = HWMON_C_REGISTER_TZ, [hwmon_temp] = HWMON_T_INPUT | HWMON_T_LABEL,
If the SCMI firmware implementation is reporting values in a scale that is different from the HWMON units, we need to scale up or down the value according to how far appart they are. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/hwmon/scmi-hwmon.c | 55 +++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 9 deletions(-)