| Message ID | 1492749112-32465-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hello Shilpasri, On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote: > Add support for adding min/max values for the inband sensors copied by > OCC to main memory. And also add current(mA) sensors to the list. > > Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> > --- > drivers/hwmon/ibmpowernv.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 53 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c > index 6d2e660..1f329fa8 100644 > --- a/drivers/hwmon/ibmpowernv.c > +++ b/drivers/hwmon/ibmpowernv.c > @@ -50,6 +50,7 @@ enum sensors { > TEMP, > POWER_SUPPLY, > POWER_INPUT, > + CURRENT, > MAX_SENSOR_TYPE, > }; > > @@ -65,7 +66,8 @@ enum sensors { > {"fan", "ibm,opal-sensor-cooling-fan"}, > {"temp", "ibm,opal-sensor-amb-temp"}, > {"in", "ibm,opal-sensor-power-supply"}, > - {"power", "ibm,opal-sensor-power"} > + {"power", "ibm,opal-sensor-power"}, > + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ why isn't there a compatible string ? > }; > > struct sensor_data { > @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev) > opal = of_find_node_by_path("/ibm,opal/sensors"); > for_each_child_of_node(opal, np) { > const char *label; > + int len; > > if (np->name == NULL) > continue; > @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev) > sensor_groups[type].attr_count++; > > /* > - * add a new attribute for labels > + * add attributes for labels, min and max > */ > if (!of_property_read_string(np, "label", &label)) > sensor_groups[type].attr_count++; We are negating of_property_read_string() above, but not below. I wonder why ? > + if (of_find_property(np, "sensor-data-min", &len)) > + sensor_groups[type].attr_count++; > + if (of_find_property(np, "sensor-data-max", &len)) > + sensor_groups[type].attr_count++; > } > > of_node_put(opal); > @@ -428,6 +435,50 @@ static int create_device_attrs(struct platform_device *pdev) > pgroups[type]->attrs[sensor_groups[type].attr_count++] = > &sdata[count++].dev_attr.attr; > } > + > + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) { > + switch (type) { > + case POWER_INPUT: > + attr_name = "input_highest"; > + break; > + case TEMP: > + attr_name = "max"; > + break; > + default: > + attr_name = "highest"; > + break; > + } May be we could use a converting routine here ? create_device_attrs() is getting big. > + sdata[count].id = sensor_id; > + sdata[count].type = type; > + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; > + create_hwmon_attr(&sdata[count], attr_name, > + show_sensor); > + pgroups[type]->attrs[sensor_groups[type].attr_count++] = > + &sdata[count++].dev_attr.attr; > + } > + > + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) { > + switch (type) { > + case POWER_INPUT: > + attr_name = "input_lowest"; > + break; > + case TEMP: > + attr_name = "min"; > + break; > + default: > + attr_name = "lowest"; > + break; > + } same here. Thanks, C. > + sdata[count].id = sensor_id; > + sdata[count].type = type; > + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; > + create_hwmon_attr(&sdata[count], attr_name, > + show_sensor); > + pgroups[type]->attrs[sensor_groups[type].attr_count++] = > + &sdata[count++].dev_attr.attr; > + } > } > > exit_put_node: > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Cedric, On 04/21/2017 05:17 PM, Cédric Le Goater wrote: > Hello Shilpasri, > > On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote: >> Add support for adding min/max values for the inband sensors copied by >> OCC to main memory. And also add current(mA) sensors to the list. >> >> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> >> --- >> drivers/hwmon/ibmpowernv.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c >> index 6d2e660..1f329fa8 100644 >> --- a/drivers/hwmon/ibmpowernv.c >> +++ b/drivers/hwmon/ibmpowernv.c >> @@ -50,6 +50,7 @@ enum sensors { >> TEMP, >> POWER_SUPPLY, >> POWER_INPUT, >> + CURRENT, >> MAX_SENSOR_TYPE, >> }; >> >> @@ -65,7 +66,8 @@ enum sensors { >> {"fan", "ibm,opal-sensor-cooling-fan"}, >> {"temp", "ibm,opal-sensor-amb-temp"}, >> {"in", "ibm,opal-sensor-power-supply"}, >> - {"power", "ibm,opal-sensor-power"} >> + {"power", "ibm,opal-sensor-power"}, >> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ > > why isn't there a compatible string ? > Skiboot exports "ibm,opal-sensor" as compatible string for all the inband sensors. Now if I define that as the compatible string here, then all the sensors would get identified as "curr" type of sensor by the driver. >> }; >> >> struct sensor_data { >> @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev) >> opal = of_find_node_by_path("/ibm,opal/sensors"); >> for_each_child_of_node(opal, np) { >> const char *label; >> + int len; >> >> if (np->name == NULL) >> continue; >> @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev) >> sensor_groups[type].attr_count++; >> >> /* >> - * add a new attribute for labels >> + * add attributes for labels, min and max >> */ >> if (!of_property_read_string(np, "label", &label)) >> sensor_groups[type].attr_count++; > > We are negating of_property_read_string() above, but not below. > I wonder why ? of_find_property() returns 'struct property *' while of_property_read_string() returns 0 on success. > >> + if (of_find_property(np, "sensor-data-min", &len)) >> + sensor_groups[type].attr_count++; >> + if (of_find_property(np, "sensor-data-max", &len)) >> + sensor_groups[type].attr_count++; >> } >> >> of_node_put(opal); >> @@ -428,6 +435,50 @@ static int create_device_attrs(struct platform_device *pdev) >> pgroups[type]->attrs[sensor_groups[type].attr_count++] = >> &sdata[count++].dev_attr.attr; >> } >> + >> + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) { >> + switch (type) { >> + case POWER_INPUT: >> + attr_name = "input_highest"; >> + break; >> + case TEMP: >> + attr_name = "max"; >> + break; >> + default: >> + attr_name = "highest"; >> + break; >> + } > > May be we could use a converting routine here ? create_device_attrs() > is getting big. Okay will do. > >> + sdata[count].id = sensor_id; >> + sdata[count].type = type; >> + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; >> + create_hwmon_attr(&sdata[count], attr_name, >> + show_sensor); >> + pgroups[type]->attrs[sensor_groups[type].attr_count++] = >> + &sdata[count++].dev_attr.attr; >> + } >> + >> + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) { >> + switch (type) { >> + case POWER_INPUT: >> + attr_name = "input_lowest"; >> + break; >> + case TEMP: >> + attr_name = "min"; >> + break; >> + default: >> + attr_name = "lowest"; >> + break; >> + } > > same here. Sure. Thanks and Regards, Shilpa > > Thanks, > > C. >> + sdata[count].id = sensor_id; >> + sdata[count].type = type; >> + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; >> + create_hwmon_attr(&sdata[count], attr_name, >> + show_sensor); >> + pgroups[type]->attrs[sensor_groups[type].attr_count++] = >> + &sdata[count++].dev_attr.attr; >> + } >> } >> >> exit_put_node: >> > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes: > On 04/21/2017 05:17 PM, Cédric Le Goater wrote: >> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote: >>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c >>> index 6d2e660..1f329fa8 100644 >>> --- a/drivers/hwmon/ibmpowernv.c >>> +++ b/drivers/hwmon/ibmpowernv.c >>> @@ -65,7 +66,8 @@ enum sensors { >>> {"fan", "ibm,opal-sensor-cooling-fan"}, >>> {"temp", "ibm,opal-sensor-amb-temp"}, >>> {"in", "ibm,opal-sensor-power-supply"}, >>> - {"power", "ibm,opal-sensor-power"} >>> + {"power", "ibm,opal-sensor-power"}, >>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ >> >> why isn't there a compatible string ? > > Skiboot exports "ibm,opal-sensor" as compatible string for all the inband > sensors. Now if I define that as the compatible string here, then all the > sensors would get identified as "curr" type of sensor by the driver. So fix it in skiboot? cheers -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/22/2017 08:11 AM, Michael Ellerman wrote: > Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes: >> On 04/21/2017 05:17 PM, Cédric Le Goater wrote: >>> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote: >>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c >>>> index 6d2e660..1f329fa8 100644 >>>> --- a/drivers/hwmon/ibmpowernv.c >>>> +++ b/drivers/hwmon/ibmpowernv.c >>>> @@ -65,7 +66,8 @@ enum sensors { >>>> {"fan", "ibm,opal-sensor-cooling-fan"}, >>>> {"temp", "ibm,opal-sensor-amb-temp"}, >>>> {"in", "ibm,opal-sensor-power-supply"}, >>>> - {"power", "ibm,opal-sensor-power"} >>>> + {"power", "ibm,opal-sensor-power"}, >>>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ >>> >>> why isn't there a compatible string ? >> >> Skiboot exports "ibm,opal-sensor" as compatible string for all the inband >> sensors. Now if I define that as the compatible string here, then all the >> sensors would get identified as "curr" type of sensor by the driver. > > So fix it in skiboot? After a memory refresh, this table is to maintain compatibility with the support of the FSP sensors in old firmware. These have peculiar device node names and properties. So What the code is doing looks correct. But, you should also modify the 'enum sensors' to include a CURRENT value. Thanks, C. -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/25/2017 04:28 PM, Cédric Le Goater wrote: > On 04/22/2017 08:11 AM, Michael Ellerman wrote: >> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes: >>> On 04/21/2017 05:17 PM, Cédric Le Goater wrote: >>>> On 04/21/2017 06:31 AM, Shilpasri G Bhat wrote: >>>>> diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c >>>>> index 6d2e660..1f329fa8 100644 >>>>> --- a/drivers/hwmon/ibmpowernv.c >>>>> +++ b/drivers/hwmon/ibmpowernv.c >>>>> @@ -65,7 +66,8 @@ enum sensors { >>>>> {"fan", "ibm,opal-sensor-cooling-fan"}, >>>>> {"temp", "ibm,opal-sensor-amb-temp"}, >>>>> {"in", "ibm,opal-sensor-power-supply"}, >>>>> - {"power", "ibm,opal-sensor-power"} >>>>> + {"power", "ibm,opal-sensor-power"}, >>>>> + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ >>>> >>>> why isn't there a compatible string ? >>> >>> Skiboot exports "ibm,opal-sensor" as compatible string for all the inband >>> sensors. Now if I define that as the compatible string here, then all the >>> sensors would get identified as "curr" type of sensor by the driver. >> >> So fix it in skiboot? > > > After a memory refresh, this table is to maintain compatibility with > the support of the FSP sensors in old firmware. These have peculiar > device node names and properties. > > So What the code is doing looks correct. But, you should also modify > the 'enum sensors' to include a CURRENT value. But the patch does that already. I was missing context. This is fine then. Thanks, C. -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
... > + sdata[count].id = sensor_id; > + sdata[count].type = type; > + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; > + create_hwmon_attr(&sdata[count], attr_name, > + show_sensor); > + pgroups[type]->attrs[sensor_groups[type].attr_count++] = > + &sdata[count++].dev_attr.attr; > + } We are duplicating these lines at least three times. I wonder if we could make a routine for them. Don't bother doing so if the number of arguments is too large. Thanks, C. -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c index 6d2e660..1f329fa8 100644 --- a/drivers/hwmon/ibmpowernv.c +++ b/drivers/hwmon/ibmpowernv.c @@ -50,6 +50,7 @@ enum sensors { TEMP, POWER_SUPPLY, POWER_INPUT, + CURRENT, MAX_SENSOR_TYPE, }; @@ -65,7 +66,8 @@ enum sensors { {"fan", "ibm,opal-sensor-cooling-fan"}, {"temp", "ibm,opal-sensor-amb-temp"}, {"in", "ibm,opal-sensor-power-supply"}, - {"power", "ibm,opal-sensor-power"} + {"power", "ibm,opal-sensor-power"}, + {"curr"}, /* Follows newer device tree compatible ibm,opal-sensor */ }; struct sensor_data { @@ -287,6 +289,7 @@ static int populate_attr_groups(struct platform_device *pdev) opal = of_find_node_by_path("/ibm,opal/sensors"); for_each_child_of_node(opal, np) { const char *label; + int len; if (np->name == NULL) continue; @@ -298,10 +301,14 @@ static int populate_attr_groups(struct platform_device *pdev) sensor_groups[type].attr_count++; /* - * add a new attribute for labels + * add attributes for labels, min and max */ if (!of_property_read_string(np, "label", &label)) sensor_groups[type].attr_count++; + if (of_find_property(np, "sensor-data-min", &len)) + sensor_groups[type].attr_count++; + if (of_find_property(np, "sensor-data-max", &len)) + sensor_groups[type].attr_count++; } of_node_put(opal); @@ -428,6 +435,50 @@ static int create_device_attrs(struct platform_device *pdev) pgroups[type]->attrs[sensor_groups[type].attr_count++] = &sdata[count++].dev_attr.attr; } + + if (!of_property_read_u32(np, "sensor-data-max", &sensor_id)) { + switch (type) { + case POWER_INPUT: + attr_name = "input_highest"; + break; + case TEMP: + attr_name = "max"; + break; + default: + attr_name = "highest"; + break; + } + + sdata[count].id = sensor_id; + sdata[count].type = type; + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; + create_hwmon_attr(&sdata[count], attr_name, + show_sensor); + pgroups[type]->attrs[sensor_groups[type].attr_count++] = + &sdata[count++].dev_attr.attr; + } + + if (!of_property_read_u32(np, "sensor-data-min", &sensor_id)) { + switch (type) { + case POWER_INPUT: + attr_name = "input_lowest"; + break; + case TEMP: + attr_name = "min"; + break; + default: + attr_name = "lowest"; + break; + } + + sdata[count].id = sensor_id; + sdata[count].type = type; + sdata[count].hwmon_index = sdata[count - 1].hwmon_index; + create_hwmon_attr(&sdata[count], attr_name, + show_sensor); + pgroups[type]->attrs[sensor_groups[type].attr_count++] = + &sdata[count++].dev_attr.attr; + } } exit_put_node:
Add support for adding min/max values for the inband sensors copied by OCC to main memory. And also add current(mA) sensors to the list. Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> --- drivers/hwmon/ibmpowernv.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-)