Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units | From | Guenter Roeck <> | Date | Tue, 7 May 2019 06:55:41 -0700 |
| |
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, >
| |