Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935526AbdCLWyN (ORCPT ); Sun, 12 Mar 2017 18:54:13 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:56608 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755747AbdCLWyE (ORCPT ); Sun, 12 Mar 2017 18:54:04 -0400 Subject: Re: [RFC 2/2] hwmon: powernv: Hwmon driver for OCC inband power and temperature sensors To: Shilpasri G Bhat References: <1489060155-22086-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com> <1489060155-22086-3-git-send-email-shilpa.bhat@linux.vnet.ibm.com> <20170309121009.GA23732@roeck-us.net> Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, benh@kernel.crashing.org, mpe@ellerman.id.au, paulus@samba.org, svaidy@linux.vnet.ibm.com, ego@linux.vnet.ibm.com, akshay.adiga@linux.vnet.ibm.com, andrew@aj.id.au, clg@kaod.org, maddy@linux.vnet.ibm.com, Rob Herring , Mark Rutland , Jean Delvare , Jonathan Corbet , devicetree@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org, Eddie James From: Guenter Roeck Message-ID: <3a5fa538-2f08-a7f0-f85c-97fdffd93a8b@roeck-us.net> Date: Sun, 12 Mar 2017 15:53:56 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15220 Lines: 481 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 >>> CC: Rob Herring >>> CC: Mark Rutland >>> CC: Jean Delvare >>> CC: Guenter Roeck >>> CC: Jonathan Corbet >>> CC: devicetree@vger.kernel.org >>> CC: linux-hwmon@vger.kernel.orgThat >>> CC: linux-doc@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 >>> +#include >>> +#include >>> +#include alphabetic order please >>> + >>> +#include >>> + >>> +#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 "); >>> +MODULE_DESCRIPTION("IBM POWERNV platform OCC inband sensors"); >>> +MODULE_LICENSE("GPL"); >>> -- >>> 1.8.3.1 >>> >> > >