[PATCH 0/2] hwmon: scmi: Scale values to target desired HWMON units

[PATCH 0/2] hwmon: scmi: Scale values to target desired HWMON units

[Florian Fainelli]

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

[PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale

[Florian Fainelli]

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

[PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units

[Florian Fainelli]

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

Re: [PATCH 1/2] firmware: arm_scmi: Fetch and store sensor scale

[Guenter Roeck]

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];
>   };
>   
> 

Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units

[Guenter Roeck]

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,
> 

Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units

[Florian Fainelli]

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

Re: [PATCH 2/2] hwmon: scmi: Scale values to target desired HWMON units

[Guenter Roeck]

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