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@xxxxxxxxx> > >> --- > >> 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