Skip to contentSkip to conversation

hwmon: occ: Add sysfs interface

RFC PATCH NAK [email protected] 2 participants 3 messages Nov 29, 2016 – Dec 13, 2016 Hardware monitoring

lore
Share & tools
lore
#0 lore raw

RFC PATCH v6 3/9 hwmon: occ: Add sysfs interface

From: "Edward A. James" <[email protected]>

Add a generic mechanism to expose the sensors provided by the OCC in sysfs.

---
 drivers/hwmon/occ/Makefile    |   2 +-
 drivers/hwmon/occ/occ_sysfs.c | 497 ++++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_sysfs.h |  51 +++++
 3 files changed, 549 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hwmon/occ/occ_sysfs.c
 create mode 100644 drivers/hwmon/occ/occ_sysfs.h
drivers/hwmon/occ/Makefile +1 -1
index 93cb52f..a6881f9 100644
@@ -1 +1 @@
-obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
drivers/hwmon/occ/occ_sysfs.c new file +497 -0

497 lines not on this page · raw message

drivers/hwmon/occ/occ_sysfs.h new file +51 -0

51 lines not on this page · raw message

Signature · 1 line
1.9.1
#1 lore raw

Hi Eddie,

Apologies for the time it's taken to get around to reviewing these patches. Several observations though:

1. You don't appear to have sent a cover letter with this series
2. The series still isn't threaded properly, which makes it irritating to chase down in my inbox. Please reply with the exact git invocations you are using to format and send the patches so I can understand what is going on. We need to get this sorted before we send the patches upstream.
3. You've stripped my signed-off-by tags from the commits.

This last point is probably the heaviest issue of the initial observations. I created this series by squashing all of your patches down, rearranging the code and then creating new commits out of the reworked code. As all of the commits are new they didn't carry your signed-off-by tag even if they contained your work, but as you will be posting them to upstream you will need to add it anyway to track the provenance of the code.

Please add my signed-off-by tags back to all patches so their custody is properly captured.

I'm off work for a couple of days, so I will review them further as a priority when I return.

Andrew

Quoted 581 lines · On Tue, 2016-11-29 at 17:03 -0600, [email protected] wrote:
Quoted 1 line

From: "Edward A. James" <[email protected]>

Add a generic mechanism to expose the sensors provided by the OCC in sysfs.

Quoted 1 line

---
 drivers/hwmon/occ/Makefile    |   2 +- drivers/hwmon/occ/occ_sysfs.c | 497 ++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_sysfs.h |  51 +++++

 3 files changed, 549 insertions(+), 1 deletion(-)

 create mode 100644 drivers/hwmon/occ/occ_sysfs.c
 create mode 100644 drivers/hwmon/occ/occ_sysfs.h

