[RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors
Guenter Roeck
linux at roeck-us.net
Mon Mar 13 09:53:56 AEDT 2017
On 03/09/2017 05:30 AM, Shilpasri G Bhat wrote:
> Hi Guenter,
>
> On 03/09/2017 05:40 PM, Guenter Roeck wrote:
>> On Thu, Mar 09, 2017 at 05:19:15PM +0530, Shilpasri G Bhat wrote:
>>> Add support to read power and temperature sensors from OCC inband
>>> sensors which are copied to main memory by OCC.
>>>
>>
>> Is this supposed to be an alternative to the submission from
>> Eddie James ? If so, is there a reason to consider such an
>> alternative ?
>>
>
> This is not an alternative submission. Eddie James' hwmon OCC driver is to
> export the sensors in BMC which is a service processor for POWER{8,9} servers.
> This patch adds a hwmon driver in the POWER9 server.
>
Both are names ...occ... kind of difficult to understand what is what.
Either case, it appears that there are some dependencies to other code
which I was not copied on. that is not very helpful.
Please use the new hwmon API (devm_hwmon_device_register_with_info); this driver
seems to be a perfect candidate. Also, please do not use function macros;
those tend to obfuscate the code and make it hard to understand.
With the new API, such macros should not be necessary.
Couple of additional comments inline.
Thanks,
Guenter
> Thanks and Regards,
> Shilpa
>
>> Thanks,
>> Guenter
>>
>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
>>> CC: Rob Herring <robh+dt at kernel.org>
>>> CC: Mark Rutland <mark.rutland at arm.com>
>>> CC: Jean Delvare <jdelvare at suse.com>
>>> CC: Guenter Roeck <linux at roeck-us.net>
>>> CC: Jonathan Corbet <corbet at lwn.net>
>>> CC: devicetree at vger.kernel.org
>>> CC: linux-hwmon at vger.kernel.orgThat
>>> CC: linux-doc at vger.kernel.org
>>> ---
>>> .../devicetree/bindings/hwmon/ibmpowernv-occ.txt | 4 +
>>> Documentation/hwmon/ibmpowernv-occ | 24 ++
>>> drivers/hwmon/Kconfig | 11 +
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/ibmpowernv-occ.c | 302 +++++++++++++++++++++
>>> 5 files changed, 342 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt
>>> create mode 100644 Documentation/hwmon/ibmpowernv-occ
>>> create mode 100644 drivers/hwmon/ibmpowernv-occ.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt b/Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt
>>> new file mode 100644
>>> index 0000000..d03f744
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/ibmpowernv-occ.txt
>>> @@ -0,0 +1,4 @@
>>> +IBM POWERNV OCC inband platform sensors
>>> +
>>> +Required device-tree property:
>>> +- compatible: "ibm,p9-occ-inband-sensor"
>>> diff --git a/Documentation/hwmon/ibmpowernv-occ b/Documentation/hwmon/ibmpowernv-occ
>>> new file mode 100644
>>> index 0000000..151028b
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/ibmpowernv-occ
>>> @@ -0,0 +1,24 @@
>>> +Kernel driver ibmpowernv-occ
>>> +=============================
>>> +
>>> +Supported systems:
>>> + * P9 server based on POWERNV platform
>>> +
>>> +Description
>>> +------------
>>> +
>>> +This driver exports the power and temperature sensors from OCC inband
>>> +sensors on P9 POWERNV platforms.
>>> +
>>> +Sysfs attributes
>>> +----------------
>>> +
>>> +powerX_input Latest power reading
>>> +powerX_input_highest Minimum power
>>> +powerX_input_lowest Maximum power
>>> +powerX_label Sensor name
>>> +
>>> +tempX_input Latest temperature reading
>>> +tempX_max Minimum temperature
>>> +tempX_min Maximum temperature
>>> +tempX_label Sensor name
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 0649d53f3..3b1dbb9 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -598,6 +598,17 @@ config SENSORS_IBMPOWERNV
>>> This driver can also be built as a module. If so, the module
>>> will be called ibmpowernv.
>>>
>>> +config SENSORS_IBMPOWERNV_OCC
>>> + tristate "IBM POWERNV OCC Inband platform sensors"
>>> + depends on PPC_POWERNV
>>> + default y
>>> + help
>>> + If you say yes here you get support for the temperature/power
>>> + OCC inband sensors on your PowerNV platform.
>>> +
>>> + This driver can also be built as a module. If so, the module
>>> + will be called ibmpowernv-occ.
>>> +
>>> config SENSORS_IIO_HWMON
>>> tristate "Hwmon driver that uses channels specified via iio maps"
>>> depends on IIO
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 5509edf..0da2207 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -75,6 +75,7 @@ obj-$(CONFIG_SENSORS_I5K_AMB) += i5k_amb.o
>>> obj-$(CONFIG_SENSORS_IBMAEM) += ibmaem.o
>>> obj-$(CONFIG_SENSORS_IBMPEX) += ibmpex.o
>>> obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o
>>> +obj-$(CONFIG_SENSORS_IBMPOWERNV_OCC)+= ibmpowernv-occ.o
>>> obj-$(CONFIG_SENSORS_IIO_HWMON) += iio_hwmon.o
>>> obj-$(CONFIG_SENSORS_INA209) += ina209.o
>>> obj-$(CONFIG_SENSORS_INA2XX) += ina2xx.o
>>> diff --git a/drivers/hwmon/ibmpowernv-occ.c b/drivers/hwmon/ibmpowernv-occ.c
>>> new file mode 100644
>>> index 0000000..97b1bbe
>>> --- /dev/null
>>> +++ b/drivers/hwmon/ibmpowernv-occ.c
>>> @@ -0,0 +1,302 @@
>>> +/*
>>> + * IBM PowerNV platform OCC inband sensors for temperature/power
>>> + * Copyright (C) 2017 IBM
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.
Unnecessary.
>>> + */
>>> +
>>> +#define DRVNAME "ibmpowernv_occ"
>>> +#define pr_fmt(fmt) DRVNAME ": " fmt
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/platform_device.h>
alphabetic order please
>>> +
>>> +#include <asm/opal.h>
>>> +
>>> +#define MAX_HWMON_ATTR_LEN 32
>>> +#define MAX_HWMON_LABEL_LEN (MAX_OCC_SENSOR_NAME_LEN * 2)
>>> +#define HWMON_ATTRS_PER_SENSOR 16
>>> +#define TO_MILLI_UNITS(x) ((x) * 1000)
>>> +#define TO_MICRO_UNITS(x) ((x) * 1000000)
>>> +
>>> +enum sensors {
>>> + TEMP,
>>> + POWER,
>>> + MAX_SENSOR_TYPE,
>>> +};
>>> +
>>> +static struct sensor_type {
>>> + const char *name;
>>> + int hwmon_id;
>>> +} sensor_types[] = {
>>> + { "temp"},
>>> + { "power"},
>>> +};
>>> +
>>> +static struct sensor_data {
>>> + u32 occ_id;
>>> + u64 offset; /* Offset to ping/pong reading buffer */
>>> + enum sensors type;
>>> + char label[MAX_HWMON_LABEL_LEN];
>>> + char name[MAX_HWMON_ATTR_LEN];
>>> + struct device_attribute attr;
>>> +} *sdata;
>>> +
>>> +static struct attribute_group sensor_attrs_group;
>>> +__ATTRIBUTE_GROUPS(sensor_attrs);
>>> +
>>> +#define show(file_name) \
>>> +static ssize_t ibmpowernv_occ_show_##file_name \
>>> +(struct device *dev, struct device_attribute *dattr, char *buf) \
>>> +{ \
>>> + struct sensor_data *sdata = container_of(dattr, \
>>> + struct sensor_data, \
>>> + attr); \
>>> + u64 val; \
>>> + int ret; \
this does or should generate a checkpatch warning.
>>> + ret = opal_occ_sensor_get_##file_name(sdata->occ_id, \
>>> + sdata->offset, \
>>> + &val); \
>>> + if (ret) \
>>> + return ret; \
>>> + if (sdata->type == TEMP) \
>>> + val = TO_MILLI_UNITS(val); \
>>> + else if (sdata->type == POWER) \
>>> + val = TO_MICRO_UNITS(val); \
>>> + return sprintf(buf, "%llu\n", val); \
>>> +}
>>> +
>>> +show(sample);
>>> +show(max);
>>> +show(min);
>>> +show(js_min);
>>> +show(js_max);
>>> +show(csm_min);
>>> +show(csm_max);
>>> +show(prof_min);
>>> +show(prof_max);
>>> +
>>> +static struct sensor_view_groups {
>>> + const char *name;
>>> + ssize_t (*show_sample)(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf);
>>> + ssize_t (*show_min)(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf);
>>> + ssize_t (*show_max)(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf);
>>> +} sensor_views[] = {
>>> + {
>>> + .name = "",
>>> + .show_sample = ibmpowernv_occ_show_sample,
>>> + .show_min = ibmpowernv_occ_show_min,
>>> + .show_max = ibmpowernv_occ_show_max
>>> + },
>>> + {
>>> + .name = "_JS",
>>> + .show_sample = ibmpowernv_occ_show_sample,
>>> + .show_min = ibmpowernv_occ_show_js_min,
>>> + .show_max = ibmpowernv_occ_show_js_max
>>> + },
>>> + { .name = "_CSM",
>>> + .show_sample = ibmpowernv_occ_show_sample,
>>> + .show_min = ibmpowernv_occ_show_csm_min,
>>> + .show_max = ibmpowernv_occ_show_csm_max
>>> + },
>>> + { .name = "_Prof",
>>> + .show_sample = ibmpowernv_occ_show_sample,
>>> + .show_min = ibmpowernv_occ_show_prof_min,
>>> + .show_max = ibmpowernv_occ_show_prof_max
>>> + },
>>> +};
>>> +
>>> +static ssize_t ibmpowernv_occ_show_label(struct device *dev,
>>> + struct device_attribute *dattr,
>>> + char *buf)
>>> +{
>>> + struct sensor_data *sdata = container_of(dattr, struct sensor_data,
>>> + attr);
>>> +
>>> + return sprintf(buf, "%s\n", sdata->label);
>>> +}
>>> +
>>> +static int ibmpowernv_occ_get_sensor_type(enum occ_sensor_type type)
>>> +{
>>> + switch (type) {
>>> + case OCC_SENSOR_TYPE_POWER:
>>> + return POWER;
>>> + case OCC_SENSOR_TYPE_TEMPERATURE:
>>> + return TEMP;
>>> + default:
>>> + return MAX_SENSOR_TYPE;
>>> + }
>>> +
>>> + return MAX_SENSOR_TYPE;
Never reached.
>>> +}
>>> +
>>> +static void ibmpowernv_occ_add_sdata(struct occ_hwmon_sensor sensor,
Passing a data structure (instead of a pointer to it) as parameter is quite
unusual. Is this really needed ? I don't really see the point.
>>> + struct sensor_data *sdata, char *name,
>>> + int hwmon_id, enum sensors type,
>>> + ssize_t (*show)(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf))
>>> +{
>>> + sdata->type = type;
>>> + sdata->occ_id = sensor.occ_id;
>>> + sdata->offset = sensor.offset;
>>> + snprintf(sdata->name, MAX_HWMON_ATTR_LEN, "%s%d_%s",
>>> + sensor_types[type].name, hwmon_id, name);
>>> + sysfs_attr_init(&sdata->attr.attr);
>>> + sdata->attr.attr.name = sdata->name;
>>> + sdata->attr.attr.mode = 0444;
>>> + sdata->attr.show = show;
>>> +}
>>> +
>>> +static void ibmpowernv_occ_add_sensor_attrs(struct occ_hwmon_sensor sensor,
>>> + int index)
>>> +{
>>> + struct attribute **attrs = sensor_attrs_group.attrs;
>>> + char attr_str[MAX_HWMON_ATTR_LEN];
>>> + enum sensors type = ibmpowernv_occ_get_sensor_type(sensor.type);
>>> + int i;
>>> +
>>> + index *= HWMON_ATTRS_PER_SENSOR;
>>> + for (i = 0; i < ARRAY_SIZE(sensor_views); i++) {
>>> + int hid = ++sensor_types[type].hwmon_id;
>>> +
>>> + /* input */
>>> + ibmpowernv_occ_add_sdata(sensor, &sdata[index], "input", hid,
>>> + type, sensor_views[i].show_sample);
>>> + attrs[index] = &sdata[index].attr.attr;
>>> + index++;
>>> +
>>> + /* min */
>>> + if (type == POWER)
>>> + snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s",
>>> + "input_lowest");
>>> + else
>>> + snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s", "min");
>>> +
>>> + ibmpowernv_occ_add_sdata(sensor, &sdata[index], attr_str, hid,
>>> + type, sensor_views[i].show_min);
>>> + attrs[index] = &sdata[index].attr.attr;
>>> + index++;
>>> +
>>> + /* max */
>>> + if (type == POWER)
>>> + snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s",
>>> + "input_highest");
>>> + else
>>> + snprintf(attr_str, MAX_HWMON_ATTR_LEN, "%s", "max");
I don't really see the point of those snprintf()s and for having attr_str[]
(instead of char *attr_str).
>>> +
>>> + ibmpowernv_occ_add_sdata(sensor, &sdata[index], attr_str, hid,
>>> + type, sensor_views[i].show_max);
>>> + attrs[index] = &sdata[index].attr.attr;
>>> + index++;
>>> +
>>> + /* label */
>>> + snprintf(sdata[index].label, MAX_HWMON_LABEL_LEN, "%s%s",
>>> + sensor.name, sensor_views[i].name);
>>> + ibmpowernv_occ_add_sdata(sensor, &sdata[index], "label", hid,
>>> + type, ibmpowernv_occ_show_label);
>>> + attrs[index] = &sdata[index].attr.attr;
>>> + index++;
>>> + }
>>> +}
>>> +
>>> +static int ibmpowernv_occ_add_device_attrs(struct platform_device *pdev)
>>> +{
>>> + struct attribute **attrs;
>>> + struct occ_hwmon_sensor *slist = NULL;
Unnecessary initialization.
>>> + int nr_sensors = 0, i;
>>> + int ret = -ENOMEM;
>>> +
>>> + slist = opal_occ_sensor_get_hwmon_list(&nr_sensors);
>>> + if (!nr_sensors)
>>> + return -ENODEV;
>>> +
>>> + if (!slist)
>>> + return ret;
>>> +
is nr_sensors guaranteed to be valid if slist is NULL ?
>>> + sdata = devm_kzalloc(&pdev->dev, nr_sensors * sizeof(*sdata) *
>>> + HWMON_ATTRS_PER_SENSOR, GFP_KERNEL);
>>> + if (!sdata)
>>> + goto out;
>>> +
>>> + attrs = devm_kzalloc(&pdev->dev, nr_sensors * sizeof(*attrs) *
>>> + HWMON_ATTRS_PER_SENSOR, GFP_KERNEL);
>>> + if (!attrs)
>>> + goto out;
>>> +
>>> + sensor_attrs_group.attrs = attrs;
>>> + for (i = 0; i < nr_sensors; i++)
>>> + ibmpowernv_occ_add_sensor_attrs(slist[i], i);
>>> +
>>> + ret = 0;
>>> +out:
>>> + kfree(slist);
>>> + return ret;
>>> +}
>>> +
>>> +static int ibmpowernv_occ_probe(struct platform_device *pdev)
>>> +{
>>> + struct device *hwmon_dev;
>>> + int err;
>>> +
>>> + err = ibmpowernv_occ_add_device_attrs(pdev);
>>> + if (err)
>>> + goto out;
>>> +
For -ENODEV and -ENOMEM you don't really want/need an error message.
>>> + hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev, DRVNAME,
>>> + NULL,
>>> + sensor_attrs_groups);
>>> +
>>> + err = PTR_ERR_OR_ZERO(hwmon_dev);
>>> +out:
>>> + if (err)
>>> + pr_warn("Failed to initialize Hwmon OCC inband sensors\n");
dev_warn() if this is really neceessary. I would suggest a more specific
error message, though. This doesn't tell if ibmpowernv_occ_add_device_attrs()
or devm_hwmon_device_register_with_groups() failed.
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static const struct platform_device_id occ_sensor_ids[] = {
>>> + { .name = "occ-inband-sensor" },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, occ_sensor_ids);
>>> +
>>> +static const struct of_device_id occ_sensor_of_ids[] = {
>>> + { .compatible = "ibm,p9-occ-inband-sensor" },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, occ_sensor_of_ids);
>>> +
>>> +static struct platform_driver ibmpowernv_occ_driver = {
>>> + .probe = ibmpowernv_occ_probe,
>>> + .id_table = occ_sensor_ids,
>>> + .driver = {
>>> + .name = DRVNAME,
>>> + .of_match_table = occ_sensor_of_ids,
>>> + },
>>> +};
>>> +
>>> +module_platform_driver(ibmpowernv_occ_driver);
>>> +
>>> +MODULE_AUTHOR("Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>");
>>> +MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 1.8.3.1
>>>
>>
>
>
More information about the Linuxppc-dev
mailing list