| Message ID | 1492749112-32465-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| 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: >
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: >> >
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
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.
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.
... > + 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.
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(-)