drivers/hwmon/occ/Makefile +1 -1
index 93cb52f..a6881f9 100644
@@ -1 +1 @@
-obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
drivers/hwmon/occ/occ_sysfs.c new file +494 -0
new file mode 100644
index 0000000..aab4fbe
@@ -0,0 +1,497 @@
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "occ_sysfs.h"
+
+#define OCC_DATA_MAX   4096
+
+/* To generate attn to OCC */
+#define ATTN_DATA      0x0006B035
+/* For BMC to read/write SRAM */
+#define OCB_ADDRESS            0x0006B070
+#define OCB_DATA               0x0006B075
+#define OCB_STATUS_CONTROL_AND 0x0006B072
+#define OCB_STATUS_CONTROL_OR  0x0006B073
+
+#define RESP_DATA_LENGTH       3
+#define RESP_HEADER_OFFSET     5
+#define SENSOR_STR_OFFSET      37
+#define SENSOR_BLOCK_NUM_OFFSET        43
+#define SENSOR_BLOCK_OFFSET    45
+
+#define MAX_SENSOR_ATTR_LEN    32
+
+struct sensor_attr_data {
+       enum sensor_type type;
+       u32 hwmon_index;
+       u32 attr_id;
+       char name[MAX_SENSOR_ATTR_LEN];
+       struct device_attribute dev_attr;
+};
+
+static ssize_t show_input(struct device *dev,
+                         struct device_attribute *attr, char *buf)
+{
+       int val;
+       struct sensor_attr_data *sdata = container_of(attr,
+                                                     struct sensor_attr_data,
+                                                     dev_attr);
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       val = occ_get_sensor_value(driver->occ, sdata->type, sdata->hwmon_index - 1);
+       if (sdata->type == TEMP)
+               val *= 1000;    /* in millidegree Celsius */
+
+       return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_label(struct device *dev,
+                         struct device_attribute *attr, char *buf)
+{
+       int val;
+       struct sensor_attr_data *sdata = container_of(attr,
+                                                     struct sensor_attr_data,
+                                                     dev_attr);
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       val = occ_get_sensor_id(driver->occ, sdata->type,
+                                       sdata->hwmon_index - 1);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_caps(struct device *dev,
+                        struct device_attribute *attr, char *buf)
+{
+       int val;
+       struct caps_sensor *sensor;
+       struct sensor_attr_data *sdata = container_of(attr,
+                                                     struct sensor_attr_data,
+                                                     dev_attr);
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       sensor = occ_get_sensor(driver->occ, CAPS);
+       if (!sensor) {
+               val = -1;
+               return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+       }
+
+       val = occ_get_caps_value(driver->occ, sensor, sdata->hwmon_index - 1,
+                                        sdata->attr_id);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_update_interval(struct device *dev,
+                                   struct device_attribute *attr, char *buf)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%lu\n", driver->update_interval);
+}
+
+static ssize_t store_update_interval(struct device *dev,
+                                    struct device_attribute *attr,
+                                    const char *buf, size_t count)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+       unsigned long val;
+       int rc;
+
+       rc = kstrtoul(buf, 10, &val);
+       if (rc)
+               return rc;
+
+       driver->update_interval = val;
+       occ_set_update_interval(driver->occ, val);
+
+       return count;
+}
+
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_update_interval,
+                  store_update_interval);
+
+static ssize_t show_name(struct device *dev,
+                        struct device_attribute *attr, char *buf)
+{
+       return snprintf(buf, PAGE_SIZE - 1, "occ\n");
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static ssize_t show_user_powercap(struct device *dev,
+                                 struct device_attribute *attr, char *buf)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->user_powercap);
+}
+
+static ssize_t store_user_powercap(struct device *dev,
+                                  struct device_attribute *attr,
+                                  const char *buf, size_t count)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+       u16 val;
+       int rc;
+
+       rc = kstrtou16(buf, 10, &val);
+       if (rc)
+               return rc;
+
+       dev_dbg(dev, "set user powercap to: %u\n", val);
+       rc = occ_set_user_powercap(driver->occ, val);
+       if (rc != 0) {
+               dev_err(dev,
+                       "ERROR: Set User Powercap: wrong return status: %x\n",
+                       rc);
+               if (rc == 0x13)
+                       dev_info(dev,
+                                "ERROR: set invalid powercap value: %x\n",
+                                val);
+               return -EINVAL;
+       }
+
+       driver->user_powercap = val;
+
+       return count;
+}
+
+static DEVICE_ATTR(user_powercap, S_IWUSR | S_IRUGO, show_user_powercap,
+                  store_user_powercap);
+
+static void deinit_sensor_groups(struct device *dev,
+                                struct sensor_group *sensor_groups)
+{
+       int cnt;
+
+       for (cnt = 0; cnt < MAX_OCC_SENSOR_TYPE; cnt++) {
+               if (sensor_groups[cnt].group.attrs)
+                       devm_kfree(dev, sensor_groups[cnt].group.attrs);
+               if (sensor_groups[cnt].sattr)
+                       devm_kfree(dev, sensor_groups[cnt].sattr);
+               sensor_groups[cnt].group.attrs = NULL;
+               sensor_groups[cnt].sattr = NULL;
+       }
+}
+
+static void sensor_attr_init(struct sensor_attr_data *sdata,
+                            char *sensor_group_name,
+                            char *attr_name,
+                            ssize_t (*show)(struct device *dev,
+                                            struct device_attribute *attr,
+                                            char *buf))
+{
+       sysfs_attr_init(&sdata->dev_attr.attr);
+
+       snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
+                sensor_group_name, sdata->hwmon_index, attr_name);
+       sdata->dev_attr.attr.name = sdata->name;
+       sdata->dev_attr.attr.mode = S_IRUGO;
+       sdata->dev_attr.show = show;
+}
+
+static int create_sensor_group(struct occ_sysfs *driver,
+                              enum sensor_type type, int sensor_num)
+{
+       struct device *dev = driver->dev;
+       struct sensor_group *sensor_groups = driver->sensor_groups;
+       struct sensor_attr_data *sdata;
+       int rc, cnt;
+
+       /* each sensor has 'label' and 'input' attributes */
+       sensor_groups[type].group.attrs =
+               devm_kzalloc(dev, sizeof(struct attribute *) *
+                            sensor_num * 2 + 1, GFP_KERNEL);
+       if (!sensor_groups[type].group.attrs) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       sensor_groups[type].sattr =
+               devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
+                            sensor_num * 2, GFP_KERNEL);
+       if (!sensor_groups[type].sattr) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       for (cnt = 0; cnt < sensor_num; cnt++) {
+               sdata = &sensor_groups[type].sattr[cnt];
+               /* hwmon attributes index starts from 1 */
+               sdata->hwmon_index = cnt + 1;
+               sdata->type = type;
+               sensor_attr_init(sdata, sensor_groups[type].name, "input",
+                                show_input);
+               sensor_groups[type].group.attrs[cnt] = &sdata->dev_attr.attr;
+
+               sdata = &sensor_groups[type].sattr[cnt + sensor_num];
+               sdata->hwmon_index = cnt + 1;
+               sdata->type = type;
+               sensor_attr_init(sdata, sensor_groups[type].name, "label",
+                                show_label);
+               sensor_groups[type].group.attrs[cnt + sensor_num] =
+                       &sdata->dev_attr.attr;
+       }
+
+       rc = sysfs_create_group(&dev->kobj, &sensor_groups[type].group);
+       if (rc)
+               goto err;
+
+       return 0;
+err:
+       deinit_sensor_groups(dev, sensor_groups);
+       return rc;
+}
+
+static void caps_sensor_attr_init(struct sensor_attr_data *sdata,
+                                 char *attr_name, uint32_t hwmon_index,
+                                 uint32_t attr_id)
+{
+       sdata->type = CAPS;
+       sdata->hwmon_index = hwmon_index;
+       sdata->attr_id = attr_id;
+
+       snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
+                "caps", sdata->hwmon_index, attr_name);
+
+       sysfs_attr_init(&sdata->dev_attr.attr);
+       sdata->dev_attr.attr.name = sdata->name;
+       sdata->dev_attr.attr.mode = S_IRUGO;
+       sdata->dev_attr.show = show_caps;
+}
+
+static int create_caps_sensor_group(struct occ_sysfs *driver,
+                                   int sensor_num)
+{
+       struct device *dev = driver->dev;
+       struct sensor_group *sensor_groups = driver->sensor_groups;
+       int field_num = driver->num_caps_fields;
+       struct sensor_attr_data *sdata;
+       int i, j, rc;
+
+       sensor_groups[CAPS].group.attrs =
+               devm_kzalloc(dev, sizeof(struct attribute *) * sensor_num *
+                            field_num + 1, GFP_KERNEL);
+       if (!sensor_groups[CAPS].group.attrs) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       sensor_groups[CAPS].sattr =
+               devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
+                            sensor_num * field_num, GFP_KERNEL);
+       if (!sensor_groups[CAPS].sattr) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       for (j = 0; j < sensor_num; ++j) {
+               for (i = 0; i < field_num; ++i) {
+                       sdata = &sensor_groups[CAPS].sattr[j * field_num + i];
+                       caps_sensor_attr_init(sdata,
+                                             driver->caps_names[i],
+                                             j + 1, i);
+                       sensor_groups[CAPS].group.attrs[j * field_num + i] =
+                               &sdata->dev_attr.attr;
+               }
+       }
+
+       rc = sysfs_create_group(&dev->kobj, &sensor_groups[CAPS].group);
+       if (rc)
+               goto err;
+
+       return rc;
+err:
+       deinit_sensor_groups(dev, sensor_groups);
+       return rc;
+}
+
+static void occ_remove_hwmon_attrs(struct occ_sysfs *driver)
+{
+       struct device *dev = driver->dev;
+
+       device_remove_file(dev, &dev_attr_user_powercap);
+       device_remove_file(dev, &dev_attr_update_interval);
+       device_remove_file(dev, &dev_attr_name);
+}
+
+static int occ_create_hwmon_attrs(struct occ_sysfs *driver)
+{
+       int i, rc, id, sensor_num;
+       struct device *dev = driver->dev;
+       struct sensor_group *sensor_groups = driver->sensor_groups;
+       struct occ_blocks _resp, *resp = &_resp;
+               occ_get_response_blocks(driver->occ, resp);
+
+       for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+               resp->sensor_block_id[i] = -1;
+
+       /* read sensor data from occ. */
+       rc = occ_update_device(driver->occ);
+       if (rc != 0) {
+               dev_err(dev, "ERROR: cannot get occ sensor data: %d\n", rc);
+               return rc;
+       }
+       if (!resp->blocks)
+               return -1;
+
+       rc = device_create_file(dev, &dev_attr_name);
+       if (rc)
+               goto error;
+
+       rc = device_create_file(dev, &dev_attr_update_interval);
+       if (rc)
+               goto error;
+
+       if (resp->sensor_block_id[CAPS] >= 0) {
+               /* user powercap: only for master OCC */
+               rc = device_create_file(dev, &dev_attr_user_powercap);
+               if (rc)
+                       goto error;
+       }
+
+       sensor_groups[FREQ].name = "freq";
+       sensor_groups[TEMP].name = "temp";
+       sensor_groups[POWER].name = "power";
+       sensor_groups[CAPS].name =  "caps";
+
+       for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
+               id = resp->sensor_block_id[i];
+               if (id < 0)
+                       continue;
+
+               sensor_num = resp->blocks[id].sensor_num;
+               if (i == CAPS)
+                       rc = create_caps_sensor_group(driver, sensor_num);
+               else
+                       rc = create_sensor_group(driver, i, sensor_num);
+               if (rc)
+                       goto error;
+       }
+
+       return 0;
+
+error:
+       dev_err(dev, "ERROR: cannot create hwmon attributes: %d\n", rc);
+       occ_remove_hwmon_attrs(driver);
+       return rc;
+}
+
+static ssize_t show_occ_online(struct device *dev,
+                              struct device_attribute *attr, char *buf)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->occ_online);
+}
+
+static ssize_t store_occ_online(struct device *dev,
+                               struct device_attribute *attr,
+                               const char *buf, size_t count)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+       unsigned long val;
+       int rc;
+
+       rc = kstrtoul(buf, 10, &val);
+       if (rc)
+               return rc;
+
+       if (val == 1) {
+               if (driver->occ_online)
+                       return count;
+
+               driver->dev = hwmon_device_register(dev);
+               if (IS_ERR(driver->dev))
+                       return PTR_ERR(driver->dev);
+
+               dev_set_drvdata(driver->dev, driver);
+
+               rc = occ_create_hwmon_attrs(driver);
+               if (rc) {
+                       hwmon_device_unregister(driver->dev);
+                       driver->dev = NULL;
+                       return rc;
+               }
+       } else if (val == 0) {
+               if (!driver->occ_online)
+                       return count;
+
+               occ_remove_hwmon_attrs(driver);
+               hwmon_device_unregister(driver->dev);
+               driver->dev = NULL;
+       } else
+               return -EINVAL;
+
+       driver->occ_online = val;
+       return count;
+}
+
+static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
+                  store_occ_online);
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+                                 struct occ_sysfs_config *config)
+{
+       struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+       int rc;
+
+       if (!hwmon)
+               return ERR_PTR(-ENOMEM);
+
+       hwmon->occ = occ;
+       hwmon->num_caps_fields = config->num_caps_fields;
+       hwmon->caps_names = config->caps_names;
+
+       dev_set_drvdata(dev, hwmon);
+
+       rc = device_create_file(dev, &dev_attr_online);
+       if (rc)
+               return ERR_PTR(rc);
+
+       return hwmon;
+}
+EXPORT_SYMBOL(occ_sysfs_start);
+
+int occ_sysfs_stop(struct occ_sysfs *driver)
+{
+       occ_remove_hwmon_attrs(driver);
+       hwmon_device_unregister(driver->dev);
+
+       device_remove_file(driver->dev, &dev_attr_online);
+
+       return 0;
+}
+EXPORT_SYMBOL(occ_sysfs_stop);
+
Quoted 1 line

