* [PATCH 0/2] hwmon: scmi: Scale values to target desired HWMON units
@ 2019-05-06 22:41 Florian Fainelli
2019-05-06 22:41 ` [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli
2019-05-06 22:41 ` [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
0 siblings, 2 replies; 7+ messages in thread
From: Florian Fainelli @ 2019-05-06 22:41 UTC (permalink / raw)
To: linux-kernel
Cc: bcm-kernel-feedback-list, Florian Fainelli, Sudeep Holla,
Jean Delvare, Guenter Roeck, linux-arm-kernel,
open list:HARDWARE MONITORING
Hi Sudeep, Guenter,
This patch series adds support for scaling SCMI sensor values read from
firmware. Sudeep, let me know if you think we should be treating scale
== 0 as a special value to preserve some firmware compatibility (not
that this would be desired).
Thanks!
Florian Fainelli (2):
firmware: arm_scmi: Fetch and store sensor scale
hwmon: scmi: Scale values to target desired HWMON units
drivers/firmware/arm_scmi/sensors.c | 7 +++-
drivers/hwmon/scmi-hwmon.c | 55 ++++++++++++++++++++++++-----
include/linux/scmi_protocol.h | 1 +
3 files changed, 53 insertions(+), 10 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale
2019-05-06 22:41 [PATCH 0/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
@ 2019-05-06 22:41 ` Florian Fainelli
2019-05-07 13:31 ` Guenter Roeck
2019-05-06 22:41 ` [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
1 sibling, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2019-05-06 22:41 UTC (permalink / raw)
To: linux-kernel
Cc: bcm-kernel-feedback-list, Florian Fainelli, Sudeep Holla,
Jean Delvare, Guenter Roeck, linux-arm-kernel,
open list:HARDWARE MONITORING
In preparation for dealing with scales within the SCMI HWMON driver,
fetch and store the sensor unit scale into the scmi_sensor_info
structure.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/firmware/arm_scmi/sensors.c | 7 ++++++-
include/linux/scmi_protocol.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index b53d5cc9c9f6..f324f0a13ebe 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -33,7 +33,8 @@ struct scmi_msg_resp_sensor_description {
#define NUM_TRIP_POINTS(x) (((x) >> 4) & 0xff)
__le32 attributes_high;
#define SENSOR_TYPE(x) ((x) & 0xff)
-#define SENSOR_SCALE(x) (((x) >> 11) & 0x3f)
+#define SENSOR_SCALE_MASK 0x3f
+#define SENSOR_SCALE(x) (((x) >> 11) & SENSOR_SCALE_MASK)
#define SENSOR_UPDATE_SCALE(x) (((x) >> 22) & 0x1f)
#define SENSOR_UPDATE_BASE(x) (((x) >> 27) & 0x1f)
u8 name[SCMI_MAX_STR_SIZE];
@@ -140,6 +141,10 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
s = &si->sensors[desc_index + cnt];
s->id = le32_to_cpu(buf->desc[cnt].id);
s->type = SENSOR_TYPE(attrh);
+ s->scale = SENSOR_SCALE(attrh);
+ /* Sign extend to a full u8 */
+ if (s->scale & ((SENSOR_SCALE_MASK + 1) >> 1))
+ s->scale |= GENMASK(7, 6);
strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
}
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 3105055c00a7..7746f171f108 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -144,6 +144,7 @@ struct scmi_power_ops {
struct scmi_sensor_info {
u32 id;
u8 type;
+ u8 scale;
char name[SCMI_MAX_STR_SIZE];
};
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units
2019-05-06 22:41 [PATCH 0/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
2019-05-06 22:41 ` [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli
@ 2019-05-06 22:41 ` Florian Fainelli
2019-05-07 13:55 ` Guenter Roeck
1 sibling, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2019-05-06 22:41 UTC (permalink / raw)
To: linux-kernel
Cc: bcm-kernel-feedback-list, Florian Fainelli, Sudeep Holla,
Jean Delvare, Guenter Roeck, linux-arm-kernel,
open list:HARDWARE MONITORING
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;
+ 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,
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale
2019-05-06 22:41 ` [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli
@ 2019-05-07 13:31 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2019-05-07 13:31 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare,
linux-arm-kernel, open list:HARDWARE MONITORING
On 5/6/19 3:41 PM, Florian Fainelli wrote:
> In preparation for dealing with scales within the SCMI HWMON driver,
> fetch and store the sensor unit scale into the scmi_sensor_info
> structure.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/firmware/arm_scmi/sensors.c | 7 ++++++-
> include/linux/scmi_protocol.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index b53d5cc9c9f6..f324f0a13ebe 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -33,7 +33,8 @@ struct scmi_msg_resp_sensor_description {
> #define NUM_TRIP_POINTS(x) (((x) >> 4) & 0xff)
> __le32 attributes_high;
> #define SENSOR_TYPE(x) ((x) & 0xff)
> -#define SENSOR_SCALE(x) (((x) >> 11) & 0x3f)
> +#define SENSOR_SCALE_MASK 0x3f
> +#define SENSOR_SCALE(x) (((x) >> 11) & SENSOR_SCALE_MASK)
> #define SENSOR_UPDATE_SCALE(x) (((x) >> 22) & 0x1f)
> #define SENSOR_UPDATE_BASE(x) (((x) >> 27) & 0x1f)
> u8 name[SCMI_MAX_STR_SIZE];
> @@ -140,6 +141,10 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
> s = &si->sensors[desc_index + cnt];
> s->id = le32_to_cpu(buf->desc[cnt].id);
> s->type = SENSOR_TYPE(attrh);
> + s->scale = SENSOR_SCALE(attrh);
> + /* Sign extend to a full u8 */
> + if (s->scale & ((SENSOR_SCALE_MASK + 1) >> 1))
The logic here is quite confusing. I think it would be better to define,
say, SENSOR_SCALE_SIGN and use it.
> + s->scale |= GENMASK(7, 6);
> strlcpy(s->name, buf->desc[cnt].name, SCMI_MAX_STR_SIZE);
> }
>
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 3105055c00a7..7746f171f108 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -144,6 +144,7 @@ struct scmi_power_ops {
> struct scmi_sensor_info {
> u32 id;
> u8 type;
> + u8 scale;
Why not s8 if this is a signed value ?
Thanks,
Guenter
> char name[SCMI_MAX_STR_SIZE];
> };
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units
2019-05-06 22:41 ` [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
@ 2019-05-07 13:55 ` Guenter Roeck
2019-05-07 17:44 ` Florian Fainelli
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2019-05-07 13:55 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare,
linux-arm-kernel, open list:HARDWARE MONITORING
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,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units
2019-05-07 13:55 ` Guenter Roeck
@ 2019-05-07 17:44 ` Florian Fainelli
2019-05-07 18:26 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2019-05-07 17:44 UTC (permalink / raw)
To: Guenter Roeck, Florian Fainelli, linux-kernel
Cc: bcm-kernel-feedback-list, Sudeep Holla, Jean Delvare,
linux-arm-kernel, open list:HARDWARE MONITORING
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!
--
Florian
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units
2019-05-07 17:44 ` Florian Fainelli
@ 2019-05-07 18:26 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2019-05-07 18:26 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, bcm-kernel-feedback-list, Sudeep Holla,
Jean Delvare, linux-arm-kernel, open list:HARDWARE MONITORING
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-07 18:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-06 22:41 [PATCH 0/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
2019-05-06 22:41 ` [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale Florian Fainelli
2019-05-07 13:31 ` Guenter Roeck
2019-05-06 22:41 ` [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units Florian Fainelli
2019-05-07 13:55 ` Guenter Roeck
2019-05-07 17:44 ` Florian Fainelli
2019-05-07 18:26 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).