+MODULE_AUTHOR("Eddie James <[email protected]>");

drivers/hwmon/occ/occ_sysfs.c new file +2 -0
+MODULE_DESCRIPTION("OCC sysfs driver");
+MODULE_LICENSE("GPL");
drivers/hwmon/occ/occ_sysfs.h new file +51 -0
new file mode 100644
index 0000000..e0219b6
@@ -0,0 +1,51 @@
+#ifndef OCC_SYSFS_H
+#define OCC_SYSFS_H
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include "occ.h"
+
+struct sensor_group {
+       char *name;
+       struct sensor_attr_data *sattr;
+       struct attribute_group group;
+};
+
+struct occ_sysfs_config {
+       unsigned int num_caps_fields;
+       char **caps_names;
+};
+
+struct occ_sysfs {
+       struct device *dev;
+       struct occ *occ;
+
+       u16 user_powercap;
+       bool occ_online;
+       struct sensor_group sensor_groups[MAX_OCC_SENSOR_TYPE];
+       unsigned long update_interval;
+       unsigned int num_caps_fields;
+       char **caps_names;
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+                                 struct occ_sysfs_config *config);
+
+int occ_sysfs_stop(struct occ_sysfs *driver);
+
+#endif
#2 lore raw
In reply to [email protected]

Hi Eddie,

Quoted 66 lines · On Tue, 2016-11-29 at 17:03 -0600, [email protected] wrote:
Quoted 1 line

From: "Edward A. James" <[email protected]>

Add a generic mechanism to expose the sensors provided by the OCC in sysfs.

Quoted 1 line

---
 drivers/hwmon/occ/Makefile    |   2 +- drivers/hwmon/occ/occ_sysfs.c | 497 ++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_sysfs.h |  51 +++++

 3 files changed, 549 insertions(+), 1 deletion(-)

 create mode 100644 drivers/hwmon/occ/occ_sysfs.c
 create mode 100644 drivers/hwmon/occ/occ_sysfs.h

drivers/hwmon/occ/Makefile +1 -1
index 93cb52f..a6881f9 100644
@@ -1 +1 @@
-obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o occ_sysfs.o
drivers/hwmon/occ/occ_sysfs.c new file +39 -0
new file mode 100644
index 0000000..aab4fbe
@@ -0,0 +1,497 @@
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+
+#include "occ_sysfs.h"
+
+#define OCC_DATA_MAX   4096
+
+/* To generate attn to OCC */
+#define ATTN_DATA      0x0006B035
+/* For BMC to read/write SRAM */
+#define OCB_ADDRESS            0x0006B070
+#define OCB_DATA               0x0006B075

drivers/hwmon/occ/occ_sysfs.c line 49

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+#define OCB_STATUS_CONTROL_AND 0x0006B072
+#define OCB_STATUS_CONTROL_OR  0x0006B073
+
+#define RESP_DATA_LENGTH       3
+#define RESP_HEADER_OFFSET     5
+#define SENSOR_STR_OFFSET      37
+#define SENSOR_BLOCK_NUM_OFFSET        43
+#define SENSOR_BLOCK_OFFSET    45
+
+#define MAX_SENSOR_ATTR_LEN    32

A number of these #defines are unused. Please audit them and remove any that are unnecessary.

Quoted 28 lines
drivers/hwmon/occ/occ_sysfs.c new file +28 -0
+
+struct sensor_attr_data {
+       enum sensor_type type;
+       u32 hwmon_index;
+       u32 attr_id;
+       char name[MAX_SENSOR_ATTR_LEN];
+       struct device_attribute dev_attr;
+};
+
+static ssize_t show_input(struct device *dev,
+                         struct device_attribute *attr, char *buf)
+{
+       int val;
+       struct sensor_attr_data *sdata = container_of(attr,
+                                                     struct sensor_attr_data,
+                                                     dev_attr);
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       val = occ_get_sensor_value(driver->occ, sdata->type, sdata->hwmon_index - 1);
+       if (sdata->type == TEMP)
+               val *= 1000;    /* in millidegree Celsius */
+
+       return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_label(struct device *dev,
+                         struct device_attribute *attr, char *buf)
+{

drivers/hwmon/occ/occ_sysfs.c line 87

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+       int val;
+       struct sensor_attr_data *sdata = container_of(attr,
+                                                     struct sensor_attr_data,
+                                                     dev_attr);
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       val = occ_get_sensor_id(driver->occ, sdata->type,
+                                       sdata->hwmon_index - 1);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);

Can we improve this at all? What can I do with this number? For instance, can I use it to look up the sensor in the OCC documentation? The kernel documentation suggests this should be a text string, which I guess it is once you render it down with snprintf, but that's a bit obscure :)

At least, can you please add some comments about what we can expect this value to mean?

Quoted 76 lines
drivers/hwmon/occ/occ_sysfs.c new file +76 -0
+}
+
+static ssize_t show_caps(struct device *dev,
+                        struct device_attribute *attr, char *buf)
+{
+       int val;
+       struct caps_sensor *sensor;
+       struct sensor_attr_data *sdata = container_of(attr,
+                                                     struct sensor_attr_data,
+                                                     dev_attr);
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       sensor = occ_get_sensor(driver->occ, CAPS);
+       if (!sensor) {
+               val = -1;
+               return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+       }
+
+       val = occ_get_caps_value(driver->occ, sensor, sdata->hwmon_index - 1,
+                                        sdata->attr_id);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
+}
+
+static ssize_t show_update_interval(struct device *dev,
+                                   struct device_attribute *attr, char *buf)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%lu\n", driver->update_interval);
+}
+
+static ssize_t store_update_interval(struct device *dev,
+                                    struct device_attribute *attr,
+                                    const char *buf, size_t count)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+       unsigned long val;
+       int rc;
+
+       rc = kstrtoul(buf, 10, &val);
+       if (rc)
+               return rc;
+
+       driver->update_interval = val;
+       occ_set_update_interval(driver->occ, val);
+
+       return count;
+}
+
+static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, show_update_interval,
+                  store_update_interval);
+
+static ssize_t show_name(struct device *dev,
+                        struct device_attribute *attr, char *buf)
+{
+       return snprintf(buf, PAGE_SIZE - 1, "occ\n");
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static ssize_t show_user_powercap(struct device *dev,
+                                 struct device_attribute *attr, char *buf)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->user_powercap);
+}
+
+static ssize_t store_user_powercap(struct device *dev,
+                                  struct device_attribute *attr,
+                                  const char *buf, size_t count)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+       u16 val;
+       int rc;

drivers/hwmon/occ/occ_sysfs.c line 173

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+
+       rc = kstrtou16(buf, 10, &val);
+       if (rc)
+               return rc;
+
+       dev_dbg(dev, "set user powercap to: %u\n", val);
+       rc = occ_set_user_powercap(driver->occ, val);
+       if (rc != 0) {
+               dev_err(dev,
+                       "ERROR: Set User Powercap: wrong return status: %x\n",

Should probably be '0x%x'.

drivers/hwmon/occ/occ_sysfs.c line 177

Quoted 4 lines
drivers/hwmon/occ/occ_sysfs.c new file +4 -0
+                       rc);
+               if (rc == 0x13)
+                       dev_info(dev,
+                                "ERROR: set invalid powercap value: %x\n",

As above.

Quoted 6 lines
drivers/hwmon/occ/occ_sysfs.c new file +6 -0
+                                val);
+               return -EINVAL;
+       }
+
+       driver->user_powercap = val;
+

drivers/hwmon/occ/occ_sysfs.c line 193

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+       return count;
+}
+
+static DEVICE_ATTR(user_powercap, S_IWUSR | S_IRUGO, show_user_powercap,
+                  store_user_powercap);
+
+static void deinit_sensor_groups(struct device *dev,
+                                struct sensor_group *sensor_groups)
+{
+       int cnt;

Nit: I'd prefer 'i' here

Quoted 43 lines
drivers/hwmon/occ/occ_sysfs.c new file +43 -0
+
+       for (cnt = 0; cnt < MAX_OCC_SENSOR_TYPE; cnt++) {
+               if (sensor_groups[cnt].group.attrs)
+                       devm_kfree(dev, sensor_groups[cnt].group.attrs);
+               if (sensor_groups[cnt].sattr)
+                       devm_kfree(dev, sensor_groups[cnt].sattr);
+               sensor_groups[cnt].group.attrs = NULL;
+               sensor_groups[cnt].sattr = NULL;
+       }
+}
+
+static void sensor_attr_init(struct sensor_attr_data *sdata,
+                            char *sensor_group_name,
+                            char *attr_name,
+                            ssize_t (*show)(struct device *dev,
+                                            struct device_attribute *attr,
+                                            char *buf))
+{
+       sysfs_attr_init(&sdata->dev_attr.attr);
+
+       snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
+                sensor_group_name, sdata->hwmon_index, attr_name);
+       sdata->dev_attr.attr.name = sdata->name;
+       sdata->dev_attr.attr.mode = S_IRUGO;
+       sdata->dev_attr.show = show;
+}
+
+static int create_sensor_group(struct occ_sysfs *driver,
+                              enum sensor_type type, int sensor_num)
+{
+       struct device *dev = driver->dev;
+       struct sensor_group *sensor_groups = driver->sensor_groups;
+       struct sensor_attr_data *sdata;
+       int rc, cnt;
+
+       /* each sensor has 'label' and 'input' attributes */
+       sensor_groups[type].group.attrs =
+               devm_kzalloc(dev, sizeof(struct attribute *) *
+                            sensor_num * 2 + 1, GFP_KERNEL);
+       if (!sensor_groups[type].group.attrs) {
+               rc = -ENOMEM;
+               goto err;
+       }

drivers/hwmon/occ/occ_sysfs.c line 246

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+
+       sensor_groups[type].sattr =
+               devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
+                            sensor_num * 2, GFP_KERNEL);
+       if (!sensor_groups[type].sattr) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       for (cnt = 0; cnt < sensor_num; cnt++) {

Same nit as above - 'i' is pretty standard for this

Quoted 87 lines
drivers/hwmon/occ/occ_sysfs.c new file +87 -0
+               sdata = &sensor_groups[type].sattr[cnt];
+               /* hwmon attributes index starts from 1 */
+               sdata->hwmon_index = cnt + 1;
+               sdata->type = type;
+               sensor_attr_init(sdata, sensor_groups[type].name, "input",
+                                show_input);
+               sensor_groups[type].group.attrs[cnt] = &sdata->dev_attr.attr;
+
+               sdata = &sensor_groups[type].sattr[cnt + sensor_num];
+               sdata->hwmon_index = cnt + 1;
+               sdata->type = type;
+               sensor_attr_init(sdata, sensor_groups[type].name, "label",
+                                show_label);
+               sensor_groups[type].group.attrs[cnt + sensor_num] =
+                       &sdata->dev_attr.attr;
+       }
+
+       rc = sysfs_create_group(&dev->kobj, &sensor_groups[type].group);
+       if (rc)
+               goto err;
+
+       return 0;
+err:
+       deinit_sensor_groups(dev, sensor_groups);
+       return rc;
+}
+
+static void caps_sensor_attr_init(struct sensor_attr_data *sdata,
+                                 char *attr_name, uint32_t hwmon_index,
+                                 uint32_t attr_id)
+{
+       sdata->type = CAPS;
+       sdata->hwmon_index = hwmon_index;
+       sdata->attr_id = attr_id;
+
+       snprintf(sdata->name, MAX_SENSOR_ATTR_LEN, "%s%d_%s",
+                "caps", sdata->hwmon_index, attr_name);
+
+       sysfs_attr_init(&sdata->dev_attr.attr);
+       sdata->dev_attr.attr.name = sdata->name;
+       sdata->dev_attr.attr.mode = S_IRUGO;
+       sdata->dev_attr.show = show_caps;
+}
+
+static int create_caps_sensor_group(struct occ_sysfs *driver,
+                                   int sensor_num)
+{
+       struct device *dev = driver->dev;
+       struct sensor_group *sensor_groups = driver->sensor_groups;
+       int field_num = driver->num_caps_fields;
+       struct sensor_attr_data *sdata;
+       int i, j, rc;
+
+       sensor_groups[CAPS].group.attrs =
+               devm_kzalloc(dev, sizeof(struct attribute *) * sensor_num *
+                            field_num + 1, GFP_KERNEL);
+       if (!sensor_groups[CAPS].group.attrs) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       sensor_groups[CAPS].sattr =
+               devm_kzalloc(dev, sizeof(struct sensor_attr_data) *
+                            sensor_num * field_num, GFP_KERNEL);
+       if (!sensor_groups[CAPS].sattr) {
+               rc = -ENOMEM;
+               goto err;
+       }
+
+       for (j = 0; j < sensor_num; ++j) {
+               for (i = 0; i < field_num; ++i) {
+                       sdata = &sensor_groups[CAPS].sattr[j * field_num + i];
+                       caps_sensor_attr_init(sdata,
+                                             driver->caps_names[i],
+                                             j + 1, i);
+                       sensor_groups[CAPS].group.attrs[j * field_num + i] =
+                               &sdata->dev_attr.attr;
+               }
+       }
+
+       rc = sysfs_create_group(&dev->kobj, &sensor_groups[CAPS].group);
+       if (rc)
+               goto err;
+
+       return rc;
+err:
+       deinit_sensor_groups(dev, sensor_groups);

drivers/hwmon/occ/occ_sysfs.c line 343

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+       return rc;
+}
+
+static void occ_remove_hwmon_attrs(struct occ_sysfs *driver)
+{
+       struct device *dev = driver->dev;
+
+       device_remove_file(dev, &dev_attr_user_powercap);
+       device_remove_file(dev, &dev_attr_update_interval);
+       device_remove_file(dev, &dev_attr_name);

We may not have registered this and so unconditionally removing it felt
a bit questionable, but looking at the API's implementation the errors
are just swallowed anyway.

Quoted 7 lines
drivers/hwmon/occ/occ_sysfs.c new file +7 -0
+}
+
+static int occ_create_hwmon_attrs(struct occ_sysfs *driver)
+{
+       int i, rc, id, sensor_num;
+       struct device *dev = driver->dev;
+       struct sensor_group *sensor_groups = driver->sensor_groups;

drivers/hwmon/occ/occ_sysfs.c line 360

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.c new file +10 -0
+       struct occ_blocks _resp, *resp = &_resp;
+               occ_get_response_blocks(driver->occ, resp);
+
+       for (i = 0; i < MAX_OCC_SENSOR_TYPE; ++i)
+               resp->sensor_block_id[i] = -1;
+
+       /* read sensor data from occ. */
+       rc = occ_update_device(driver->occ);
+       if (rc != 0) {
+               dev_err(dev, "ERROR: cannot get occ sensor data: %d\n", rc);

Please drop the "ERROR:" prefix and audit the rest of the patch for the issue.

drivers/hwmon/occ/occ_sysfs.c line 364

Quoted 4 lines
drivers/hwmon/occ/occ_sysfs.c new file +4 -0
+               return rc;
+       }
+       if (!resp->blocks)
+               return -1;

Please pick a sensible standard error code to use here.

Quoted 180 lines
drivers/hwmon/occ/occ_sysfs.c new file +130 -0
+
+       rc = device_create_file(dev, &dev_attr_name);
+       if (rc)
+               goto error;
+
+       rc = device_create_file(dev, &dev_attr_update_interval);
+       if (rc)
+               goto error;
+
+       if (resp->sensor_block_id[CAPS] >= 0) {
+               /* user powercap: only for master OCC */
+               rc = device_create_file(dev, &dev_attr_user_powercap);
+               if (rc)
+                       goto error;
+       }
+
+       sensor_groups[FREQ].name = "freq";
+       sensor_groups[TEMP].name = "temp";
+       sensor_groups[POWER].name = "power";
+       sensor_groups[CAPS].name =  "caps";
+
+       for (i = 0; i < MAX_OCC_SENSOR_TYPE; i++) {
+               id = resp->sensor_block_id[i];
+               if (id < 0)
+                       continue;
+
+               sensor_num = resp->blocks[id].sensor_num;
+               if (i == CAPS)
+                       rc = create_caps_sensor_group(driver, sensor_num);
+               else
+                       rc = create_sensor_group(driver, i, sensor_num);
+               if (rc)
+                       goto error;
+       }
+
+       return 0;
+
+error:
+       dev_err(dev, "ERROR: cannot create hwmon attributes: %d\n", rc);
+       occ_remove_hwmon_attrs(driver);
+       return rc;
+}
+
+static ssize_t show_occ_online(struct device *dev,
+                              struct device_attribute *attr, char *buf)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+
+       return snprintf(buf, PAGE_SIZE - 1, "%u\n", driver->occ_online);
+}
+
+static ssize_t store_occ_online(struct device *dev,
+                               struct device_attribute *attr,
+                               const char *buf, size_t count)
+{
+       struct occ_sysfs *driver = dev_get_drvdata(dev);
+       unsigned long val;
+       int rc;
+
+       rc = kstrtoul(buf, 10, &val);
+       if (rc)
+               return rc;
+
+       if (val == 1) {
+               if (driver->occ_online)
+                       return count;
+
+               driver->dev = hwmon_device_register(dev);
+               if (IS_ERR(driver->dev))
+                       return PTR_ERR(driver->dev);
+
+               dev_set_drvdata(driver->dev, driver);
+
+               rc = occ_create_hwmon_attrs(driver);
+               if (rc) {
+                       hwmon_device_unregister(driver->dev);
+                       driver->dev = NULL;
+                       return rc;
+               }
+       } else if (val == 0) {
+               if (!driver->occ_online)
+                       return count;
+
+               occ_remove_hwmon_attrs(driver);
+               hwmon_device_unregister(driver->dev);
+               driver->dev = NULL;
+       } else
+               return -EINVAL;
+
+       driver->occ_online = val;
+       return count;
+}
+
+static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
+                  store_occ_online);
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+                                 struct occ_sysfs_config *config)
+{
+       struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+       int rc;
+
+       if (!hwmon)
+               return ERR_PTR(-ENOMEM);
+
+       hwmon->occ = occ;
+       hwmon->num_caps_fields = config->num_caps_fields;
+       hwmon->caps_names = config->caps_names;
+
+       dev_set_drvdata(dev, hwmon);
+
+       rc = device_create_file(dev, &dev_attr_online);
+       if (rc)
+               return ERR_PTR(rc);
+
+       return hwmon;
+}
+EXPORT_SYMBOL(occ_sysfs_start);
+
+int occ_sysfs_stop(struct occ_sysfs *driver)
+{
+       occ_remove_hwmon_attrs(driver);
+       hwmon_device_unregister(driver->dev);
+
+       device_remove_file(driver->dev, &dev_attr_online);
+
+       return 0;
+}
+EXPORT_SYMBOL(occ_sysfs_stop);
+
Quoted 1 line

+MODULE_AUTHOR("Eddie James <[email protected]>");

drivers/hwmon/occ/occ_sysfs.c new file +2 -0
+MODULE_DESCRIPTION("OCC sysfs driver");
+MODULE_LICENSE("GPL");
drivers/hwmon/occ/occ_sysfs.h new file +41 -0
new file mode 100644
index 0000000..e0219b6
@@ -0,0 +1,51 @@
+#ifndef OCC_SYSFS_H
+#define OCC_SYSFS_H
+/*
+ * occ_sysfs.c - OCC sysfs interface
+ *
+ * This file contains the methods and data structures for the OCC hwmon driver.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * 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.
+ */
+
+#include "occ.h"
+
+struct sensor_group {
+       char *name;
+       struct sensor_attr_data *sattr;
+       struct attribute_group group;
+};
+
+struct occ_sysfs_config {
+       unsigned int num_caps_fields;
+       char **caps_names;
+};
+
+struct occ_sysfs {
+       struct device *dev;
+       struct occ *occ;
+
+       u16 user_powercap;
+       bool occ_online;
+       struct sensor_group sensor_groups[MAX_OCC_SENSOR_TYPE];
+       unsigned long update_interval;

drivers/hwmon/occ/occ_sysfs.h line 51

Quoted 10 lines
drivers/hwmon/occ/occ_sysfs.h new file +10 -0
+       unsigned int num_caps_fields;
+       char **caps_names;
+};
+
+struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
+                                 struct occ_sysfs_config *config);
+
+int occ_sysfs_stop(struct occ_sysfs *driver);
+
+#endif

Andrew