* [PATCH 1/7] hwmon: (core) New hwmon registration API
@ 2016-06-26 3:26 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Up to now, each hwmon driver has to implement its own sysfs attributes.
This requires a lot of template code, and distracts from the driver's core
function to read and write chip registers.
To be able to reduce driver complexity, move sensor attribute handling
and thermal zone registration into hwmon core. By using the new API,
driver size is typically reduced by 20-50% depending on driver complexity
and the number of sysfs attributes supported.
With this patch, the new API only supports thermal sensors. Support for
other sensor types will be added with subsequent patches.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/hwmon.c | 485 ++++++++++++++++++++++++++++++++++++++++++++++----
include/linux/hwmon.h | 122 +++++++++++++
2 files changed, 574 insertions(+), 33 deletions(-)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index a26c385a435b..9530644ae297 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -12,17 +12,16 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/module.h>
+#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/err.h>
-#include <linux/slab.h>
-#include <linux/kdev_t.h>
-#include <linux/idr.h>
#include <linux/hwmon.h>
-#include <linux/gfp.h>
-#include <linux/spinlock.h>
+#include <linux/idr.h>
+#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/slab.h>
#include <linux/string.h>
+#include <linux/thermal.h>
#define HWMON_ID_PREFIX "hwmon"
#define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
@@ -30,9 +29,33 @@
struct hwmon_device {
const char *name;
struct device dev;
+ const struct hwmon_chip_info *chip;
+
+ struct attribute_group group;
+ const struct attribute_group **groups;
};
#define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
+struct hwmon_device_attribute {
+ struct device_attribute dev_attr;
+ const struct hwmon_ops *ops;
+ enum hwmon_sensor_types type;
+ u32 attr;
+ int index;
+};
+#define to_hwmon_attr(d) \
+ container_of(d, struct hwmon_device_attribute, dev_attr)
+
+/*
+ * Thermal zone information
+ * In addition to the reference to the hwmon device,
+ * also provides the sensor index.
+ */
+struct hwmon_thermal_data {
+ struct hwmon_device *hwdev; /* Reference to hwmon device */
+ int index; /* sensor index */
+};
+
static ssize_t
show_name(struct device *dev, struct device_attribute *attr, char *buf)
{
@@ -80,25 +103,280 @@ static struct class hwmon_class = {
static DEFINE_IDA(hwmon_ida);
-/**
- * hwmon_device_register_with_groups - register w/ hwmon
- * @dev: the parent device
- * @name: hwmon name attribute
- * @drvdata: driver data to attach to created device
- * @groups: List of attribute groups to create
- *
- * hwmon_device_unregister() must be called when the device is no
- * longer needed.
- *
- * Returns the pointer to the new device.
- */
-struct device *
-hwmon_device_register_with_groups(struct device *dev, const char *name,
- void *drvdata,
- const struct attribute_group **groups)
+/* Thermal zone handling */
+
+static int hwmon_thermal_get_temp(void *data, int *temp)
+{
+ struct hwmon_thermal_data *tdata = data;
+ struct hwmon_device *hwdev = tdata->hwdev;
+ int ret;
+ long t;
+
+ ret = hwdev->chip->ops->read(&hwdev->dev, hwmon_temp, hwmon_temp_input,
+ tdata->index, &t);
+ if (ret < 0)
+ return ret;
+
+ *temp = t;
+
+ return 0;
+}
+
+static struct thermal_zone_of_device_ops hwmon_thermal_ops = {
+ .get_temp = hwmon_thermal_get_temp,
+};
+
+static int hwmon_thermal_add_sensor(struct device *dev,
+ struct hwmon_device *hwdev,
+ int index)
+{
+ struct hwmon_thermal_data *tdata;
+
+ tdata = devm_kzalloc(dev, sizeof(*tdata), GFP_KERNEL);
+ if (!tdata)
+ return -ENOMEM;
+
+ tdata->hwdev = hwdev;
+ tdata->index = index;
+
+ devm_thermal_zone_of_sensor_register(&hwdev->dev, index, tdata,
+ &hwmon_thermal_ops);
+
+ return 0;
+}
+
+/* sysfs attribute management */
+
+static ssize_t hwmon_attr_show(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+ long val;
+ int ret;
+
+ ret = hattr->ops->read(dev, hattr->type, hattr->attr, hattr->index,
+ &val);
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t hwmon_attr_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+ long val;
+ int ret;
+
+ ret = kstrtol(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ ret = hattr->ops->write(dev, hattr->type, hattr->attr, hattr->index,
+ val);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static int hwmon_attr_base(enum hwmon_sensor_types type)
+{
+ if (type == hwmon_in)
+ return 0;
+ return 1;
+}
+
+static struct attribute *hwmon_genattr(struct device *dev,
+ const void *drvdata,
+ enum hwmon_sensor_types type,
+ u32 attr,
+ int index,
+ const char *template,
+ const struct hwmon_ops *ops)
+{
+ struct hwmon_device_attribute *hattr;
+ struct device_attribute *dattr;
+ struct attribute *a;
+ umode_t mode;
+ char *name;
+
+ if (!template)
+ return ERR_PTR(-EINVAL);
+
+ mode = ops->is_visible(drvdata, type, attr, index);
+ if (!mode)
+ return ERR_PTR(-ENOENT);
+
+ if ((mode & S_IRUGO) && !ops->read)
+ return ERR_PTR(-EINVAL);
+ if ((mode & S_IWUGO) && !ops->write)
+ return ERR_PTR(-EINVAL);
+
+ if (type == hwmon_chip) {
+ name = (char *)template;
+ } else {
+ name = devm_kzalloc(dev, strlen(template) + 16, GFP_KERNEL);
+ if (!name)
+ return ERR_PTR(-ENOMEM);
+ scnprintf(name, strlen(template) + 16, template,
+ index + hwmon_attr_base(type));
+ }
+
+ hattr = devm_kzalloc(dev, sizeof(*hattr), GFP_KERNEL);
+ if (!hattr)
+ return ERR_PTR(-ENOMEM);
+
+ hattr->type = type;
+ hattr->attr = attr;
+ hattr->index = index;
+ hattr->ops = ops;
+
+ dattr = &hattr->dev_attr;
+ if (mode & S_IRUGO)
+ dattr->show = hwmon_attr_show;
+ if (mode & S_IWUGO)
+ dattr->store = hwmon_attr_store;
+
+ a = &dattr->attr;
+ sysfs_attr_init(a);
+ a->name = name;
+ a->mode = mode;
+
+ return a;
+}
+
+static const char * const hwmon_chip_attr_templates[] = {
+ [hwmon_chip_temp_reset_history] = "temp_reset_history",
+ [hwmon_chip_update_interval] = "update_interval",
+ [hwmon_chip_alarms] = "alarms",
+};
+
+static const char * const hwmon_temp_attr_templates[] = {
+ [hwmon_temp_input] = "temp%d_input",
+ [hwmon_temp_type] = "temp%d_type",
+ [hwmon_temp_lcrit] = "temp%d_lcrit",
+ [hwmon_temp_lcrit_hyst] = "temp%d_lcrit_hyst",
+ [hwmon_temp_min] = "temp%d_min",
+ [hwmon_temp_min_hyst] = "temp%d_min_hyst",
+ [hwmon_temp_max] = "temp%d_max",
+ [hwmon_temp_max_hyst] = "temp%d_max_hyst",
+ [hwmon_temp_crit] = "temp%d_crit",
+ [hwmon_temp_crit_hyst] = "temp%d_crit_hyst",
+ [hwmon_temp_emergency] = "temp%d_emergency",
+ [hwmon_temp_emergency_hyst] = "temp%d_emergency_hyst",
+ [hwmon_temp_alarm] = "temp%d_alarm",
+ [hwmon_temp_lcrit_alarm] = "temp%d_lcrit_alarm",
+ [hwmon_temp_min_alarm] = "temp%d_min_alarm",
+ [hwmon_temp_max_alarm] = "temp%d_max_alarm",
+ [hwmon_temp_crit_alarm] = "temp%d_crit_alarm",
+ [hwmon_temp_emergency_alarm] = "temp%d_emergency_alarm",
+ [hwmon_temp_fault] = "temp%d_fault",
+ [hwmon_temp_offset] = "temp%d_offset",
+ [hwmon_temp_label] = "temp%d_label",
+ [hwmon_temp_lowest] = "temp%d_lowest",
+ [hwmon_temp_highest] = "temp%d_highest",
+ [hwmon_temp_reset_history] = "temp%d_reset_history",
+};
+
+static const char * const *__templates[] = {
+ [hwmon_chip] = hwmon_chip_attr_templates,
+ [hwmon_temp] = hwmon_temp_attr_templates,
+};
+
+static const int __templates_size[] = {
+ [hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
+ [hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
+};
+
+static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
+{
+ int i, n;
+
+ for (i = n = 0; info->config[i]; i++)
+ n += hweight32(info->config[i]);
+
+ return n;
+}
+
+static int hwmon_genattrs(struct device *dev,
+ const void *drvdata,
+ struct attribute **attrs,
+ const struct hwmon_ops *ops,
+ const struct hwmon_channel_info *info)
+{
+ const char * const *templates;
+ int template_size;
+ int i, aindex = 0;
+
+ if (info->type >= ARRAY_SIZE(__templates))
+ return -EINVAL;
+
+ templates = __templates[info->type];
+ template_size = __templates_size[info->type];
+
+ for (i = 0; info->config[i]; i++) {
+ u32 attr_mask = info->config[i];
+ u32 attr;
+
+ while (attr_mask) {
+ struct attribute *a;
+
+ attr = __ffs(attr_mask);
+ attr_mask &= ~BIT(attr);
+ if (attr >= template_size)
+ return -EINVAL;
+ a = hwmon_genattr(dev, drvdata, info->type, attr, i,
+ templates[attr], ops);
+ if (IS_ERR(a)) {
+ if (PTR_ERR(a) != -ENOENT)
+ return PTR_ERR(a);
+ continue;
+ }
+ attrs[aindex++] = a;
+ }
+ }
+ return aindex;
+}
+
+static struct attribute **
+__hwmon_create_attrs(struct device *dev, const void *drvdata,
+ const struct hwmon_chip_info *chip)
+{
+ int ret, i, aindex = 0, nattrs = 0;
+ struct attribute **attrs;
+
+ for (i = 0; chip->info[i]; i++)
+ nattrs += hwmon_num_channel_attrs(chip->info[i]);
+
+ if (nattrs == 0)
+ return ERR_PTR(-EINVAL);
+
+ attrs = devm_kcalloc(dev, nattrs + 1, sizeof(*attrs), GFP_KERNEL);
+ if (attrs == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; chip->info[i]; i++) {
+ ret = hwmon_genattrs(dev, drvdata, &attrs[aindex], chip->ops,
+ chip->info[i]);
+ if (ret < 0)
+ return ERR_PTR(ret);
+ aindex += ret;
+ }
+
+ return attrs;
+}
+
+static struct device *
+__hwmon_device_register(struct device *dev, const char *name, void *drvdata,
+ const struct hwmon_chip_info *chip,
+ const struct attribute_group **groups)
{
struct hwmon_device *hwdev;
- int err, id;
+ struct device *hdev;
+ int i, j, err, id;
/* Do not accept invalid characters in hwmon name attribute */
if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
@@ -114,28 +392,128 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
goto ida_remove;
}
+ hdev = &hwdev->dev;
+
+ if (chip && chip->ops->is_visible) {
+ struct attribute **attrs;
+ int ngroups = 2;
+
+ if (groups)
+ for (i = 0; groups[i]; i++)
+ ngroups++;
+
+ hwdev->groups = devm_kcalloc(dev, ngroups, sizeof(*groups),
+ GFP_KERNEL);
+ if (hwdev->groups == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ attrs = __hwmon_create_attrs(dev, drvdata, chip);
+ if (IS_ERR(attrs)) {
+ err = PTR_ERR(attrs);
+ goto free_hwmon;
+ }
+
+ hwdev->group.attrs = attrs;
+ ngroups = 0;
+ hwdev->groups[ngroups++] = &hwdev->group;
+
+ if (groups) {
+ for (i = 0; groups[i]; i++)
+ hwdev->groups[ngroups++] = groups[i];
+ }
+
+ hdev->groups = hwdev->groups;
+ } else {
+ hdev->groups = groups;
+ }
+
hwdev->name = name;
- hwdev->dev.class = &hwmon_class;
- hwdev->dev.parent = dev;
- hwdev->dev.groups = groups;
- hwdev->dev.of_node = dev ? dev->of_node : NULL;
- dev_set_drvdata(&hwdev->dev, drvdata);
- dev_set_name(&hwdev->dev, HWMON_ID_FORMAT, id);
- err = device_register(&hwdev->dev);
+ hdev->class = &hwmon_class;
+ hdev->parent = dev;
+ hdev->of_node = dev ? dev->of_node : NULL;
+ hwdev->chip = chip;
+ dev_set_drvdata(hdev, drvdata);
+ dev_set_name(hdev, HWMON_ID_FORMAT, id);
+ err = device_register(hdev);
if (err)
- goto free;
+ goto free_hwmon;
+
+ if (chip && chip->ops->is_visible && chip->ops->read &&
+ chip->info[0]->type == hwmon_chip &&
+ (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
+ const struct hwmon_channel_info **info = chip->info;
+
+ for (i = 1; info[i]; i++) {
+ if (info[i]->type != hwmon_temp)
+ continue;
+
+ for (j = 0; info[i]->config[j]; j++) {
+ if (!chip->ops->is_visible(drvdata, hwmon_temp,
+ hwmon_temp_input, j))
+ continue;
+ if (info[i]->config[j] & HWMON_T_INPUT)
+ hwmon_thermal_add_sensor(dev, hwdev, j);
+ }
+ }
+ }
- return &hwdev->dev;
+ return hdev;
-free:
+free_hwmon:
kfree(hwdev);
ida_remove:
ida_simple_remove(&hwmon_ida, id);
return ERR_PTR(err);
}
+
+/**
+ * hwmon_device_register_with_groups - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @groups: List of attribute groups to create
+ *
+ * hwmon_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *
+hwmon_device_register_with_groups(struct device *dev, const char *name,
+ void *drvdata,
+ const struct attribute_group **groups)
+{
+ return __hwmon_device_register(dev, name, drvdata, NULL, groups);
+}
EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
/**
+ * hwmon_device_register_with_info - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @info: Pointer to hwmon chip information
+ * @groups - pointer to list of driver specific attribute groups
+ *
+ * hwmon_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *
+hwmon_device_register_with_info(struct device *dev, const char *name,
+ void *drvdata,
+ const struct hwmon_chip_info *chip,
+ const struct attribute_group **groups)
+{
+ if (chip && (!chip->ops || !chip->info))
+ return ERR_PTR(-EINVAL);
+
+ return __hwmon_device_register(dev, name, drvdata, chip, groups);
+}
+EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
+
+/**
* hwmon_device_register - register w/ hwmon
* @dev: the device to register
*
@@ -213,6 +591,47 @@ error:
}
EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
+/**
+ * devm_hwmon_device_register_with_info - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @info: Pointer to hwmon chip information
+ * @groups - pointer to list of driver specific attribute groups
+ *
+ * Returns the pointer to the new device. The new device is automatically
+ * unregistered with the parent device.
+ */
+struct device *
+devm_hwmon_device_register_with_info(struct device *dev, const char *name,
+ void *drvdata,
+ const struct hwmon_chip_info *chip,
+ const struct attribute_group **groups)
+{
+ struct device **ptr, *hwdev;
+
+ if (!dev)
+ return ERR_PTR(-EINVAL);
+
+ ptr = devres_alloc(devm_hwmon_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
+ groups);
+ if (IS_ERR(hwdev))
+ goto error;
+
+ *ptr = hwdev;
+ devres_add(dev, ptr);
+ return hwdev;
+
+error:
+ devres_free(ptr);
+ return hwdev;
+}
+EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_info);
+
static int devm_hwmon_match(struct device *dev, void *res, void *data)
{
struct device **hwdev = res;
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 09354f6c1d63..99250ad092fd 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -14,9 +14,121 @@
#ifndef _HWMON_H_
#define _HWMON_H_
+#include <linux/bitops.h>
+
struct device;
struct attribute_group;
+enum hwmon_sensor_types {
+ hwmon_chip,
+ hwmon_temp,
+ hwmon_in,
+ hwmon_curr,
+ hwmon_power,
+ hwmon_energy,
+};
+
+enum hwmon_chip_attributes {
+ hwmon_chip_temp_reset_history,
+ hwmon_chip_register_tz,
+ hwmon_chip_update_interval,
+ hwmon_chip_alarms,
+};
+
+#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
+#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
+#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
+#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
+#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
+
+enum hwmon_temp_attributes {
+ hwmon_temp_input = 0,
+ hwmon_temp_type,
+ hwmon_temp_lcrit,
+ hwmon_temp_lcrit_hyst,
+ hwmon_temp_min,
+ hwmon_temp_min_hyst,
+ hwmon_temp_max,
+ hwmon_temp_max_hyst,
+ hwmon_temp_crit,
+ hwmon_temp_crit_hyst,
+ hwmon_temp_emergency,
+ hwmon_temp_emergency_hyst,
+ hwmon_temp_alarm,
+ hwmon_temp_lcrit_alarm,
+ hwmon_temp_min_alarm,
+ hwmon_temp_max_alarm,
+ hwmon_temp_crit_alarm,
+ hwmon_temp_emergency_alarm,
+ hwmon_temp_fault,
+ hwmon_temp_offset,
+ hwmon_temp_label,
+ hwmon_temp_lowest,
+ hwmon_temp_highest,
+ hwmon_temp_reset_history,
+};
+
+#define HWMON_T_INPUT BIT(hwmon_temp_input)
+#define HWMON_T_TYPE BIT(hwmon_temp_type)
+#define HWMON_T_LCRIT BIT(hwmon_temp_lcrit)
+#define HWMON_T_LCRIT_HYST BIT(hwmon_temp_lcrit_hyst)
+#define HWMON_T_MIN BIT(hwmon_temp_min)
+#define HWMON_T_MIN_HYST BIT(hwmon_temp_min_hyst)
+#define HWMON_T_MAX BIT(hwmon_temp_max)
+#define HWMON_T_MAX_HYST BIT(hwmon_temp_max_hyst)
+#define HWMON_T_CRIT BIT(hwmon_temp_crit)
+#define HWMON_T_CRIT_HYST BIT(hwmon_temp_crit_hyst)
+#define HWMON_T_EMERGENCY BIT(hwmon_temp_emergency)
+#define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
+#define HWMON_T_MIN_ALARM BIT(hwmon_temp_min_alarm)
+#define HWMON_T_MAX_ALARM BIT(hwmon_temp_max_alarm)
+#define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
+#define HWMON_T_EMERGENCY_ALARM BIT(hwmon_temp_emergency_alarm)
+#define HWMON_T_FAULT BIT(hwmon_temp_fault)
+#define HWMON_T_OFFSET BIT(hwmon_temp_offset)
+#define HWMON_T_LABEL BIT(hwmon_temp_label)
+#define HWMON_T_LOWEST BIT(hwmon_temp_lowest)
+#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
+#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
+
+/**
+ * struct hwmon_ops - hwmon device operations
+ * @is_visible: Callback to return attribute visibility.
+ * Optional.
+ * @read: Read callback. Optional.
+ * @write: Write callback. Optional.
+ */
+struct hwmon_ops {
+ umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
+ u32 attr, int);
+ int (*read)(struct device *, enum hwmon_sensor_types type,
+ u32 attr, int, long *);
+ int (*write)(struct device *, enum hwmon_sensor_types type,
+ u32 attr, int, long);
+};
+
+/**
+ * Channel information
+ * @type: Channel type.
+ * @config: Pointer to NULL-terminated list of channel parameters.
+ * Use for per-channel attributes.
+ */
+struct hwmon_channel_info {
+ enum hwmon_sensor_types type;
+ const u32 *config;
+};
+
+/**
+ * Chip configuration
+ * @ops: Pointer to hwmon operations.
+ * @info: Null-terminated list of channel information.
+ *
+ */
+struct hwmon_chip_info {
+ const struct hwmon_ops *ops;
+ const struct hwmon_channel_info **info;
+};
+
struct device *hwmon_device_register(struct device *dev);
struct device *
hwmon_device_register_with_groups(struct device *dev, const char *name,
@@ -26,6 +138,16 @@ struct device *
devm_hwmon_device_register_with_groups(struct device *dev, const char *name,
void *drvdata,
const struct attribute_group **groups);
+struct device *
+hwmon_device_register_with_info(struct device *dev,
+ const char *name, void *drvdata,
+ const struct hwmon_chip_info *info,
+ const struct attribute_group **groups);
+struct device *
+devm_hwmon_device_register_with_info(struct device *dev,
+ const char *name, void *drvdata,
+ const struct hwmon_chip_info *info,
+ const struct attribute_group **groups);
void hwmon_device_unregister(struct device *dev);
void devm_hwmon_device_unregister(struct device *dev);
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 1/7] hwmon: (core) New hwmon registration API
@ 2016-06-26 3:26 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Guenter Roeck
Up to now, each hwmon driver has to implement its own sysfs attributes.
This requires a lot of template code, and distracts from the driver's core
function to read and write chip registers.
To be able to reduce driver complexity, move sensor attribute handling
and thermal zone registration into hwmon core. By using the new API,
driver size is typically reduced by 20-50% depending on driver complexity
and the number of sysfs attributes supported.
With this patch, the new API only supports thermal sensors. Support for
other sensor types will be added with subsequent patches.
Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
drivers/hwmon/hwmon.c | 485 ++++++++++++++++++++++++++++++++++++++++++++++----
include/linux/hwmon.h | 122 +++++++++++++
2 files changed, 574 insertions(+), 33 deletions(-)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index a26c385a435b..9530644ae297 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -12,17 +12,16 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/module.h>
+#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/err.h>
-#include <linux/slab.h>
-#include <linux/kdev_t.h>
-#include <linux/idr.h>
#include <linux/hwmon.h>
-#include <linux/gfp.h>
-#include <linux/spinlock.h>
+#include <linux/idr.h>
+#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/slab.h>
#include <linux/string.h>
+#include <linux/thermal.h>
#define HWMON_ID_PREFIX "hwmon"
#define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
@@ -30,9 +29,33 @@
struct hwmon_device {
const char *name;
struct device dev;
+ const struct hwmon_chip_info *chip;
+
+ struct attribute_group group;
+ const struct attribute_group **groups;
};
#define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
+struct hwmon_device_attribute {
+ struct device_attribute dev_attr;
+ const struct hwmon_ops *ops;
+ enum hwmon_sensor_types type;
+ u32 attr;
+ int index;
+};
+#define to_hwmon_attr(d) \
+ container_of(d, struct hwmon_device_attribute, dev_attr)
+
+/*
+ * Thermal zone information
+ * In addition to the reference to the hwmon device,
+ * also provides the sensor index.
+ */
+struct hwmon_thermal_data {
+ struct hwmon_device *hwdev; /* Reference to hwmon device */
+ int index; /* sensor index */
+};
+
static ssize_t
show_name(struct device *dev, struct device_attribute *attr, char *buf)
{
@@ -80,25 +103,280 @@ static struct class hwmon_class = {
static DEFINE_IDA(hwmon_ida);
-/**
- * hwmon_device_register_with_groups - register w/ hwmon
- * @dev: the parent device
- * @name: hwmon name attribute
- * @drvdata: driver data to attach to created device
- * @groups: List of attribute groups to create
- *
- * hwmon_device_unregister() must be called when the device is no
- * longer needed.
- *
- * Returns the pointer to the new device.
- */
-struct device *
-hwmon_device_register_with_groups(struct device *dev, const char *name,
- void *drvdata,
- const struct attribute_group **groups)
+/* Thermal zone handling */
+
+static int hwmon_thermal_get_temp(void *data, int *temp)
+{
+ struct hwmon_thermal_data *tdata = data;
+ struct hwmon_device *hwdev = tdata->hwdev;
+ int ret;
+ long t;
+
+ ret = hwdev->chip->ops->read(&hwdev->dev, hwmon_temp, hwmon_temp_input,
+ tdata->index, &t);
+ if (ret < 0)
+ return ret;
+
+ *temp = t;
+
+ return 0;
+}
+
+static struct thermal_zone_of_device_ops hwmon_thermal_ops = {
+ .get_temp = hwmon_thermal_get_temp,
+};
+
+static int hwmon_thermal_add_sensor(struct device *dev,
+ struct hwmon_device *hwdev,
+ int index)
+{
+ struct hwmon_thermal_data *tdata;
+
+ tdata = devm_kzalloc(dev, sizeof(*tdata), GFP_KERNEL);
+ if (!tdata)
+ return -ENOMEM;
+
+ tdata->hwdev = hwdev;
+ tdata->index = index;
+
+ devm_thermal_zone_of_sensor_register(&hwdev->dev, index, tdata,
+ &hwmon_thermal_ops);
+
+ return 0;
+}
+
+/* sysfs attribute management */
+
+static ssize_t hwmon_attr_show(struct device *dev,
+ struct device_attribute *devattr, char *buf)
+{
+ struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+ long val;
+ int ret;
+
+ ret = hattr->ops->read(dev, hattr->type, hattr->attr, hattr->index,
+ &val);
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%ld\n", val);
+}
+
+static ssize_t hwmon_attr_store(struct device *dev,
+ struct device_attribute *devattr,
+ const char *buf, size_t count)
+{
+ struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
+ long val;
+ int ret;
+
+ ret = kstrtol(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ ret = hattr->ops->write(dev, hattr->type, hattr->attr, hattr->index,
+ val);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static int hwmon_attr_base(enum hwmon_sensor_types type)
+{
+ if (type == hwmon_in)
+ return 0;
+ return 1;
+}
+
+static struct attribute *hwmon_genattr(struct device *dev,
+ const void *drvdata,
+ enum hwmon_sensor_types type,
+ u32 attr,
+ int index,
+ const char *template,
+ const struct hwmon_ops *ops)
+{
+ struct hwmon_device_attribute *hattr;
+ struct device_attribute *dattr;
+ struct attribute *a;
+ umode_t mode;
+ char *name;
+
+ if (!template)
+ return ERR_PTR(-EINVAL);
+
+ mode = ops->is_visible(drvdata, type, attr, index);
+ if (!mode)
+ return ERR_PTR(-ENOENT);
+
+ if ((mode & S_IRUGO) && !ops->read)
+ return ERR_PTR(-EINVAL);
+ if ((mode & S_IWUGO) && !ops->write)
+ return ERR_PTR(-EINVAL);
+
+ if (type == hwmon_chip) {
+ name = (char *)template;
+ } else {
+ name = devm_kzalloc(dev, strlen(template) + 16, GFP_KERNEL);
+ if (!name)
+ return ERR_PTR(-ENOMEM);
+ scnprintf(name, strlen(template) + 16, template,
+ index + hwmon_attr_base(type));
+ }
+
+ hattr = devm_kzalloc(dev, sizeof(*hattr), GFP_KERNEL);
+ if (!hattr)
+ return ERR_PTR(-ENOMEM);
+
+ hattr->type = type;
+ hattr->attr = attr;
+ hattr->index = index;
+ hattr->ops = ops;
+
+ dattr = &hattr->dev_attr;
+ if (mode & S_IRUGO)
+ dattr->show = hwmon_attr_show;
+ if (mode & S_IWUGO)
+ dattr->store = hwmon_attr_store;
+
+ a = &dattr->attr;
+ sysfs_attr_init(a);
+ a->name = name;
+ a->mode = mode;
+
+ return a;
+}
+
+static const char * const hwmon_chip_attr_templates[] = {
+ [hwmon_chip_temp_reset_history] = "temp_reset_history",
+ [hwmon_chip_update_interval] = "update_interval",
+ [hwmon_chip_alarms] = "alarms",
+};
+
+static const char * const hwmon_temp_attr_templates[] = {
+ [hwmon_temp_input] = "temp%d_input",
+ [hwmon_temp_type] = "temp%d_type",
+ [hwmon_temp_lcrit] = "temp%d_lcrit",
+ [hwmon_temp_lcrit_hyst] = "temp%d_lcrit_hyst",
+ [hwmon_temp_min] = "temp%d_min",
+ [hwmon_temp_min_hyst] = "temp%d_min_hyst",
+ [hwmon_temp_max] = "temp%d_max",
+ [hwmon_temp_max_hyst] = "temp%d_max_hyst",
+ [hwmon_temp_crit] = "temp%d_crit",
+ [hwmon_temp_crit_hyst] = "temp%d_crit_hyst",
+ [hwmon_temp_emergency] = "temp%d_emergency",
+ [hwmon_temp_emergency_hyst] = "temp%d_emergency_hyst",
+ [hwmon_temp_alarm] = "temp%d_alarm",
+ [hwmon_temp_lcrit_alarm] = "temp%d_lcrit_alarm",
+ [hwmon_temp_min_alarm] = "temp%d_min_alarm",
+ [hwmon_temp_max_alarm] = "temp%d_max_alarm",
+ [hwmon_temp_crit_alarm] = "temp%d_crit_alarm",
+ [hwmon_temp_emergency_alarm] = "temp%d_emergency_alarm",
+ [hwmon_temp_fault] = "temp%d_fault",
+ [hwmon_temp_offset] = "temp%d_offset",
+ [hwmon_temp_label] = "temp%d_label",
+ [hwmon_temp_lowest] = "temp%d_lowest",
+ [hwmon_temp_highest] = "temp%d_highest",
+ [hwmon_temp_reset_history] = "temp%d_reset_history",
+};
+
+static const char * const *__templates[] = {
+ [hwmon_chip] = hwmon_chip_attr_templates,
+ [hwmon_temp] = hwmon_temp_attr_templates,
+};
+
+static const int __templates_size[] = {
+ [hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
+ [hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
+};
+
+static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
+{
+ int i, n;
+
+ for (i = n = 0; info->config[i]; i++)
+ n += hweight32(info->config[i]);
+
+ return n;
+}
+
+static int hwmon_genattrs(struct device *dev,
+ const void *drvdata,
+ struct attribute **attrs,
+ const struct hwmon_ops *ops,
+ const struct hwmon_channel_info *info)
+{
+ const char * const *templates;
+ int template_size;
+ int i, aindex = 0;
+
+ if (info->type >= ARRAY_SIZE(__templates))
+ return -EINVAL;
+
+ templates = __templates[info->type];
+ template_size = __templates_size[info->type];
+
+ for (i = 0; info->config[i]; i++) {
+ u32 attr_mask = info->config[i];
+ u32 attr;
+
+ while (attr_mask) {
+ struct attribute *a;
+
+ attr = __ffs(attr_mask);
+ attr_mask &= ~BIT(attr);
+ if (attr >= template_size)
+ return -EINVAL;
+ a = hwmon_genattr(dev, drvdata, info->type, attr, i,
+ templates[attr], ops);
+ if (IS_ERR(a)) {
+ if (PTR_ERR(a) != -ENOENT)
+ return PTR_ERR(a);
+ continue;
+ }
+ attrs[aindex++] = a;
+ }
+ }
+ return aindex;
+}
+
+static struct attribute **
+__hwmon_create_attrs(struct device *dev, const void *drvdata,
+ const struct hwmon_chip_info *chip)
+{
+ int ret, i, aindex = 0, nattrs = 0;
+ struct attribute **attrs;
+
+ for (i = 0; chip->info[i]; i++)
+ nattrs += hwmon_num_channel_attrs(chip->info[i]);
+
+ if (nattrs == 0)
+ return ERR_PTR(-EINVAL);
+
+ attrs = devm_kcalloc(dev, nattrs + 1, sizeof(*attrs), GFP_KERNEL);
+ if (attrs == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ for (i = 0; chip->info[i]; i++) {
+ ret = hwmon_genattrs(dev, drvdata, &attrs[aindex], chip->ops,
+ chip->info[i]);
+ if (ret < 0)
+ return ERR_PTR(ret);
+ aindex += ret;
+ }
+
+ return attrs;
+}
+
+static struct device *
+__hwmon_device_register(struct device *dev, const char *name, void *drvdata,
+ const struct hwmon_chip_info *chip,
+ const struct attribute_group **groups)
{
struct hwmon_device *hwdev;
- int err, id;
+ struct device *hdev;
+ int i, j, err, id;
/* Do not accept invalid characters in hwmon name attribute */
if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
@@ -114,28 +392,128 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
goto ida_remove;
}
+ hdev = &hwdev->dev;
+
+ if (chip && chip->ops->is_visible) {
+ struct attribute **attrs;
+ int ngroups = 2;
+
+ if (groups)
+ for (i = 0; groups[i]; i++)
+ ngroups++;
+
+ hwdev->groups = devm_kcalloc(dev, ngroups, sizeof(*groups),
+ GFP_KERNEL);
+ if (hwdev->groups == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ attrs = __hwmon_create_attrs(dev, drvdata, chip);
+ if (IS_ERR(attrs)) {
+ err = PTR_ERR(attrs);
+ goto free_hwmon;
+ }
+
+ hwdev->group.attrs = attrs;
+ ngroups = 0;
+ hwdev->groups[ngroups++] = &hwdev->group;
+
+ if (groups) {
+ for (i = 0; groups[i]; i++)
+ hwdev->groups[ngroups++] = groups[i];
+ }
+
+ hdev->groups = hwdev->groups;
+ } else {
+ hdev->groups = groups;
+ }
+
hwdev->name = name;
- hwdev->dev.class = &hwmon_class;
- hwdev->dev.parent = dev;
- hwdev->dev.groups = groups;
- hwdev->dev.of_node = dev ? dev->of_node : NULL;
- dev_set_drvdata(&hwdev->dev, drvdata);
- dev_set_name(&hwdev->dev, HWMON_ID_FORMAT, id);
- err = device_register(&hwdev->dev);
+ hdev->class = &hwmon_class;
+ hdev->parent = dev;
+ hdev->of_node = dev ? dev->of_node : NULL;
+ hwdev->chip = chip;
+ dev_set_drvdata(hdev, drvdata);
+ dev_set_name(hdev, HWMON_ID_FORMAT, id);
+ err = device_register(hdev);
if (err)
- goto free;
+ goto free_hwmon;
+
+ if (chip && chip->ops->is_visible && chip->ops->read &&
+ chip->info[0]->type == hwmon_chip &&
+ (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
+ const struct hwmon_channel_info **info = chip->info;
+
+ for (i = 1; info[i]; i++) {
+ if (info[i]->type != hwmon_temp)
+ continue;
+
+ for (j = 0; info[i]->config[j]; j++) {
+ if (!chip->ops->is_visible(drvdata, hwmon_temp,
+ hwmon_temp_input, j))
+ continue;
+ if (info[i]->config[j] & HWMON_T_INPUT)
+ hwmon_thermal_add_sensor(dev, hwdev, j);
+ }
+ }
+ }
- return &hwdev->dev;
+ return hdev;
-free:
+free_hwmon:
kfree(hwdev);
ida_remove:
ida_simple_remove(&hwmon_ida, id);
return ERR_PTR(err);
}
+
+/**
+ * hwmon_device_register_with_groups - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @groups: List of attribute groups to create
+ *
+ * hwmon_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *
+hwmon_device_register_with_groups(struct device *dev, const char *name,
+ void *drvdata,
+ const struct attribute_group **groups)
+{
+ return __hwmon_device_register(dev, name, drvdata, NULL, groups);
+}
EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
/**
+ * hwmon_device_register_with_info - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @info: Pointer to hwmon chip information
+ * @groups - pointer to list of driver specific attribute groups
+ *
+ * hwmon_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *
+hwmon_device_register_with_info(struct device *dev, const char *name,
+ void *drvdata,
+ const struct hwmon_chip_info *chip,
+ const struct attribute_group **groups)
+{
+ if (chip && (!chip->ops || !chip->info))
+ return ERR_PTR(-EINVAL);
+
+ return __hwmon_device_register(dev, name, drvdata, chip, groups);
+}
+EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
+
+/**
* hwmon_device_register - register w/ hwmon
* @dev: the device to register
*
@@ -213,6 +591,47 @@ error:
}
EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
+/**
+ * devm_hwmon_device_register_with_info - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @info: Pointer to hwmon chip information
+ * @groups - pointer to list of driver specific attribute groups
+ *
+ * Returns the pointer to the new device. The new device is automatically
+ * unregistered with the parent device.
+ */
+struct device *
+devm_hwmon_device_register_with_info(struct device *dev, const char *name,
+ void *drvdata,
+ const struct hwmon_chip_info *chip,
+ const struct attribute_group **groups)
+{
+ struct device **ptr, *hwdev;
+
+ if (!dev)
+ return ERR_PTR(-EINVAL);
+
+ ptr = devres_alloc(devm_hwmon_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
+ groups);
+ if (IS_ERR(hwdev))
+ goto error;
+
+ *ptr = hwdev;
+ devres_add(dev, ptr);
+ return hwdev;
+
+error:
+ devres_free(ptr);
+ return hwdev;
+}
+EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_info);
+
static int devm_hwmon_match(struct device *dev, void *res, void *data)
{
struct device **hwdev = res;
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 09354f6c1d63..99250ad092fd 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -14,9 +14,121 @@
#ifndef _HWMON_H_
#define _HWMON_H_
+#include <linux/bitops.h>
+
struct device;
struct attribute_group;
+enum hwmon_sensor_types {
+ hwmon_chip,
+ hwmon_temp,
+ hwmon_in,
+ hwmon_curr,
+ hwmon_power,
+ hwmon_energy,
+};
+
+enum hwmon_chip_attributes {
+ hwmon_chip_temp_reset_history,
+ hwmon_chip_register_tz,
+ hwmon_chip_update_interval,
+ hwmon_chip_alarms,
+};
+
+#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
+#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
+#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
+#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
+#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
+
+enum hwmon_temp_attributes {
+ hwmon_temp_input = 0,
+ hwmon_temp_type,
+ hwmon_temp_lcrit,
+ hwmon_temp_lcrit_hyst,
+ hwmon_temp_min,
+ hwmon_temp_min_hyst,
+ hwmon_temp_max,
+ hwmon_temp_max_hyst,
+ hwmon_temp_crit,
+ hwmon_temp_crit_hyst,
+ hwmon_temp_emergency,
+ hwmon_temp_emergency_hyst,
+ hwmon_temp_alarm,
+ hwmon_temp_lcrit_alarm,
+ hwmon_temp_min_alarm,
+ hwmon_temp_max_alarm,
+ hwmon_temp_crit_alarm,
+ hwmon_temp_emergency_alarm,
+ hwmon_temp_fault,
+ hwmon_temp_offset,
+ hwmon_temp_label,
+ hwmon_temp_lowest,
+ hwmon_temp_highest,
+ hwmon_temp_reset_history,
+};
+
+#define HWMON_T_INPUT BIT(hwmon_temp_input)
+#define HWMON_T_TYPE BIT(hwmon_temp_type)
+#define HWMON_T_LCRIT BIT(hwmon_temp_lcrit)
+#define HWMON_T_LCRIT_HYST BIT(hwmon_temp_lcrit_hyst)
+#define HWMON_T_MIN BIT(hwmon_temp_min)
+#define HWMON_T_MIN_HYST BIT(hwmon_temp_min_hyst)
+#define HWMON_T_MAX BIT(hwmon_temp_max)
+#define HWMON_T_MAX_HYST BIT(hwmon_temp_max_hyst)
+#define HWMON_T_CRIT BIT(hwmon_temp_crit)
+#define HWMON_T_CRIT_HYST BIT(hwmon_temp_crit_hyst)
+#define HWMON_T_EMERGENCY BIT(hwmon_temp_emergency)
+#define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
+#define HWMON_T_MIN_ALARM BIT(hwmon_temp_min_alarm)
+#define HWMON_T_MAX_ALARM BIT(hwmon_temp_max_alarm)
+#define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
+#define HWMON_T_EMERGENCY_ALARM BIT(hwmon_temp_emergency_alarm)
+#define HWMON_T_FAULT BIT(hwmon_temp_fault)
+#define HWMON_T_OFFSET BIT(hwmon_temp_offset)
+#define HWMON_T_LABEL BIT(hwmon_temp_label)
+#define HWMON_T_LOWEST BIT(hwmon_temp_lowest)
+#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
+#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
+
+/**
+ * struct hwmon_ops - hwmon device operations
+ * @is_visible: Callback to return attribute visibility.
+ * Optional.
+ * @read: Read callback. Optional.
+ * @write: Write callback. Optional.
+ */
+struct hwmon_ops {
+ umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
+ u32 attr, int);
+ int (*read)(struct device *, enum hwmon_sensor_types type,
+ u32 attr, int, long *);
+ int (*write)(struct device *, enum hwmon_sensor_types type,
+ u32 attr, int, long);
+};
+
+/**
+ * Channel information
+ * @type: Channel type.
+ * @config: Pointer to NULL-terminated list of channel parameters.
+ * Use for per-channel attributes.
+ */
+struct hwmon_channel_info {
+ enum hwmon_sensor_types type;
+ const u32 *config;
+};
+
+/**
+ * Chip configuration
+ * @ops: Pointer to hwmon operations.
+ * @info: Null-terminated list of channel information.
+ *
+ */
+struct hwmon_chip_info {
+ const struct hwmon_ops *ops;
+ const struct hwmon_channel_info **info;
+};
+
struct device *hwmon_device_register(struct device *dev);
struct device *
hwmon_device_register_with_groups(struct device *dev, const char *name,
@@ -26,6 +138,16 @@ struct device *
devm_hwmon_device_register_with_groups(struct device *dev, const char *name,
void *drvdata,
const struct attribute_group **groups);
+struct device *
+hwmon_device_register_with_info(struct device *dev,
+ const char *name, void *drvdata,
+ const struct hwmon_chip_info *info,
+ const struct attribute_group **groups);
+struct device *
+devm_hwmon_device_register_with_info(struct device *dev,
+ const char *name, void *drvdata,
+ const struct hwmon_chip_info *info,
+ const struct attribute_group **groups);
void hwmon_device_unregister(struct device *dev);
void devm_hwmon_device_unregister(struct device *dev);
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] hwmon: (core) New hwmon registration API
2016-06-26 3:26 ` Guenter Roeck
(?)
@ 2016-07-10 15:51 ` Jonathan Cameron
2016-07-11 1:31 ` Guenter Roeck
-1 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2016-07-10 15:51 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm, linux-iio, linux-hwmon,
linux-kernel
On 26/06/16 04:26, Guenter Roeck wrote:
> Up to now, each hwmon driver has to implement its own sysfs attributes.
> This requires a lot of template code, and distracts from the driver's core
> function to read and write chip registers.
>
> To be able to reduce driver complexity, move sensor attribute handling
> and thermal zone registration into hwmon core. By using the new API,
> driver size is typically reduced by 20-50% depending on driver complexity
> and the number of sysfs attributes supported.
>
> With this patch, the new API only supports thermal sensors. Support for
> other sensor types will be added with subsequent patches.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Hi Guenter.
Sorry it took me so long to get to this....
Anyhow, mostly looks clean, effective and consise to me.
Various bits and pieces inline. There are a few bits I might (when time
allows) lift over to iio as they are nicer than what we have.
My one lesson learned from IIO is always be wary of space in bitmaps. We
haven't run out yet but are getting close. You may end up in a similar
state here a few years down the line.
Jonathan
> ---
> drivers/hwmon/hwmon.c | 485 ++++++++++++++++++++++++++++++++++++++++++++++----
> include/linux/hwmon.h | 122 +++++++++++++
> 2 files changed, 574 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index a26c385a435b..9530644ae297 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -12,17 +12,16 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> -#include <linux/module.h>
> +#include <linux/bitops.h>
> #include <linux/device.h>
> #include <linux/err.h>
> -#include <linux/slab.h>
> -#include <linux/kdev_t.h>
> -#include <linux/idr.h>
> #include <linux/hwmon.h>
> -#include <linux/gfp.h>
> -#include <linux/spinlock.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
Some unconnected changes in this. Arguably reordering is good and
all but should be in a precursor patch so it's obvious what has
been added or removed here.
> #include <linux/pci.h>
> +#include <linux/slab.h>
> #include <linux/string.h>
> +#include <linux/thermal.h>
>
> #define HWMON_ID_PREFIX "hwmon"
> #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
> @@ -30,9 +29,33 @@
> struct hwmon_device {
> const char *name;
> struct device dev;
> + const struct hwmon_chip_info *chip;
> +
> + struct attribute_group group;
> + const struct attribute_group **groups;
> };
> #define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
>
> +struct hwmon_device_attribute {
> + struct device_attribute dev_attr;
> + const struct hwmon_ops *ops;
> + enum hwmon_sensor_types type;
> + u32 attr;
> + int index;
> +};
> +#define to_hwmon_attr(d) \
> + container_of(d, struct hwmon_device_attribute, dev_attr)
> +
> +/*
> + * Thermal zone information
> + * In addition to the reference to the hwmon device,
> + * also provides the sensor index.
> + */
> +struct hwmon_thermal_data {
> + struct hwmon_device *hwdev; /* Reference to hwmon device */
> + int index; /* sensor index */
> +};
> +
> static ssize_t
> show_name(struct device *dev, struct device_attribute *attr, char *buf)
> {
> @@ -80,25 +103,280 @@ static struct class hwmon_class = {
>
> static DEFINE_IDA(hwmon_ida);
>
> -/**
> - * hwmon_device_register_with_groups - register w/ hwmon
> - * @dev: the parent device
> - * @name: hwmon name attribute
> - * @drvdata: driver data to attach to created device
> - * @groups: List of attribute groups to create
> - *
> - * hwmon_device_unregister() must be called when the device is no
> - * longer needed.
> - *
> - * Returns the pointer to the new device.
> - */
> -struct device *
> -hwmon_device_register_with_groups(struct device *dev, const char *name,
> - void *drvdata,
> - const struct attribute_group **groups)
> +/* Thermal zone handling */
> +
> +static int hwmon_thermal_get_temp(void *data, int *temp)
> +{
> + struct hwmon_thermal_data *tdata = data;
> + struct hwmon_device *hwdev = tdata->hwdev;
> + int ret;
> + long t;
> +
> + ret = hwdev->chip->ops->read(&hwdev->dev, hwmon_temp, hwmon_temp_input,
> + tdata->index, &t);
> + if (ret < 0)
> + return ret;
> +
> + *temp = t;
> +
> + return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops hwmon_thermal_ops = {
> + .get_temp = hwmon_thermal_get_temp,
> +};
> +
> +static int hwmon_thermal_add_sensor(struct device *dev,
> + struct hwmon_device *hwdev,
> + int index)
> +{
> + struct hwmon_thermal_data *tdata;
> +
> + tdata = devm_kzalloc(dev, sizeof(*tdata), GFP_KERNEL);
> + if (!tdata)
> + return -ENOMEM;
> +
> + tdata->hwdev = hwdev;
> + tdata->index = index;
> +
> + devm_thermal_zone_of_sensor_register(&hwdev->dev, index, tdata,
> + &hwmon_thermal_ops);
> +
> + return 0;
> +}
> +
> +/* sysfs attribute management */
> +
> +static ssize_t hwmon_attr_show(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> + long val;
> + int ret;
> +
> + ret = hattr->ops->read(dev, hattr->type, hattr->attr, hattr->index,
> + &val);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%ld\n", val);
> +}
> +
> +static ssize_t hwmon_attr_store(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
> + long val;
> + int ret;
> +
> + ret = kstrtol(buf, 10, &val);
> + if (ret < 0)
> + return ret;
> +
> + ret = hattr->ops->write(dev, hattr->type, hattr->attr, hattr->index,
> + val);
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> +
> +static int hwmon_attr_base(enum hwmon_sensor_types type)
> +{
> + if (type == hwmon_in)
> + return 0;
> + return 1;
> +}
> +
> +static struct attribute *hwmon_genattr(struct device *dev,
> + const void *drvdata,
> + enum hwmon_sensor_types type,
> + u32 attr,
> + int index,
> + const char *template,
> + const struct hwmon_ops *ops)
> +{
> + struct hwmon_device_attribute *hattr;
> + struct device_attribute *dattr;
> + struct attribute *a;
> + umode_t mode;
> + char *name;
> +
> + if (!template)
> + return ERR_PTR(-EINVAL);
> +
> + mode = ops->is_visible(drvdata, type, attr, index);
> + if (!mode)
> + return ERR_PTR(-ENOENT);
> +
I've been wondering about doing something similar to this in IIO for a while
as our attributes are far to often rw when they should only be one or the
other. This is much more elegant. We could get away with it (nasty as it is)
on the basis of no previous ABI to match where as you have to have this...
Can see this is_visible callback might get a little messy in some cases.
Actually, is is_visible a good name? I'd think that would control whether
we can see the attribute at all. Perhaps something like
get_filemode would be a better callback naming?
> + if ((mode & S_IRUGO) && !ops->read)
> + return ERR_PTR(-EINVAL);
> + if ((mode & S_IWUGO) && !ops->write)
> + return ERR_PTR(-EINVAL);
> +
> + if (type == hwmon_chip) {
> + name = (char *)template;
> + } else {
> + name = devm_kzalloc(dev, strlen(template) + 16, GFP_KERNEL);
> + if (!name)
> + return ERR_PTR(-ENOMEM);
> + scnprintf(name, strlen(template) + 16, template,
> + index + hwmon_attr_base(type));
> + }
> +
> + hattr = devm_kzalloc(dev, sizeof(*hattr), GFP_KERNEL);
> + if (!hattr)
> + return ERR_PTR(-ENOMEM);
> +
> + hattr->type = type;
> + hattr->attr = attr;
> + hattr->index = index;
> + hattr->ops = ops;
> +
> + dattr = &hattr->dev_attr;
> + if (mode & S_IRUGO)
> + dattr->show = hwmon_attr_show;
> + if (mode & S_IWUGO)
> + dattr->store = hwmon_attr_store;
Technically I think there is no need to worry about setting the store / show
when they aren't needed. Maybe, not worth the really small savings though...
> +
> + a = &dattr->attr;
> + sysfs_attr_init(a);
> + a->name = name;
> + a->mode = mode;
> +
> + return a;
> +}
> +
> +static const char * const hwmon_chip_attr_templates[] = {
> + [hwmon_chip_temp_reset_history] = "temp_reset_history",
> + [hwmon_chip_update_interval] = "update_interval",
> + [hwmon_chip_alarms] = "alarms",
> +};
> +
> +static const char * const hwmon_temp_attr_templates[] = {
> + [hwmon_temp_input] = "temp%d_input",
> + [hwmon_temp_type] = "temp%d_type",
> + [hwmon_temp_lcrit] = "temp%d_lcrit",
> + [hwmon_temp_lcrit_hyst] = "temp%d_lcrit_hyst",
> + [hwmon_temp_min] = "temp%d_min",
> + [hwmon_temp_min_hyst] = "temp%d_min_hyst",
> + [hwmon_temp_max] = "temp%d_max",
> + [hwmon_temp_max_hyst] = "temp%d_max_hyst",
> + [hwmon_temp_crit] = "temp%d_crit",
> + [hwmon_temp_crit_hyst] = "temp%d_crit_hyst",
> + [hwmon_temp_emergency] = "temp%d_emergency",
> + [hwmon_temp_emergency_hyst] = "temp%d_emergency_hyst",
> + [hwmon_temp_alarm] = "temp%d_alarm",
> + [hwmon_temp_lcrit_alarm] = "temp%d_lcrit_alarm",
> + [hwmon_temp_min_alarm] = "temp%d_min_alarm",
> + [hwmon_temp_max_alarm] = "temp%d_max_alarm",
> + [hwmon_temp_crit_alarm] = "temp%d_crit_alarm",
> + [hwmon_temp_emergency_alarm] = "temp%d_emergency_alarm",
> + [hwmon_temp_fault] = "temp%d_fault",
> + [hwmon_temp_offset] = "temp%d_offset",
> + [hwmon_temp_label] = "temp%d_label",
> + [hwmon_temp_lowest] = "temp%d_lowest",
> + [hwmon_temp_highest] = "temp%d_highest",
> + [hwmon_temp_reset_history] = "temp%d_reset_history",
> +};
> +
> +static const char * const *__templates[] = {
> + [hwmon_chip] = hwmon_chip_attr_templates,
> + [hwmon_temp] = hwmon_temp_attr_templates,
> +};
> +
> +static const int __templates_size[] = {
> + [hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
> + [hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
> +};
> +
> +static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
> +{
> + int i, n;
> +
> + for (i = n = 0; info->config[i]; i++)
> + n += hweight32(info->config[i]);
> +
> + return n;
> +}
> +
> +static int hwmon_genattrs(struct device *dev,
> + const void *drvdata,
> + struct attribute **attrs,
> + const struct hwmon_ops *ops,
> + const struct hwmon_channel_info *info)
> +{
> + const char * const *templates;
> + int template_size;
> + int i, aindex = 0;
> +
> + if (info->type >= ARRAY_SIZE(__templates))
> + return -EINVAL;
> +
> + templates = __templates[info->type];
> + template_size = __templates_size[info->type];
> +
> + for (i = 0; info->config[i]; i++) {
> + u32 attr_mask = info->config[i];
> + u32 attr;
> +
> + while (attr_mask) {
> + struct attribute *a;
> +
> + attr = __ffs(attr_mask);
> + attr_mask &= ~BIT(attr);
loop instead using the for_each_bit_set? Tiny bit cleaner perhaps...
> + if (attr >= template_size)
> + return -EINVAL;
> + a = hwmon_genattr(dev, drvdata, info->type, attr, i,
> + templates[attr], ops);
> + if (IS_ERR(a)) {
> + if (PTR_ERR(a) != -ENOENT)
> + return PTR_ERR(a);
> + continue;
> + }
> + attrs[aindex++] = a;
> + }
> + }
> + return aindex;
> +}
> +
> +static struct attribute **
> +__hwmon_create_attrs(struct device *dev, const void *drvdata,
> + const struct hwmon_chip_info *chip)
> +{
> + int ret, i, aindex = 0, nattrs = 0;
> + struct attribute **attrs;
> +
> + for (i = 0; chip->info[i]; i++)
> + nattrs += hwmon_num_channel_attrs(chip->info[i]);
> +
> + if (nattrs == 0)
> + return ERR_PTR(-EINVAL);
> +
> + attrs = devm_kcalloc(dev, nattrs + 1, sizeof(*attrs), GFP_KERNEL);
> + if (attrs == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + for (i = 0; chip->info[i]; i++) {
> + ret = hwmon_genattrs(dev, drvdata, &attrs[aindex], chip->ops,
> + chip->info[i]);
> + if (ret < 0)
> + return ERR_PTR(ret);
> + aindex += ret;
> + }
> +
> + return attrs;
> +}
> +
> +static struct device *
> +__hwmon_device_register(struct device *dev, const char *name, void *drvdata,
> + const struct hwmon_chip_info *chip,
> + const struct attribute_group **groups)
> {
> struct hwmon_device *hwdev;
> - int err, id;
> + struct device *hdev;
> + int i, j, err, id;
>
> /* Do not accept invalid characters in hwmon name attribute */
> if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
> @@ -114,28 +392,128 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
> goto ida_remove;
> }
>
> + hdev = &hwdev->dev;
> +
> + if (chip && chip->ops->is_visible) {
> + struct attribute **attrs;
> + int ngroups = 2;
> +
> + if (groups)
> + for (i = 0; groups[i]; i++)
> + ngroups++;
> +
> + hwdev->groups = devm_kcalloc(dev, ngroups, sizeof(*groups),
> + GFP_KERNEL);
> + if (hwdev->groups == NULL)
> + return ERR_PTR(-ENOMEM);
> +
> + attrs = __hwmon_create_attrs(dev, drvdata, chip);
> + if (IS_ERR(attrs)) {
> + err = PTR_ERR(attrs);
> + goto free_hwmon;
> + }
> +
> + hwdev->group.attrs = attrs;
> + ngroups = 0;
> + hwdev->groups[ngroups++] = &hwdev->group;
> +
> + if (groups) {
> + for (i = 0; groups[i]; i++)
> + hwdev->groups[ngroups++] = groups[i];
> + }
> +
> + hdev->groups = hwdev->groups;
> + } else {
> + hdev->groups = groups;
> + }
> +
> hwdev->name = name;
> - hwdev->dev.class = &hwmon_class;
> - hwdev->dev.parent = dev;
> - hwdev->dev.groups = groups;
> - hwdev->dev.of_node = dev ? dev->of_node : NULL;
> - dev_set_drvdata(&hwdev->dev, drvdata);
> - dev_set_name(&hwdev->dev, HWMON_ID_FORMAT, id);
> - err = device_register(&hwdev->dev);
> + hdev->class = &hwmon_class;
> + hdev->parent = dev;
> + hdev->of_node = dev ? dev->of_node : NULL;
> + hwdev->chip = chip;
> + dev_set_drvdata(hdev, drvdata);
> + dev_set_name(hdev, HWMON_ID_FORMAT, id);
> + err = device_register(hdev);
> if (err)
> - goto free;
> + goto free_hwmon;
> +
> + if (chip && chip->ops->is_visible && chip->ops->read &&
> + chip->info[0]->type == hwmon_chip &&
> + (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
> + const struct hwmon_channel_info **info = chip->info;
> +
> + for (i = 1; info[i]; i++) {
> + if (info[i]->type != hwmon_temp)
> + continue;
> +
> + for (j = 0; info[i]->config[j]; j++) {
> + if (!chip->ops->is_visible(drvdata, hwmon_temp,
> + hwmon_temp_input, j))
> + continue;
> + if (info[i]->config[j] & HWMON_T_INPUT)
> + hwmon_thermal_add_sensor(dev, hwdev, j);
> + }
> + }
> + }
>
> - return &hwdev->dev;
> + return hdev;
>
> -free:
> +free_hwmon:
> kfree(hwdev);
> ida_remove:
> ida_simple_remove(&hwmon_ida, id);
> return ERR_PTR(err);
> }
> +
> +/**
> + * hwmon_device_register_with_groups - register w/ hwmon
> + * @dev: the parent device
> + * @name: hwmon name attribute
> + * @drvdata: driver data to attach to created device
> + * @groups: List of attribute groups to create
> + *
> + * hwmon_device_unregister() must be called when the device is no
> + * longer needed.
> + *
> + * Returns the pointer to the new device.
> + */
> +struct device *
> +hwmon_device_register_with_groups(struct device *dev, const char *name,
> + void *drvdata,
> + const struct attribute_group **groups)
> +{
> + return __hwmon_device_register(dev, name, drvdata, NULL, groups);
> +}
> EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
>
> /**
> + * hwmon_device_register_with_info - register w/ hwmon
> + * @dev: the parent device
> + * @name: hwmon name attribute
> + * @drvdata: driver data to attach to created device
> + * @info: Pointer to hwmon chip information
> + * @groups - pointer to list of driver specific attribute groups
> + *
> + * hwmon_device_unregister() must be called when the device is no
> + * longer needed.
> + *
> + * Returns the pointer to the new device.
> + */
> +struct device *
> +hwmon_device_register_with_info(struct device *dev, const char *name,
> + void *drvdata,
> + const struct hwmon_chip_info *chip,
> + const struct attribute_group **groups)
> +{
> + if (chip && (!chip->ops || !chip->info))
> + return ERR_PTR(-EINVAL);
> +
> + return __hwmon_device_register(dev, name, drvdata, chip, groups);
> +}
> +EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
> +
> +/**
> * hwmon_device_register - register w/ hwmon
> * @dev: the device to register
> *
> @@ -213,6 +591,47 @@ error:
> }
> EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
>
> +/**
> + * devm_hwmon_device_register_with_info - register w/ hwmon
> + * @dev: the parent device
> + * @name: hwmon name attribute
> + * @drvdata: driver data to attach to created device
> + * @info: Pointer to hwmon chip information
> + * @groups - pointer to list of driver specific attribute groups
> + *
> + * Returns the pointer to the new device. The new device is automatically
> + * unregistered with the parent device.
> + */
> +struct device *
> +devm_hwmon_device_register_with_info(struct device *dev, const char *name,
> + void *drvdata,
> + const struct hwmon_chip_info *chip,
> + const struct attribute_group **groups)
> +{
> + struct device **ptr, *hwdev;
> +
> + if (!dev)
> + return ERR_PTR(-EINVAL);
> +
> + ptr = devres_alloc(devm_hwmon_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
> + groups);
> + if (IS_ERR(hwdev))
> + goto error;
> +
> + *ptr = hwdev;
> + devres_add(dev, ptr);
Perhaps a blank line here? (really tiny improvement in readability?
> + return hwdev;
> +
> +error:
> + devres_free(ptr);
> + return hwdev;
> +}
> +EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_info);
> +
> static int devm_hwmon_match(struct device *dev, void *res, void *data)
> {
> struct device **hwdev = res;
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 09354f6c1d63..99250ad092fd 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -14,9 +14,121 @@
> #ifndef _HWMON_H_
> #define _HWMON_H_
>
> +#include <linux/bitops.h>
> +
> struct device;
> struct attribute_group;
>
> +enum hwmon_sensor_types {
> + hwmon_chip,
> + hwmon_temp,
> + hwmon_in,
> + hwmon_curr,
> + hwmon_power,
> + hwmon_energy,
> +};
> +
> +enum hwmon_chip_attributes {
> + hwmon_chip_temp_reset_history,
> + hwmon_chip_register_tz,
> + hwmon_chip_update_interval,
> + hwmon_chip_alarms,
> +};
> +
> +#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
> +#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
> +#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
> +#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
> +#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
> +
> +enum hwmon_temp_attributes {
> + hwmon_temp_input = 0,
> + hwmon_temp_type,
> + hwmon_temp_lcrit,
> + hwmon_temp_lcrit_hyst,
> + hwmon_temp_min,
> + hwmon_temp_min_hyst,
> + hwmon_temp_max,
> + hwmon_temp_max_hyst,
> + hwmon_temp_crit,
> + hwmon_temp_crit_hyst,
> + hwmon_temp_emergency,
> + hwmon_temp_emergency_hyst,
> + hwmon_temp_alarm,
> + hwmon_temp_lcrit_alarm,
> + hwmon_temp_min_alarm,
> + hwmon_temp_max_alarm,
> + hwmon_temp_crit_alarm,
> + hwmon_temp_emergency_alarm,
> + hwmon_temp_fault,
> + hwmon_temp_offset,
> + hwmon_temp_label,
> + hwmon_temp_lowest,
> + hwmon_temp_highest,
> + hwmon_temp_reset_history,
> +};
> +
> +#define HWMON_T_INPUT BIT(hwmon_temp_input)
> +#define HWMON_T_TYPE BIT(hwmon_temp_type)
> +#define HWMON_T_LCRIT BIT(hwmon_temp_lcrit)
> +#define HWMON_T_LCRIT_HYST BIT(hwmon_temp_lcrit_hyst)
> +#define HWMON_T_MIN BIT(hwmon_temp_min)
> +#define HWMON_T_MIN_HYST BIT(hwmon_temp_min_hyst)
> +#define HWMON_T_MAX BIT(hwmon_temp_max)
> +#define HWMON_T_MAX_HYST BIT(hwmon_temp_max_hyst)
> +#define HWMON_T_CRIT BIT(hwmon_temp_crit)
> +#define HWMON_T_CRIT_HYST BIT(hwmon_temp_crit_hyst)
> +#define HWMON_T_EMERGENCY BIT(hwmon_temp_emergency)
> +#define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
> +#define HWMON_T_MIN_ALARM BIT(hwmon_temp_min_alarm)
> +#define HWMON_T_MAX_ALARM BIT(hwmon_temp_max_alarm)
> +#define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
> +#define HWMON_T_EMERGENCY_ALARM BIT(hwmon_temp_emergency_alarm)
> +#define HWMON_T_FAULT BIT(hwmon_temp_fault)
> +#define HWMON_T_OFFSET BIT(hwmon_temp_offset)
> +#define HWMON_T_LABEL BIT(hwmon_temp_label)
> +#define HWMON_T_LOWEST BIT(hwmon_temp_lowest)
> +#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
> +#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
> +
> +/**
> + * struct hwmon_ops - hwmon device operations
> + * @is_visible: Callback to return attribute visibility.
> + * Optional.
> + * @read: Read callback. Optional.
I'd like to see a little more expansion in this comment on the form of the
callbacks. What are the parameters etc? I'd expect to look here first
to find that out rather than diving into docs.
> + * @write: Write callback. Optional.
> + */
> +struct hwmon_ops {
> + umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
> + u32 attr, int);
> + int (*read)(struct device *, enum hwmon_sensor_types type,
> + u32 attr, int, long *);
> + int (*write)(struct device *, enum hwmon_sensor_types type,
> + u32 attr, int, long);
> +};
> +
> +/**
> + * Channel information
> + * @type: Channel type.
> + * @config: Pointer to NULL-terminated list of channel parameters.
> + * Use for per-channel attributes.
> + */
> +struct hwmon_channel_info {
> + enum hwmon_sensor_types type;
> + const u32 *config;
Hmm. Be very careful about running out of space. I did much the same
in IIO as you know and am somewhat regretting it now a using a bitmap is
lovely and efficient, but really annoying once you run out of bits
if you've used a fixed (short) length..
Using a bitmap as defined in bitmap.h doesn't work as there is no elegant
way of statically defining them beyond the first long. I really wish
there was a way of doing this but can't figure out how..
Maybe just go for a u64 from the start (though that then complicates the
use of some of the bitops stuff) I suspect we'll be jumping to
that in IIO fairly soon and it'd be a lot less painful to start there
perhaps...
> +};
> +
> +/**
> + * Chip configuration
> + * @ops: Pointer to hwmon operations.
> + * @info: Null-terminated list of channel information.
Drop the bonus blank line.
We tossed up in IIO for a long time on whether to use a null terminated list
of our equivalent info elements, or an explict count. Ended up with count
as it often allows sharing of the big array of channel info structures
between different parts where one part adds more hardware over the other..
I'm not saying that the gain by doing that was all that signficant, but
I thought I'd mention the trade offs.
Here I think you have made the right decision.
> + *
> + */
> +struct hwmon_chip_info {
> + const struct hwmon_ops *ops;
> + const struct hwmon_channel_info **info;
> +};
> +
> struct device *hwmon_device_register(struct device *dev);
> struct device *
> hwmon_device_register_with_groups(struct device *dev, const char *name,
> @@ -26,6 +138,16 @@ struct device *
> devm_hwmon_device_register_with_groups(struct device *dev, const char *name,
> void *drvdata,
> const struct attribute_group **groups);
> +struct device *
> +hwmon_device_register_with_info(struct device *dev,
> + const char *name, void *drvdata,
> + const struct hwmon_chip_info *info,
> + const struct attribute_group **groups);
> +struct device *
> +devm_hwmon_device_register_with_info(struct device *dev,
> + const char *name, void *drvdata,
> + const struct hwmon_chip_info *info,
> + const struct attribute_group **groups);
>
> void hwmon_device_unregister(struct device *dev);
> void devm_hwmon_device_unregister(struct device *dev);
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] hwmon: (core) New hwmon registration API
@ 2016-07-11 1:31 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-07-11 1:31 UTC (permalink / raw)
To: Jonathan Cameron, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm, linux-iio, linux-hwmon,
linux-kernel
Hi Jonathan,
On 07/10/2016 08:51 AM, Jonathan Cameron wrote:
> On 26/06/16 04:26, Guenter Roeck wrote:
>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>> This requires a lot of template code, and distracts from the driver's core
>> function to read and write chip registers.
>>
>> To be able to reduce driver complexity, move sensor attribute handling
>> and thermal zone registration into hwmon core. By using the new API,
>> driver size is typically reduced by 20-50% depending on driver complexity
>> and the number of sysfs attributes supported.
>>
>> With this patch, the new API only supports thermal sensors. Support for
>> other sensor types will be added with subsequent patches.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Hi Guenter.
>
> Sorry it took me so long to get to this....
>
> Anyhow, mostly looks clean, effective and consise to me.
> Various bits and pieces inline. There are a few bits I might (when time
> allows) lift over to iio as they are nicer than what we have.
>
> My one lesson learned from IIO is always be wary of space in bitmaps. We
> haven't run out yet but are getting close. You may end up in a similar
> state here a few years down the line.
>
Yes, I spend a lot of time arguing with myself over u32 vs. u64. I decided
to stick with u32 for now, reasons being that there are multiple number spaces
and that the bit mask is only really used for registration (and thus relatively
easy to change).
More comments inline.
> Jonathan
>> ---
>> drivers/hwmon/hwmon.c | 485 ++++++++++++++++++++++++++++++++++++++++++++++----
>> include/linux/hwmon.h | 122 +++++++++++++
>> 2 files changed, 574 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>> index a26c385a435b..9530644ae297 100644
>> --- a/drivers/hwmon/hwmon.c
>> +++ b/drivers/hwmon/hwmon.c
>> @@ -12,17 +12,16 @@
>>
>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>> -#include <linux/module.h>
>> +#include <linux/bitops.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> -#include <linux/slab.h>
>> -#include <linux/kdev_t.h>
>> -#include <linux/idr.h>
>> #include <linux/hwmon.h>
>> -#include <linux/gfp.h>
>> -#include <linux/spinlock.h>
>> +#include <linux/idr.h>
>> +#include <linux/module.h>
> Some unconnected changes in this. Arguably reordering is good and
> all but should be in a precursor patch so it's obvious what has
> been added or removed here.
>
Split into a separate patch.
>> #include <linux/pci.h>
>> +#include <linux/slab.h>
>> #include <linux/string.h>
>> +#include <linux/thermal.h>
>>
>> #define HWMON_ID_PREFIX "hwmon"
>> #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
>> @@ -30,9 +29,33 @@
>> struct hwmon_device {
>> const char *name;
>> struct device dev;
>> + const struct hwmon_chip_info *chip;
>> +
>> + struct attribute_group group;
>> + const struct attribute_group **groups;
>> };
>> #define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
>>
>> +struct hwmon_device_attribute {
>> + struct device_attribute dev_attr;
>> + const struct hwmon_ops *ops;
>> + enum hwmon_sensor_types type;
>> + u32 attr;
>> + int index;
>> +};
>> +#define to_hwmon_attr(d) \
>> + container_of(d, struct hwmon_device_attribute, dev_attr)
>> +
>> +/*
>> + * Thermal zone information
>> + * In addition to the reference to the hwmon device,
>> + * also provides the sensor index.
>> + */
>> +struct hwmon_thermal_data {
>> + struct hwmon_device *hwdev; /* Reference to hwmon device */
>> + int index; /* sensor index */
>> +};
>> +
>> static ssize_t
>> show_name(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> @@ -80,25 +103,280 @@ static struct class hwmon_class = {
>>
>> static DEFINE_IDA(hwmon_ida);
>>
>> -/**
>> - * hwmon_device_register_with_groups - register w/ hwmon
>> - * @dev: the parent device
>> - * @name: hwmon name attribute
>> - * @drvdata: driver data to attach to created device
>> - * @groups: List of attribute groups to create
>> - *
>> - * hwmon_device_unregister() must be called when the device is no
>> - * longer needed.
>> - *
>> - * Returns the pointer to the new device.
>> - */
>> -struct device *
>> -hwmon_device_register_with_groups(struct device *dev, const char *name,
>> - void *drvdata,
>> - const struct attribute_group **groups)
>> +/* Thermal zone handling */
>> +
>> +static int hwmon_thermal_get_temp(void *data, int *temp)
>> +{
>> + struct hwmon_thermal_data *tdata = data;
>> + struct hwmon_device *hwdev = tdata->hwdev;
>> + int ret;
>> + long t;
>> +
>> + ret = hwdev->chip->ops->read(&hwdev->dev, hwmon_temp, hwmon_temp_input,
>> + tdata->index, &t);
>> + if (ret < 0)
>> + return ret;
>> +
>> + *temp = t;
>> +
>> + return 0;
>> +}
>> +
>> +static struct thermal_zone_of_device_ops hwmon_thermal_ops = {
>> + .get_temp = hwmon_thermal_get_temp,
>> +};
>> +
>> +static int hwmon_thermal_add_sensor(struct device *dev,
>> + struct hwmon_device *hwdev,
>> + int index)
>> +{
>> + struct hwmon_thermal_data *tdata;
>> +
>> + tdata = devm_kzalloc(dev, sizeof(*tdata), GFP_KERNEL);
>> + if (!tdata)
>> + return -ENOMEM;
>> +
>> + tdata->hwdev = hwdev;
>> + tdata->index = index;
>> +
>> + devm_thermal_zone_of_sensor_register(&hwdev->dev, index, tdata,
>> + &hwmon_thermal_ops);
>> +
>> + return 0;
>> +}
>> +
>> +/* sysfs attribute management */
>> +
>> +static ssize_t hwmon_attr_show(struct device *dev,
>> + struct device_attribute *devattr, char *buf)
>> +{
>> + struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>> + long val;
>> + int ret;
>> +
>> + ret = hattr->ops->read(dev, hattr->type, hattr->attr, hattr->index,
>> + &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return sprintf(buf, "%ld\n", val);
>> +}
>> +
>> +static ssize_t hwmon_attr_store(struct device *dev,
>> + struct device_attribute *devattr,
>> + const char *buf, size_t count)
>> +{
>> + struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>> + long val;
>> + int ret;
>> +
>> + ret = kstrtol(buf, 10, &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = hattr->ops->write(dev, hattr->type, hattr->attr, hattr->index,
>> + val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return count;
>> +}
>> +
>> +static int hwmon_attr_base(enum hwmon_sensor_types type)
>> +{
>> + if (type == hwmon_in)
>> + return 0;
>> + return 1;
>> +}
>> +
>> +static struct attribute *hwmon_genattr(struct device *dev,
>> + const void *drvdata,
>> + enum hwmon_sensor_types type,
>> + u32 attr,
>> + int index,
>> + const char *template,
>> + const struct hwmon_ops *ops)
>> +{
>> + struct hwmon_device_attribute *hattr;
>> + struct device_attribute *dattr;
>> + struct attribute *a;
>> + umode_t mode;
>> + char *name;
>> +
>> + if (!template)
>> + return ERR_PTR(-EINVAL);
>> +
>> + mode = ops->is_visible(drvdata, type, attr, index);
>> + if (!mode)
>> + return ERR_PTR(-ENOENT);
>> +
> I've been wondering about doing something similar to this in IIO for a while
> as our attributes are far to often rw when they should only be one or the
> other. This is much more elegant. We could get away with it (nasty as it is)
> on the basis of no previous ABI to match where as you have to have this...
> Can see this is_visible callback might get a little messy in some cases.
>
So far it wasn't that bad. For chips supporting multiple sensor types,
I found it useful to have per-type functions in the driver. I also thought about
using per-type callbacks, but decided against it to avoid large (and mostly
empty) data structures in the driver. Also, hwmon drivers always had to deal with
this situation, so the code to determine sensor visibility is already there
(either by implementing the is_visible callback in struct attribute_group,
or by dynamically selecting which attribute groups are needed in the probe
function).
> Actually, is is_visible a good name? I'd think that would control whether
> we can see the attribute at all. Perhaps something like
> get_filemode would be a better callback naming?
>
I picked is_visible because that is the name used in struct attribute_group,
and it has the same functionality there.
>> + if ((mode & S_IRUGO) && !ops->read)
>> + return ERR_PTR(-EINVAL);
>> + if ((mode & S_IWUGO) && !ops->write)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (type == hwmon_chip) {
>> + name = (char *)template;
>> + } else {
>> + name = devm_kzalloc(dev, strlen(template) + 16, GFP_KERNEL);
>> + if (!name)
>> + return ERR_PTR(-ENOMEM);
>> + scnprintf(name, strlen(template) + 16, template,
>> + index + hwmon_attr_base(type));
>> + }
>> +
>> + hattr = devm_kzalloc(dev, sizeof(*hattr), GFP_KERNEL);
>> + if (!hattr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + hattr->type = type;
>> + hattr->attr = attr;
>> + hattr->index = index;
>> + hattr->ops = ops;
>> +
>> + dattr = &hattr->dev_attr;
>> + if (mode & S_IRUGO)
>> + dattr->show = hwmon_attr_show;
>> + if (mode & S_IWUGO)
>> + dattr->store = hwmon_attr_store;
> Technically I think there is no need to worry about setting the store / show
> when they aren't needed. Maybe, not worth the really small savings though...
Yes, you are right. Not to save some code, but the if statements are really not
necessary and don't add any value.
>> +
>> + a = &dattr->attr;
>> + sysfs_attr_init(a);
>> + a->name = name;
>> + a->mode = mode;
>> +
>> + return a;
>> +}
>> +
>> +static const char * const hwmon_chip_attr_templates[] = {
>> + [hwmon_chip_temp_reset_history] = "temp_reset_history",
>> + [hwmon_chip_update_interval] = "update_interval",
>> + [hwmon_chip_alarms] = "alarms",
>> +};
>> +
>> +static const char * const hwmon_temp_attr_templates[] = {
>> + [hwmon_temp_input] = "temp%d_input",
>> + [hwmon_temp_type] = "temp%d_type",
>> + [hwmon_temp_lcrit] = "temp%d_lcrit",
>> + [hwmon_temp_lcrit_hyst] = "temp%d_lcrit_hyst",
>> + [hwmon_temp_min] = "temp%d_min",
>> + [hwmon_temp_min_hyst] = "temp%d_min_hyst",
>> + [hwmon_temp_max] = "temp%d_max",
>> + [hwmon_temp_max_hyst] = "temp%d_max_hyst",
>> + [hwmon_temp_crit] = "temp%d_crit",
>> + [hwmon_temp_crit_hyst] = "temp%d_crit_hyst",
>> + [hwmon_temp_emergency] = "temp%d_emergency",
>> + [hwmon_temp_emergency_hyst] = "temp%d_emergency_hyst",
>> + [hwmon_temp_alarm] = "temp%d_alarm",
>> + [hwmon_temp_lcrit_alarm] = "temp%d_lcrit_alarm",
>> + [hwmon_temp_min_alarm] = "temp%d_min_alarm",
>> + [hwmon_temp_max_alarm] = "temp%d_max_alarm",
>> + [hwmon_temp_crit_alarm] = "temp%d_crit_alarm",
>> + [hwmon_temp_emergency_alarm] = "temp%d_emergency_alarm",
>> + [hwmon_temp_fault] = "temp%d_fault",
>> + [hwmon_temp_offset] = "temp%d_offset",
>> + [hwmon_temp_label] = "temp%d_label",
>> + [hwmon_temp_lowest] = "temp%d_lowest",
>> + [hwmon_temp_highest] = "temp%d_highest",
>> + [hwmon_temp_reset_history] = "temp%d_reset_history",
>> +};
>> +
>> +static const char * const *__templates[] = {
>> + [hwmon_chip] = hwmon_chip_attr_templates,
>> + [hwmon_temp] = hwmon_temp_attr_templates,
>> +};
>> +
>> +static const int __templates_size[] = {
>> + [hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
>> + [hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
>> +};
>> +
>> +static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
>> +{
>> + int i, n;
>> +
>> + for (i = n = 0; info->config[i]; i++)
>> + n += hweight32(info->config[i]);
>> +
>> + return n;
>> +}
>> +
>> +static int hwmon_genattrs(struct device *dev,
>> + const void *drvdata,
>> + struct attribute **attrs,
>> + const struct hwmon_ops *ops,
>> + const struct hwmon_channel_info *info)
>> +{
>> + const char * const *templates;
>> + int template_size;
>> + int i, aindex = 0;
>> +
>> + if (info->type >= ARRAY_SIZE(__templates))
>> + return -EINVAL;
>> +
>> + templates = __templates[info->type];
>> + template_size = __templates_size[info->type];
>> +
>> + for (i = 0; info->config[i]; i++) {
>> + u32 attr_mask = info->config[i];
>> + u32 attr;
>> +
>> + while (attr_mask) {
>> + struct attribute *a;
>> +
>> + attr = __ffs(attr_mask);
>> + attr_mask &= ~BIT(attr);
> loop instead using the for_each_bit_set? Tiny bit cleaner perhaps...
A bit, maybe, though not much. I changed it, but since for_each_set_bit() requires
a pointer to an unsigned long as argument, I still need attr_mask as temporary
variable (I don't want to use unsigned long for the config bit mask since
its size is not well defined). Hmm ... that also means that switching to u64
in the future will be tricky. I'll need to think about that some more.
>> + if (attr >= template_size)
>> + return -EINVAL;
>> + a = hwmon_genattr(dev, drvdata, info->type, attr, i,
>> + templates[attr], ops);
>> + if (IS_ERR(a)) {
>> + if (PTR_ERR(a) != -ENOENT)
>> + return PTR_ERR(a);
>> + continue;
>> + }
>> + attrs[aindex++] = a;
>> + }
>> + }
>> + return aindex;
>> +}
>> +
>> +static struct attribute **
>> +__hwmon_create_attrs(struct device *dev, const void *drvdata,
>> + const struct hwmon_chip_info *chip)
>> +{
>> + int ret, i, aindex = 0, nattrs = 0;
>> + struct attribute **attrs;
>> +
>> + for (i = 0; chip->info[i]; i++)
>> + nattrs += hwmon_num_channel_attrs(chip->info[i]);
>> +
>> + if (nattrs == 0)
>> + return ERR_PTR(-EINVAL);
>> +
>> + attrs = devm_kcalloc(dev, nattrs + 1, sizeof(*attrs), GFP_KERNEL);
>> + if (attrs == NULL)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + for (i = 0; chip->info[i]; i++) {
>> + ret = hwmon_genattrs(dev, drvdata, &attrs[aindex], chip->ops,
>> + chip->info[i]);
>> + if (ret < 0)
>> + return ERR_PTR(ret);
>> + aindex += ret;
>> + }
>> +
>> + return attrs;
>> +}
>> +
>> +static struct device *
>> +__hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>> + const struct hwmon_chip_info *chip,
>> + const struct attribute_group **groups)
>> {
>> struct hwmon_device *hwdev;
>> - int err, id;
>> + struct device *hdev;
>> + int i, j, err, id;
>>
>> /* Do not accept invalid characters in hwmon name attribute */
>> if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
>> @@ -114,28 +392,128 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
>> goto ida_remove;
>> }
>>
>> + hdev = &hwdev->dev;
>> +
>> + if (chip && chip->ops->is_visible) {
>> + struct attribute **attrs;
>> + int ngroups = 2;
>> +
>> + if (groups)
>> + for (i = 0; groups[i]; i++)
>> + ngroups++;
>> +
>> + hwdev->groups = devm_kcalloc(dev, ngroups, sizeof(*groups),
>> + GFP_KERNEL);
>> + if (hwdev->groups == NULL)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + attrs = __hwmon_create_attrs(dev, drvdata, chip);
>> + if (IS_ERR(attrs)) {
>> + err = PTR_ERR(attrs);
>> + goto free_hwmon;
>> + }
>> +
>> + hwdev->group.attrs = attrs;
>> + ngroups = 0;
>> + hwdev->groups[ngroups++] = &hwdev->group;
>> +
>> + if (groups) {
>> + for (i = 0; groups[i]; i++)
>> + hwdev->groups[ngroups++] = groups[i];
>> + }
>> +
>> + hdev->groups = hwdev->groups;
>> + } else {
>> + hdev->groups = groups;
>> + }
>> +
>> hwdev->name = name;
>> - hwdev->dev.class = &hwmon_class;
>> - hwdev->dev.parent = dev;
>> - hwdev->dev.groups = groups;
>> - hwdev->dev.of_node = dev ? dev->of_node : NULL;
>> - dev_set_drvdata(&hwdev->dev, drvdata);
>> - dev_set_name(&hwdev->dev, HWMON_ID_FORMAT, id);
>> - err = device_register(&hwdev->dev);
>> + hdev->class = &hwmon_class;
>> + hdev->parent = dev;
>> + hdev->of_node = dev ? dev->of_node : NULL;
>> + hwdev->chip = chip;
>> + dev_set_drvdata(hdev, drvdata);
>> + dev_set_name(hdev, HWMON_ID_FORMAT, id);
>> + err = device_register(hdev);
>> if (err)
>> - goto free;
>> + goto free_hwmon;
>> +
>> + if (chip && chip->ops->is_visible && chip->ops->read &&
>> + chip->info[0]->type == hwmon_chip &&
>> + (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
>> + const struct hwmon_channel_info **info = chip->info;
>> +
>> + for (i = 1; info[i]; i++) {
>> + if (info[i]->type != hwmon_temp)
>> + continue;
>> +
>> + for (j = 0; info[i]->config[j]; j++) {
>> + if (!chip->ops->is_visible(drvdata, hwmon_temp,
>> + hwmon_temp_input, j))
>> + continue;
>> + if (info[i]->config[j] & HWMON_T_INPUT)
>> + hwmon_thermal_add_sensor(dev, hwdev, j);
>> + }
>> + }
>> + }
>>
>> - return &hwdev->dev;
>> + return hdev;
>>
>> -free:
>> +free_hwmon:
>> kfree(hwdev);
>> ida_remove:
>> ida_simple_remove(&hwmon_ida, id);
>> return ERR_PTR(err);
>> }
>> +
>> +/**
>> + * hwmon_device_register_with_groups - register w/ hwmon
>> + * @dev: the parent device
>> + * @name: hwmon name attribute
>> + * @drvdata: driver data to attach to created device
>> + * @groups: List of attribute groups to create
>> + *
>> + * hwmon_device_unregister() must be called when the device is no
>> + * longer needed.
>> + *
>> + * Returns the pointer to the new device.
>> + */
>> +struct device *
>> +hwmon_device_register_with_groups(struct device *dev, const char *name,
>> + void *drvdata,
>> + const struct attribute_group **groups)
>> +{
>> + return __hwmon_device_register(dev, name, drvdata, NULL, groups);
>> +}
>> EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
>>
>> /**
>> + * hwmon_device_register_with_info - register w/ hwmon
>> + * @dev: the parent device
>> + * @name: hwmon name attribute
>> + * @drvdata: driver data to attach to created device
>> + * @info: Pointer to hwmon chip information
>> + * @groups - pointer to list of driver specific attribute groups
>> + *
>> + * hwmon_device_unregister() must be called when the device is no
>> + * longer needed.
>> + *
>> + * Returns the pointer to the new device.
>> + */
>> +struct device *
>> +hwmon_device_register_with_info(struct device *dev, const char *name,
>> + void *drvdata,
>> + const struct hwmon_chip_info *chip,
>> + const struct attribute_group **groups)
>> +{
>> + if (chip && (!chip->ops || !chip->info))
>> + return ERR_PTR(-EINVAL);
>> +
>> + return __hwmon_device_register(dev, name, drvdata, chip, groups);
>> +}
>> +EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
>> +
>> +/**
>> * hwmon_device_register - register w/ hwmon
>> * @dev: the device to register
>> *
>> @@ -213,6 +591,47 @@ error:
>> }
>> EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
>>
>> +/**
>> + * devm_hwmon_device_register_with_info - register w/ hwmon
>> + * @dev: the parent device
>> + * @name: hwmon name attribute
>> + * @drvdata: driver data to attach to created device
>> + * @info: Pointer to hwmon chip information
>> + * @groups - pointer to list of driver specific attribute groups
>> + *
>> + * Returns the pointer to the new device. The new device is automatically
>> + * unregistered with the parent device.
>> + */
>> +struct device *
>> +devm_hwmon_device_register_with_info(struct device *dev, const char *name,
>> + void *drvdata,
>> + const struct hwmon_chip_info *chip,
>> + const struct attribute_group **groups)
>> +{
>> + struct device **ptr, *hwdev;
>> +
>> + if (!dev)
>> + return ERR_PTR(-EINVAL);
>> +
>> + ptr = devres_alloc(devm_hwmon_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
>> + groups);
>> + if (IS_ERR(hwdev))
>> + goto error;
>> +
>> + *ptr = hwdev;
>> + devres_add(dev, ptr);
> Perhaps a blank line here? (really tiny improvement in readability?
>> + return hwdev;
>> +
>> +error:
>> + devres_free(ptr);
>> + return hwdev;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_info);
>> +
>> static int devm_hwmon_match(struct device *dev, void *res, void *data)
>> {
>> struct device **hwdev = res;
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index 09354f6c1d63..99250ad092fd 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -14,9 +14,121 @@
>> #ifndef _HWMON_H_
>> #define _HWMON_H_
>>
>> +#include <linux/bitops.h>
>> +
>> struct device;
>> struct attribute_group;
>>
>> +enum hwmon_sensor_types {
>> + hwmon_chip,
>> + hwmon_temp,
>> + hwmon_in,
>> + hwmon_curr,
>> + hwmon_power,
>> + hwmon_energy,
>> +};
>> +
>> +enum hwmon_chip_attributes {
>> + hwmon_chip_temp_reset_history,
>> + hwmon_chip_register_tz,
>> + hwmon_chip_update_interval,
>> + hwmon_chip_alarms,
>> +};
>> +
>> +#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
>> +#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
>> +#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
>> +#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
>> +#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
>> +
>> +enum hwmon_temp_attributes {
>> + hwmon_temp_input = 0,
>> + hwmon_temp_type,
>> + hwmon_temp_lcrit,
>> + hwmon_temp_lcrit_hyst,
>> + hwmon_temp_min,
>> + hwmon_temp_min_hyst,
>> + hwmon_temp_max,
>> + hwmon_temp_max_hyst,
>> + hwmon_temp_crit,
>> + hwmon_temp_crit_hyst,
>> + hwmon_temp_emergency,
>> + hwmon_temp_emergency_hyst,
>> + hwmon_temp_alarm,
>> + hwmon_temp_lcrit_alarm,
>> + hwmon_temp_min_alarm,
>> + hwmon_temp_max_alarm,
>> + hwmon_temp_crit_alarm,
>> + hwmon_temp_emergency_alarm,
>> + hwmon_temp_fault,
>> + hwmon_temp_offset,
>> + hwmon_temp_label,
>> + hwmon_temp_lowest,
>> + hwmon_temp_highest,
>> + hwmon_temp_reset_history,
>> +};
>> +
>> +#define HWMON_T_INPUT BIT(hwmon_temp_input)
>> +#define HWMON_T_TYPE BIT(hwmon_temp_type)
>> +#define HWMON_T_LCRIT BIT(hwmon_temp_lcrit)
>> +#define HWMON_T_LCRIT_HYST BIT(hwmon_temp_lcrit_hyst)
>> +#define HWMON_T_MIN BIT(hwmon_temp_min)
>> +#define HWMON_T_MIN_HYST BIT(hwmon_temp_min_hyst)
>> +#define HWMON_T_MAX BIT(hwmon_temp_max)
>> +#define HWMON_T_MAX_HYST BIT(hwmon_temp_max_hyst)
>> +#define HWMON_T_CRIT BIT(hwmon_temp_crit)
>> +#define HWMON_T_CRIT_HYST BIT(hwmon_temp_crit_hyst)
>> +#define HWMON_T_EMERGENCY BIT(hwmon_temp_emergency)
>> +#define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
>> +#define HWMON_T_MIN_ALARM BIT(hwmon_temp_min_alarm)
>> +#define HWMON_T_MAX_ALARM BIT(hwmon_temp_max_alarm)
>> +#define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
>> +#define HWMON_T_EMERGENCY_ALARM BIT(hwmon_temp_emergency_alarm)
>> +#define HWMON_T_FAULT BIT(hwmon_temp_fault)
>> +#define HWMON_T_OFFSET BIT(hwmon_temp_offset)
>> +#define HWMON_T_LABEL BIT(hwmon_temp_label)
>> +#define HWMON_T_LOWEST BIT(hwmon_temp_lowest)
>> +#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
>> +#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
>> +
>> +/**
>> + * struct hwmon_ops - hwmon device operations
>> + * @is_visible: Callback to return attribute visibility.
>> + * Optional.
>> + * @read: Read callback. Optional.
> I'd like to see a little more expansion in this comment on the form of the
> callbacks. What are the parameters etc? I'd expect to look here first
> to find that out rather than diving into docs.
Done.
>> + * @write: Write callback. Optional.
>> + */
>> +struct hwmon_ops {
>> + umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
>> + u32 attr, int);
>> + int (*read)(struct device *, enum hwmon_sensor_types type,
>> + u32 attr, int, long *);
>> + int (*write)(struct device *, enum hwmon_sensor_types type,
>> + u32 attr, int, long);
>> +};
>> +
>> +/**
>> + * Channel information
>> + * @type: Channel type.
>> + * @config: Pointer to NULL-terminated list of channel parameters.
>> + * Use for per-channel attributes.
>> + */
>> +struct hwmon_channel_info {
>> + enum hwmon_sensor_types type;
>> + const u32 *config;
> Hmm. Be very careful about running out of space. I did much the same
> in IIO as you know and am somewhat regretting it now a using a bitmap is
> lovely and efficient, but really annoying once you run out of bits
> if you've used a fixed (short) length..
>
> Using a bitmap as defined in bitmap.h doesn't work as there is no elegant
> way of statically defining them beyond the first long. I really wish
> there was a way of doing this but can't figure out how..
>
Me not either, which is why I did not use it.
> Maybe just go for a u64 from the start (though that then complicates the
> use of some of the bitops stuff) I suspect we'll be jumping to
> that in IIO fairly soon and it'd be a lot less painful to start there
> perhaps...
>
My solution trying to avoid that was to use separate number spaces
for the different sensor types. If/when we hit the limit (temperatures
are getting close), we would have two options: Define a new sensor type
to handle the overflow, or switch to u64. Ultimately I decided to stick
with u32 for now because u64 seemed overkill. On the other side, I made
that decision when the read/write/is_visible API to the drivers included
the bit mask, which is no longer the case. Maybe I'll try with u64 to see
its impact on code size.
>
>> +};
>> +
>> +/**
>> + * Chip configuration
>> + * @ops: Pointer to hwmon operations.
>> + * @info: Null-terminated list of channel information.
> Drop the bonus blank line.
>
> We tossed up in IIO for a long time on whether to use a null terminated list
> of our equivalent info elements, or an explict count. Ended up with count
> as it often allows sharing of the big array of channel info structures
> between different parts where one part adds more hardware over the other..
>
> I'm not saying that the gain by doing that was all that signficant, but
> I thought I'd mention the trade offs.
I went through the same thinking process. At the end, I decided to go with
the terminated list since passing it around seemed to be simpler.
The core has to walk through the list anyway, and the hwmon code does not
require big data structures to describe the channels.
> Here I think you have made the right decision.
Thanks for the feedback, and thanks a lot for your time!
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] hwmon: (core) New hwmon registration API
@ 2016-07-11 1:31 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-07-11 1:31 UTC (permalink / raw)
To: Jonathan Cameron, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Jonathan,
On 07/10/2016 08:51 AM, Jonathan Cameron wrote:
> On 26/06/16 04:26, Guenter Roeck wrote:
>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>> This requires a lot of template code, and distracts from the driver's core
>> function to read and write chip registers.
>>
>> To be able to reduce driver complexity, move sensor attribute handling
>> and thermal zone registration into hwmon core. By using the new API,
>> driver size is typically reduced by 20-50% depending on driver complexity
>> and the number of sysfs attributes supported.
>>
>> With this patch, the new API only supports thermal sensors. Support for
>> other sensor types will be added with subsequent patches.
>>
>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Hi Guenter.
>
> Sorry it took me so long to get to this....
>
> Anyhow, mostly looks clean, effective and consise to me.
> Various bits and pieces inline. There are a few bits I might (when time
> allows) lift over to iio as they are nicer than what we have.
>
> My one lesson learned from IIO is always be wary of space in bitmaps. We
> haven't run out yet but are getting close. You may end up in a similar
> state here a few years down the line.
>
Yes, I spend a lot of time arguing with myself over u32 vs. u64. I decided
to stick with u32 for now, reasons being that there are multiple number spaces
and that the bit mask is only really used for registration (and thus relatively
easy to change).
More comments inline.
> Jonathan
>> ---
>> drivers/hwmon/hwmon.c | 485 ++++++++++++++++++++++++++++++++++++++++++++++----
>> include/linux/hwmon.h | 122 +++++++++++++
>> 2 files changed, 574 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>> index a26c385a435b..9530644ae297 100644
>> --- a/drivers/hwmon/hwmon.c
>> +++ b/drivers/hwmon/hwmon.c
>> @@ -12,17 +12,16 @@
>>
>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>
>> -#include <linux/module.h>
>> +#include <linux/bitops.h>
>> #include <linux/device.h>
>> #include <linux/err.h>
>> -#include <linux/slab.h>
>> -#include <linux/kdev_t.h>
>> -#include <linux/idr.h>
>> #include <linux/hwmon.h>
>> -#include <linux/gfp.h>
>> -#include <linux/spinlock.h>
>> +#include <linux/idr.h>
>> +#include <linux/module.h>
> Some unconnected changes in this. Arguably reordering is good and
> all but should be in a precursor patch so it's obvious what has
> been added or removed here.
>
Split into a separate patch.
>> #include <linux/pci.h>
>> +#include <linux/slab.h>
>> #include <linux/string.h>
>> +#include <linux/thermal.h>
>>
>> #define HWMON_ID_PREFIX "hwmon"
>> #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
>> @@ -30,9 +29,33 @@
>> struct hwmon_device {
>> const char *name;
>> struct device dev;
>> + const struct hwmon_chip_info *chip;
>> +
>> + struct attribute_group group;
>> + const struct attribute_group **groups;
>> };
>> #define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
>>
>> +struct hwmon_device_attribute {
>> + struct device_attribute dev_attr;
>> + const struct hwmon_ops *ops;
>> + enum hwmon_sensor_types type;
>> + u32 attr;
>> + int index;
>> +};
>> +#define to_hwmon_attr(d) \
>> + container_of(d, struct hwmon_device_attribute, dev_attr)
>> +
>> +/*
>> + * Thermal zone information
>> + * In addition to the reference to the hwmon device,
>> + * also provides the sensor index.
>> + */
>> +struct hwmon_thermal_data {
>> + struct hwmon_device *hwdev; /* Reference to hwmon device */
>> + int index; /* sensor index */
>> +};
>> +
>> static ssize_t
>> show_name(struct device *dev, struct device_attribute *attr, char *buf)
>> {
>> @@ -80,25 +103,280 @@ static struct class hwmon_class = {
>>
>> static DEFINE_IDA(hwmon_ida);
>>
>> -/**
>> - * hwmon_device_register_with_groups - register w/ hwmon
>> - * @dev: the parent device
>> - * @name: hwmon name attribute
>> - * @drvdata: driver data to attach to created device
>> - * @groups: List of attribute groups to create
>> - *
>> - * hwmon_device_unregister() must be called when the device is no
>> - * longer needed.
>> - *
>> - * Returns the pointer to the new device.
>> - */
>> -struct device *
>> -hwmon_device_register_with_groups(struct device *dev, const char *name,
>> - void *drvdata,
>> - const struct attribute_group **groups)
>> +/* Thermal zone handling */
>> +
>> +static int hwmon_thermal_get_temp(void *data, int *temp)
>> +{
>> + struct hwmon_thermal_data *tdata = data;
>> + struct hwmon_device *hwdev = tdata->hwdev;
>> + int ret;
>> + long t;
>> +
>> + ret = hwdev->chip->ops->read(&hwdev->dev, hwmon_temp, hwmon_temp_input,
>> + tdata->index, &t);
>> + if (ret < 0)
>> + return ret;
>> +
>> + *temp = t;
>> +
>> + return 0;
>> +}
>> +
>> +static struct thermal_zone_of_device_ops hwmon_thermal_ops = {
>> + .get_temp = hwmon_thermal_get_temp,
>> +};
>> +
>> +static int hwmon_thermal_add_sensor(struct device *dev,
>> + struct hwmon_device *hwdev,
>> + int index)
>> +{
>> + struct hwmon_thermal_data *tdata;
>> +
>> + tdata = devm_kzalloc(dev, sizeof(*tdata), GFP_KERNEL);
>> + if (!tdata)
>> + return -ENOMEM;
>> +
>> + tdata->hwdev = hwdev;
>> + tdata->index = index;
>> +
>> + devm_thermal_zone_of_sensor_register(&hwdev->dev, index, tdata,
>> + &hwmon_thermal_ops);
>> +
>> + return 0;
>> +}
>> +
>> +/* sysfs attribute management */
>> +
>> +static ssize_t hwmon_attr_show(struct device *dev,
>> + struct device_attribute *devattr, char *buf)
>> +{
>> + struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>> + long val;
>> + int ret;
>> +
>> + ret = hattr->ops->read(dev, hattr->type, hattr->attr, hattr->index,
>> + &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return sprintf(buf, "%ld\n", val);
>> +}
>> +
>> +static ssize_t hwmon_attr_store(struct device *dev,
>> + struct device_attribute *devattr,
>> + const char *buf, size_t count)
>> +{
>> + struct hwmon_device_attribute *hattr = to_hwmon_attr(devattr);
>> + long val;
>> + int ret;
>> +
>> + ret = kstrtol(buf, 10, &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = hattr->ops->write(dev, hattr->type, hattr->attr, hattr->index,
>> + val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return count;
>> +}
>> +
>> +static int hwmon_attr_base(enum hwmon_sensor_types type)
>> +{
>> + if (type == hwmon_in)
>> + return 0;
>> + return 1;
>> +}
>> +
>> +static struct attribute *hwmon_genattr(struct device *dev,
>> + const void *drvdata,
>> + enum hwmon_sensor_types type,
>> + u32 attr,
>> + int index,
>> + const char *template,
>> + const struct hwmon_ops *ops)
>> +{
>> + struct hwmon_device_attribute *hattr;
>> + struct device_attribute *dattr;
>> + struct attribute *a;
>> + umode_t mode;
>> + char *name;
>> +
>> + if (!template)
>> + return ERR_PTR(-EINVAL);
>> +
>> + mode = ops->is_visible(drvdata, type, attr, index);
>> + if (!mode)
>> + return ERR_PTR(-ENOENT);
>> +
> I've been wondering about doing something similar to this in IIO for a while
> as our attributes are far to often rw when they should only be one or the
> other. This is much more elegant. We could get away with it (nasty as it is)
> on the basis of no previous ABI to match where as you have to have this...
> Can see this is_visible callback might get a little messy in some cases.
>
So far it wasn't that bad. For chips supporting multiple sensor types,
I found it useful to have per-type functions in the driver. I also thought about
using per-type callbacks, but decided against it to avoid large (and mostly
empty) data structures in the driver. Also, hwmon drivers always had to deal with
this situation, so the code to determine sensor visibility is already there
(either by implementing the is_visible callback in struct attribute_group,
or by dynamically selecting which attribute groups are needed in the probe
function).
> Actually, is is_visible a good name? I'd think that would control whether
> we can see the attribute at all. Perhaps something like
> get_filemode would be a better callback naming?
>
I picked is_visible because that is the name used in struct attribute_group,
and it has the same functionality there.
>> + if ((mode & S_IRUGO) && !ops->read)
>> + return ERR_PTR(-EINVAL);
>> + if ((mode & S_IWUGO) && !ops->write)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (type == hwmon_chip) {
>> + name = (char *)template;
>> + } else {
>> + name = devm_kzalloc(dev, strlen(template) + 16, GFP_KERNEL);
>> + if (!name)
>> + return ERR_PTR(-ENOMEM);
>> + scnprintf(name, strlen(template) + 16, template,
>> + index + hwmon_attr_base(type));
>> + }
>> +
>> + hattr = devm_kzalloc(dev, sizeof(*hattr), GFP_KERNEL);
>> + if (!hattr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + hattr->type = type;
>> + hattr->attr = attr;
>> + hattr->index = index;
>> + hattr->ops = ops;
>> +
>> + dattr = &hattr->dev_attr;
>> + if (mode & S_IRUGO)
>> + dattr->show = hwmon_attr_show;
>> + if (mode & S_IWUGO)
>> + dattr->store = hwmon_attr_store;
> Technically I think there is no need to worry about setting the store / show
> when they aren't needed. Maybe, not worth the really small savings though...
Yes, you are right. Not to save some code, but the if statements are really not
necessary and don't add any value.
>> +
>> + a = &dattr->attr;
>> + sysfs_attr_init(a);
>> + a->name = name;
>> + a->mode = mode;
>> +
>> + return a;
>> +}
>> +
>> +static const char * const hwmon_chip_attr_templates[] = {
>> + [hwmon_chip_temp_reset_history] = "temp_reset_history",
>> + [hwmon_chip_update_interval] = "update_interval",
>> + [hwmon_chip_alarms] = "alarms",
>> +};
>> +
>> +static const char * const hwmon_temp_attr_templates[] = {
>> + [hwmon_temp_input] = "temp%d_input",
>> + [hwmon_temp_type] = "temp%d_type",
>> + [hwmon_temp_lcrit] = "temp%d_lcrit",
>> + [hwmon_temp_lcrit_hyst] = "temp%d_lcrit_hyst",
>> + [hwmon_temp_min] = "temp%d_min",
>> + [hwmon_temp_min_hyst] = "temp%d_min_hyst",
>> + [hwmon_temp_max] = "temp%d_max",
>> + [hwmon_temp_max_hyst] = "temp%d_max_hyst",
>> + [hwmon_temp_crit] = "temp%d_crit",
>> + [hwmon_temp_crit_hyst] = "temp%d_crit_hyst",
>> + [hwmon_temp_emergency] = "temp%d_emergency",
>> + [hwmon_temp_emergency_hyst] = "temp%d_emergency_hyst",
>> + [hwmon_temp_alarm] = "temp%d_alarm",
>> + [hwmon_temp_lcrit_alarm] = "temp%d_lcrit_alarm",
>> + [hwmon_temp_min_alarm] = "temp%d_min_alarm",
>> + [hwmon_temp_max_alarm] = "temp%d_max_alarm",
>> + [hwmon_temp_crit_alarm] = "temp%d_crit_alarm",
>> + [hwmon_temp_emergency_alarm] = "temp%d_emergency_alarm",
>> + [hwmon_temp_fault] = "temp%d_fault",
>> + [hwmon_temp_offset] = "temp%d_offset",
>> + [hwmon_temp_label] = "temp%d_label",
>> + [hwmon_temp_lowest] = "temp%d_lowest",
>> + [hwmon_temp_highest] = "temp%d_highest",
>> + [hwmon_temp_reset_history] = "temp%d_reset_history",
>> +};
>> +
>> +static const char * const *__templates[] = {
>> + [hwmon_chip] = hwmon_chip_attr_templates,
>> + [hwmon_temp] = hwmon_temp_attr_templates,
>> +};
>> +
>> +static const int __templates_size[] = {
>> + [hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
>> + [hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
>> +};
>> +
>> +static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
>> +{
>> + int i, n;
>> +
>> + for (i = n = 0; info->config[i]; i++)
>> + n += hweight32(info->config[i]);
>> +
>> + return n;
>> +}
>> +
>> +static int hwmon_genattrs(struct device *dev,
>> + const void *drvdata,
>> + struct attribute **attrs,
>> + const struct hwmon_ops *ops,
>> + const struct hwmon_channel_info *info)
>> +{
>> + const char * const *templates;
>> + int template_size;
>> + int i, aindex = 0;
>> +
>> + if (info->type >= ARRAY_SIZE(__templates))
>> + return -EINVAL;
>> +
>> + templates = __templates[info->type];
>> + template_size = __templates_size[info->type];
>> +
>> + for (i = 0; info->config[i]; i++) {
>> + u32 attr_mask = info->config[i];
>> + u32 attr;
>> +
>> + while (attr_mask) {
>> + struct attribute *a;
>> +
>> + attr = __ffs(attr_mask);
>> + attr_mask &= ~BIT(attr);
> loop instead using the for_each_bit_set? Tiny bit cleaner perhaps...
A bit, maybe, though not much. I changed it, but since for_each_set_bit() requires
a pointer to an unsigned long as argument, I still need attr_mask as temporary
variable (I don't want to use unsigned long for the config bit mask since
its size is not well defined). Hmm ... that also means that switching to u64
in the future will be tricky. I'll need to think about that some more.
>> + if (attr >= template_size)
>> + return -EINVAL;
>> + a = hwmon_genattr(dev, drvdata, info->type, attr, i,
>> + templates[attr], ops);
>> + if (IS_ERR(a)) {
>> + if (PTR_ERR(a) != -ENOENT)
>> + return PTR_ERR(a);
>> + continue;
>> + }
>> + attrs[aindex++] = a;
>> + }
>> + }
>> + return aindex;
>> +}
>> +
>> +static struct attribute **
>> +__hwmon_create_attrs(struct device *dev, const void *drvdata,
>> + const struct hwmon_chip_info *chip)
>> +{
>> + int ret, i, aindex = 0, nattrs = 0;
>> + struct attribute **attrs;
>> +
>> + for (i = 0; chip->info[i]; i++)
>> + nattrs += hwmon_num_channel_attrs(chip->info[i]);
>> +
>> + if (nattrs == 0)
>> + return ERR_PTR(-EINVAL);
>> +
>> + attrs = devm_kcalloc(dev, nattrs + 1, sizeof(*attrs), GFP_KERNEL);
>> + if (attrs == NULL)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + for (i = 0; chip->info[i]; i++) {
>> + ret = hwmon_genattrs(dev, drvdata, &attrs[aindex], chip->ops,
>> + chip->info[i]);
>> + if (ret < 0)
>> + return ERR_PTR(ret);
>> + aindex += ret;
>> + }
>> +
>> + return attrs;
>> +}
>> +
>> +static struct device *
>> +__hwmon_device_register(struct device *dev, const char *name, void *drvdata,
>> + const struct hwmon_chip_info *chip,
>> + const struct attribute_group **groups)
>> {
>> struct hwmon_device *hwdev;
>> - int err, id;
>> + struct device *hdev;
>> + int i, j, err, id;
>>
>> /* Do not accept invalid characters in hwmon name attribute */
>> if (name && (!strlen(name) || strpbrk(name, "-* \t\n")))
>> @@ -114,28 +392,128 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
>> goto ida_remove;
>> }
>>
>> + hdev = &hwdev->dev;
>> +
>> + if (chip && chip->ops->is_visible) {
>> + struct attribute **attrs;
>> + int ngroups = 2;
>> +
>> + if (groups)
>> + for (i = 0; groups[i]; i++)
>> + ngroups++;
>> +
>> + hwdev->groups = devm_kcalloc(dev, ngroups, sizeof(*groups),
>> + GFP_KERNEL);
>> + if (hwdev->groups == NULL)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + attrs = __hwmon_create_attrs(dev, drvdata, chip);
>> + if (IS_ERR(attrs)) {
>> + err = PTR_ERR(attrs);
>> + goto free_hwmon;
>> + }
>> +
>> + hwdev->group.attrs = attrs;
>> + ngroups = 0;
>> + hwdev->groups[ngroups++] = &hwdev->group;
>> +
>> + if (groups) {
>> + for (i = 0; groups[i]; i++)
>> + hwdev->groups[ngroups++] = groups[i];
>> + }
>> +
>> + hdev->groups = hwdev->groups;
>> + } else {
>> + hdev->groups = groups;
>> + }
>> +
>> hwdev->name = name;
>> - hwdev->dev.class = &hwmon_class;
>> - hwdev->dev.parent = dev;
>> - hwdev->dev.groups = groups;
>> - hwdev->dev.of_node = dev ? dev->of_node : NULL;
>> - dev_set_drvdata(&hwdev->dev, drvdata);
>> - dev_set_name(&hwdev->dev, HWMON_ID_FORMAT, id);
>> - err = device_register(&hwdev->dev);
>> + hdev->class = &hwmon_class;
>> + hdev->parent = dev;
>> + hdev->of_node = dev ? dev->of_node : NULL;
>> + hwdev->chip = chip;
>> + dev_set_drvdata(hdev, drvdata);
>> + dev_set_name(hdev, HWMON_ID_FORMAT, id);
>> + err = device_register(hdev);
>> if (err)
>> - goto free;
>> + goto free_hwmon;
>> +
>> + if (chip && chip->ops->is_visible && chip->ops->read &&
>> + chip->info[0]->type == hwmon_chip &&
>> + (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
>> + const struct hwmon_channel_info **info = chip->info;
>> +
>> + for (i = 1; info[i]; i++) {
>> + if (info[i]->type != hwmon_temp)
>> + continue;
>> +
>> + for (j = 0; info[i]->config[j]; j++) {
>> + if (!chip->ops->is_visible(drvdata, hwmon_temp,
>> + hwmon_temp_input, j))
>> + continue;
>> + if (info[i]->config[j] & HWMON_T_INPUT)
>> + hwmon_thermal_add_sensor(dev, hwdev, j);
>> + }
>> + }
>> + }
>>
>> - return &hwdev->dev;
>> + return hdev;
>>
>> -free:
>> +free_hwmon:
>> kfree(hwdev);
>> ida_remove:
>> ida_simple_remove(&hwmon_ida, id);
>> return ERR_PTR(err);
>> }
>> +
>> +/**
>> + * hwmon_device_register_with_groups - register w/ hwmon
>> + * @dev: the parent device
>> + * @name: hwmon name attribute
>> + * @drvdata: driver data to attach to created device
>> + * @groups: List of attribute groups to create
>> + *
>> + * hwmon_device_unregister() must be called when the device is no
>> + * longer needed.
>> + *
>> + * Returns the pointer to the new device.
>> + */
>> +struct device *
>> +hwmon_device_register_with_groups(struct device *dev, const char *name,
>> + void *drvdata,
>> + const struct attribute_group **groups)
>> +{
>> + return __hwmon_device_register(dev, name, drvdata, NULL, groups);
>> +}
>> EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
>>
>> /**
>> + * hwmon_device_register_with_info - register w/ hwmon
>> + * @dev: the parent device
>> + * @name: hwmon name attribute
>> + * @drvdata: driver data to attach to created device
>> + * @info: Pointer to hwmon chip information
>> + * @groups - pointer to list of driver specific attribute groups
>> + *
>> + * hwmon_device_unregister() must be called when the device is no
>> + * longer needed.
>> + *
>> + * Returns the pointer to the new device.
>> + */
>> +struct device *
>> +hwmon_device_register_with_info(struct device *dev, const char *name,
>> + void *drvdata,
>> + const struct hwmon_chip_info *chip,
>> + const struct attribute_group **groups)
>> +{
>> + if (chip && (!chip->ops || !chip->info))
>> + return ERR_PTR(-EINVAL);
>> +
>> + return __hwmon_device_register(dev, name, drvdata, chip, groups);
>> +}
>> +EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
>> +
>> +/**
>> * hwmon_device_register - register w/ hwmon
>> * @dev: the device to register
>> *
>> @@ -213,6 +591,47 @@ error:
>> }
>> EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
>>
>> +/**
>> + * devm_hwmon_device_register_with_info - register w/ hwmon
>> + * @dev: the parent device
>> + * @name: hwmon name attribute
>> + * @drvdata: driver data to attach to created device
>> + * @info: Pointer to hwmon chip information
>> + * @groups - pointer to list of driver specific attribute groups
>> + *
>> + * Returns the pointer to the new device. The new device is automatically
>> + * unregistered with the parent device.
>> + */
>> +struct device *
>> +devm_hwmon_device_register_with_info(struct device *dev, const char *name,
>> + void *drvdata,
>> + const struct hwmon_chip_info *chip,
>> + const struct attribute_group **groups)
>> +{
>> + struct device **ptr, *hwdev;
>> +
>> + if (!dev)
>> + return ERR_PTR(-EINVAL);
>> +
>> + ptr = devres_alloc(devm_hwmon_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
>> + groups);
>> + if (IS_ERR(hwdev))
>> + goto error;
>> +
>> + *ptr = hwdev;
>> + devres_add(dev, ptr);
> Perhaps a blank line here? (really tiny improvement in readability?
>> + return hwdev;
>> +
>> +error:
>> + devres_free(ptr);
>> + return hwdev;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_info);
>> +
>> static int devm_hwmon_match(struct device *dev, void *res, void *data)
>> {
>> struct device **hwdev = res;
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index 09354f6c1d63..99250ad092fd 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -14,9 +14,121 @@
>> #ifndef _HWMON_H_
>> #define _HWMON_H_
>>
>> +#include <linux/bitops.h>
>> +
>> struct device;
>> struct attribute_group;
>>
>> +enum hwmon_sensor_types {
>> + hwmon_chip,
>> + hwmon_temp,
>> + hwmon_in,
>> + hwmon_curr,
>> + hwmon_power,
>> + hwmon_energy,
>> +};
>> +
>> +enum hwmon_chip_attributes {
>> + hwmon_chip_temp_reset_history,
>> + hwmon_chip_register_tz,
>> + hwmon_chip_update_interval,
>> + hwmon_chip_alarms,
>> +};
>> +
>> +#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
>> +#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
>> +#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
>> +#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
>> +#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
>> +
>> +enum hwmon_temp_attributes {
>> + hwmon_temp_input = 0,
>> + hwmon_temp_type,
>> + hwmon_temp_lcrit,
>> + hwmon_temp_lcrit_hyst,
>> + hwmon_temp_min,
>> + hwmon_temp_min_hyst,
>> + hwmon_temp_max,
>> + hwmon_temp_max_hyst,
>> + hwmon_temp_crit,
>> + hwmon_temp_crit_hyst,
>> + hwmon_temp_emergency,
>> + hwmon_temp_emergency_hyst,
>> + hwmon_temp_alarm,
>> + hwmon_temp_lcrit_alarm,
>> + hwmon_temp_min_alarm,
>> + hwmon_temp_max_alarm,
>> + hwmon_temp_crit_alarm,
>> + hwmon_temp_emergency_alarm,
>> + hwmon_temp_fault,
>> + hwmon_temp_offset,
>> + hwmon_temp_label,
>> + hwmon_temp_lowest,
>> + hwmon_temp_highest,
>> + hwmon_temp_reset_history,
>> +};
>> +
>> +#define HWMON_T_INPUT BIT(hwmon_temp_input)
>> +#define HWMON_T_TYPE BIT(hwmon_temp_type)
>> +#define HWMON_T_LCRIT BIT(hwmon_temp_lcrit)
>> +#define HWMON_T_LCRIT_HYST BIT(hwmon_temp_lcrit_hyst)
>> +#define HWMON_T_MIN BIT(hwmon_temp_min)
>> +#define HWMON_T_MIN_HYST BIT(hwmon_temp_min_hyst)
>> +#define HWMON_T_MAX BIT(hwmon_temp_max)
>> +#define HWMON_T_MAX_HYST BIT(hwmon_temp_max_hyst)
>> +#define HWMON_T_CRIT BIT(hwmon_temp_crit)
>> +#define HWMON_T_CRIT_HYST BIT(hwmon_temp_crit_hyst)
>> +#define HWMON_T_EMERGENCY BIT(hwmon_temp_emergency)
>> +#define HWMON_T_EMERGENCY_HYST BIT(hwmon_temp_emergency_hyst)
>> +#define HWMON_T_MIN_ALARM BIT(hwmon_temp_min_alarm)
>> +#define HWMON_T_MAX_ALARM BIT(hwmon_temp_max_alarm)
>> +#define HWMON_T_CRIT_ALARM BIT(hwmon_temp_crit_alarm)
>> +#define HWMON_T_EMERGENCY_ALARM BIT(hwmon_temp_emergency_alarm)
>> +#define HWMON_T_FAULT BIT(hwmon_temp_fault)
>> +#define HWMON_T_OFFSET BIT(hwmon_temp_offset)
>> +#define HWMON_T_LABEL BIT(hwmon_temp_label)
>> +#define HWMON_T_LOWEST BIT(hwmon_temp_lowest)
>> +#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
>> +#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
>> +
>> +/**
>> + * struct hwmon_ops - hwmon device operations
>> + * @is_visible: Callback to return attribute visibility.
>> + * Optional.
>> + * @read: Read callback. Optional.
> I'd like to see a little more expansion in this comment on the form of the
> callbacks. What are the parameters etc? I'd expect to look here first
> to find that out rather than diving into docs.
Done.
>> + * @write: Write callback. Optional.
>> + */
>> +struct hwmon_ops {
>> + umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
>> + u32 attr, int);
>> + int (*read)(struct device *, enum hwmon_sensor_types type,
>> + u32 attr, int, long *);
>> + int (*write)(struct device *, enum hwmon_sensor_types type,
>> + u32 attr, int, long);
>> +};
>> +
>> +/**
>> + * Channel information
>> + * @type: Channel type.
>> + * @config: Pointer to NULL-terminated list of channel parameters.
>> + * Use for per-channel attributes.
>> + */
>> +struct hwmon_channel_info {
>> + enum hwmon_sensor_types type;
>> + const u32 *config;
> Hmm. Be very careful about running out of space. I did much the same
> in IIO as you know and am somewhat regretting it now a using a bitmap is
> lovely and efficient, but really annoying once you run out of bits
> if you've used a fixed (short) length..
>
> Using a bitmap as defined in bitmap.h doesn't work as there is no elegant
> way of statically defining them beyond the first long. I really wish
> there was a way of doing this but can't figure out how..
>
Me not either, which is why I did not use it.
> Maybe just go for a u64 from the start (though that then complicates the
> use of some of the bitops stuff) I suspect we'll be jumping to
> that in IIO fairly soon and it'd be a lot less painful to start there
> perhaps...
>
My solution trying to avoid that was to use separate number spaces
for the different sensor types. If/when we hit the limit (temperatures
are getting close), we would have two options: Define a new sensor type
to handle the overflow, or switch to u64. Ultimately I decided to stick
with u32 for now because u64 seemed overkill. On the other side, I made
that decision when the read/write/is_visible API to the drivers included
the bit mask, which is no longer the case. Maybe I'll try with u64 to see
its impact on code size.
>
>> +};
>> +
>> +/**
>> + * Chip configuration
>> + * @ops: Pointer to hwmon operations.
>> + * @info: Null-terminated list of channel information.
> Drop the bonus blank line.
>
> We tossed up in IIO for a long time on whether to use a null terminated list
> of our equivalent info elements, or an explict count. Ended up with count
> as it often allows sharing of the big array of channel info structures
> between different parts where one part adds more hardware over the other..
>
> I'm not saying that the gain by doing that was all that signficant, but
> I thought I'd mention the trade offs.
I went through the same thinking process. At the end, I decided to go with
the terminated list since passing it around seemed to be simpler.
The core has to walk through the list anyway, and the hwmon code does not
require big data structures to describe the channels.
> Here I think you have made the right decision.
Thanks for the feedback, and thanks a lot for your time!
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/7] hwmon: (core) Add voltage attribute support to new API
@ 2016-06-26 3:26 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/hwmon.c | 21 +++++++++++++++++++++
include/linux/hwmon.h | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 9530644ae297..d2916074befd 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -250,6 +250,7 @@ static struct attribute *hwmon_genattr(struct device *dev,
static const char * const hwmon_chip_attr_templates[] = {
[hwmon_chip_temp_reset_history] = "temp_reset_history",
+ [hwmon_chip_in_reset_history] = "in_reset_history",
[hwmon_chip_update_interval] = "update_interval",
[hwmon_chip_alarms] = "alarms",
};
@@ -281,14 +282,34 @@ static const char * const hwmon_temp_attr_templates[] = {
[hwmon_temp_reset_history] = "temp%d_reset_history",
};
+static const char * const hwmon_in_attr_templates[] = {
+ [hwmon_in_input] = "in%d_input",
+ [hwmon_in_min] = "in%d_min",
+ [hwmon_in_max] = "in%d_max",
+ [hwmon_in_lcrit] = "in%d_lcrit",
+ [hwmon_in_crit] = "in%d_crit",
+ [hwmon_in_average] = "in%d_average",
+ [hwmon_in_lowest] = "in%d_lowest",
+ [hwmon_in_highest] = "in%d_highest",
+ [hwmon_in_reset_history] = "in%d_reset_history",
+ [hwmon_in_label] = "in%d_label",
+ [hwmon_in_alarm] = "in%d_alarm",
+ [hwmon_in_min_alarm] = "in%d_min_alarm",
+ [hwmon_in_max_alarm] = "in%d_max_alarm",
+ [hwmon_in_lcrit_alarm] = "in%d_lcrit_alarm",
+ [hwmon_in_crit_alarm] = "in%d_crit_alarm",
+};
+
static const char * const *__templates[] = {
[hwmon_chip] = hwmon_chip_attr_templates,
[hwmon_temp] = hwmon_temp_attr_templates,
+ [hwmon_in] = hwmon_in_attr_templates,
};
static const int __templates_size[] = {
[hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
[hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
+ [hwmon_in] = ARRAY_SIZE(hwmon_in_attr_templates),
};
static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99250ad092fd..5e632c0fcbf4 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -30,6 +30,7 @@ enum hwmon_sensor_types {
enum hwmon_chip_attributes {
hwmon_chip_temp_reset_history,
+ hwmon_chip_in_reset_history,
hwmon_chip_register_tz,
hwmon_chip_update_interval,
hwmon_chip_alarms,
@@ -91,6 +92,40 @@ enum hwmon_temp_attributes {
#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
+enum hwmon_in_attributes {
+ hwmon_in_input,
+ hwmon_in_min,
+ hwmon_in_max,
+ hwmon_in_lcrit,
+ hwmon_in_crit,
+ hwmon_in_average,
+ hwmon_in_lowest,
+ hwmon_in_highest,
+ hwmon_in_reset_history,
+ hwmon_in_label,
+ hwmon_in_alarm,
+ hwmon_in_min_alarm,
+ hwmon_in_max_alarm,
+ hwmon_in_lcrit_alarm,
+ hwmon_in_crit_alarm,
+};
+
+#define HWMON_I_INPUT BIT(hwmon_in_input)
+#define HWMON_I_MIN BIT(hwmon_in_min)
+#define HWMON_I_MAX BIT(hwmon_in_max)
+#define HWMON_I_LCRIT BIT(hwmon_in_lcrit)
+#define HWMON_I_CRIT BIT(hwmon_in_crit)
+#define HWMON_I_AVERAGE BIT(hwmon_in_average)
+#define HWMON_I_LOWEST BIT(hwmon_in_lowest)
+#define HWMON_I_HIGHEST BIT(hwmon_in_highest)
+#define HWMON_I_RESET_HISTORY BIT(hwmon_in_reset_history)
+#define HWMON_I_LABEL BIT(hwmon_in_label)
+#define HWMON_I_ALARM BIT(hwmon_in_alarm)
+#define HWMON_I_MIN_ALARM BIT(hwmon_in_min_alarm)
+#define HWMON_I_MAX_ALARM BIT(hwmon_in_max_alarm)
+#define HWMON_I_LCRIT_ALARM BIT(hwmon_in_lcrit_alarm)
+#define HWMON_I_CRIT_ALARM BIT(hwmon_in_crit_alarm)
+
/**
* struct hwmon_ops - hwmon device operations
* @is_visible: Callback to return attribute visibility.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/7] hwmon: (core) Add voltage attribute support to new API
@ 2016-06-26 3:26 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Guenter Roeck
Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
drivers/hwmon/hwmon.c | 21 +++++++++++++++++++++
include/linux/hwmon.h | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 9530644ae297..d2916074befd 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -250,6 +250,7 @@ static struct attribute *hwmon_genattr(struct device *dev,
static const char * const hwmon_chip_attr_templates[] = {
[hwmon_chip_temp_reset_history] = "temp_reset_history",
+ [hwmon_chip_in_reset_history] = "in_reset_history",
[hwmon_chip_update_interval] = "update_interval",
[hwmon_chip_alarms] = "alarms",
};
@@ -281,14 +282,34 @@ static const char * const hwmon_temp_attr_templates[] = {
[hwmon_temp_reset_history] = "temp%d_reset_history",
};
+static const char * const hwmon_in_attr_templates[] = {
+ [hwmon_in_input] = "in%d_input",
+ [hwmon_in_min] = "in%d_min",
+ [hwmon_in_max] = "in%d_max",
+ [hwmon_in_lcrit] = "in%d_lcrit",
+ [hwmon_in_crit] = "in%d_crit",
+ [hwmon_in_average] = "in%d_average",
+ [hwmon_in_lowest] = "in%d_lowest",
+ [hwmon_in_highest] = "in%d_highest",
+ [hwmon_in_reset_history] = "in%d_reset_history",
+ [hwmon_in_label] = "in%d_label",
+ [hwmon_in_alarm] = "in%d_alarm",
+ [hwmon_in_min_alarm] = "in%d_min_alarm",
+ [hwmon_in_max_alarm] = "in%d_max_alarm",
+ [hwmon_in_lcrit_alarm] = "in%d_lcrit_alarm",
+ [hwmon_in_crit_alarm] = "in%d_crit_alarm",
+};
+
static const char * const *__templates[] = {
[hwmon_chip] = hwmon_chip_attr_templates,
[hwmon_temp] = hwmon_temp_attr_templates,
+ [hwmon_in] = hwmon_in_attr_templates,
};
static const int __templates_size[] = {
[hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
[hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
+ [hwmon_in] = ARRAY_SIZE(hwmon_in_attr_templates),
};
static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99250ad092fd..5e632c0fcbf4 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -30,6 +30,7 @@ enum hwmon_sensor_types {
enum hwmon_chip_attributes {
hwmon_chip_temp_reset_history,
+ hwmon_chip_in_reset_history,
hwmon_chip_register_tz,
hwmon_chip_update_interval,
hwmon_chip_alarms,
@@ -91,6 +92,40 @@ enum hwmon_temp_attributes {
#define HWMON_T_HIGHEST BIT(hwmon_temp_highest)
#define HWMON_T_RESET_HISTORY BIT(hwmon_temp_reset_history)
+enum hwmon_in_attributes {
+ hwmon_in_input,
+ hwmon_in_min,
+ hwmon_in_max,
+ hwmon_in_lcrit,
+ hwmon_in_crit,
+ hwmon_in_average,
+ hwmon_in_lowest,
+ hwmon_in_highest,
+ hwmon_in_reset_history,
+ hwmon_in_label,
+ hwmon_in_alarm,
+ hwmon_in_min_alarm,
+ hwmon_in_max_alarm,
+ hwmon_in_lcrit_alarm,
+ hwmon_in_crit_alarm,
+};
+
+#define HWMON_I_INPUT BIT(hwmon_in_input)
+#define HWMON_I_MIN BIT(hwmon_in_min)
+#define HWMON_I_MAX BIT(hwmon_in_max)
+#define HWMON_I_LCRIT BIT(hwmon_in_lcrit)
+#define HWMON_I_CRIT BIT(hwmon_in_crit)
+#define HWMON_I_AVERAGE BIT(hwmon_in_average)
+#define HWMON_I_LOWEST BIT(hwmon_in_lowest)
+#define HWMON_I_HIGHEST BIT(hwmon_in_highest)
+#define HWMON_I_RESET_HISTORY BIT(hwmon_in_reset_history)
+#define HWMON_I_LABEL BIT(hwmon_in_label)
+#define HWMON_I_ALARM BIT(hwmon_in_alarm)
+#define HWMON_I_MIN_ALARM BIT(hwmon_in_min_alarm)
+#define HWMON_I_MAX_ALARM BIT(hwmon_in_max_alarm)
+#define HWMON_I_LCRIT_ALARM BIT(hwmon_in_lcrit_alarm)
+#define HWMON_I_CRIT_ALARM BIT(hwmon_in_crit_alarm)
+
/**
* struct hwmon_ops - hwmon device operations
* @is_visible: Callback to return attribute visibility.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/7] hwmon: (core) Add current attribute support to new API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
2016-06-26 3:26 ` Guenter Roeck
2016-06-26 3:26 ` Guenter Roeck
@ 2016-06-26 3:26 ` Guenter Roeck
2016-06-26 3:26 ` [PATCH 4/7] hwmon: (core) Add power " Guenter Roeck
` (5 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/hwmon.c | 21 +++++++++++++++++++++
include/linux/hwmon.h | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index d2916074befd..8c3c7ffe828b 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -251,6 +251,7 @@ static struct attribute *hwmon_genattr(struct device *dev,
static const char * const hwmon_chip_attr_templates[] = {
[hwmon_chip_temp_reset_history] = "temp_reset_history",
[hwmon_chip_in_reset_history] = "in_reset_history",
+ [hwmon_chip_curr_reset_history] = "curr_reset_history",
[hwmon_chip_update_interval] = "update_interval",
[hwmon_chip_alarms] = "alarms",
};
@@ -300,16 +301,36 @@ static const char * const hwmon_in_attr_templates[] = {
[hwmon_in_crit_alarm] = "in%d_crit_alarm",
};
+static const char * const hwmon_curr_attr_templates[] = {
+ [hwmon_curr_input] = "curr%d_input",
+ [hwmon_curr_min] = "curr%d_min",
+ [hwmon_curr_max] = "curr%d_max",
+ [hwmon_curr_lcrit] = "curr%d_lcrit",
+ [hwmon_curr_crit] = "curr%d_crit",
+ [hwmon_curr_average] = "curr%d_average",
+ [hwmon_curr_lowest] = "curr%d_lowest",
+ [hwmon_curr_highest] = "curr%d_highest",
+ [hwmon_curr_reset_history] = "curr%d_reset_history",
+ [hwmon_curr_label] = "curr%d_label",
+ [hwmon_curr_alarm] = "curr%d_alarm",
+ [hwmon_curr_min_alarm] = "curr%d_min_alarm",
+ [hwmon_curr_max_alarm] = "curr%d_max_alarm",
+ [hwmon_curr_lcrit_alarm] = "curr%d_lcrit_alarm",
+ [hwmon_curr_crit_alarm] = "curr%d_crit_alarm",
+};
+
static const char * const *__templates[] = {
[hwmon_chip] = hwmon_chip_attr_templates,
[hwmon_temp] = hwmon_temp_attr_templates,
[hwmon_in] = hwmon_in_attr_templates,
+ [hwmon_curr] = hwmon_curr_attr_templates,
};
static const int __templates_size[] = {
[hwmon_chip] = ARRAY_SIZE(hwmon_chip_attr_templates),
[hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
[hwmon_in] = ARRAY_SIZE(hwmon_in_attr_templates),
+ [hwmon_curr] = ARRAY_SIZE(hwmon_curr_attr_templates),
};
static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 5e632c0fcbf4..bac3c5dc672f 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -31,6 +31,7 @@ enum hwmon_sensor_types {
enum hwmon_chip_attributes {
hwmon_chip_temp_reset_history,
hwmon_chip_in_reset_history,
+ hwmon_chip_curr_reset_history,
hwmon_chip_register_tz,
hwmon_chip_update_interval,
hwmon_chip_alarms,
@@ -38,6 +39,7 @@ enum hwmon_chip_attributes {
#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
+#define HWMON_C_CURR_RESET_HISTORY BIT(hwmon_chip_curr_reset_history)
#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
@@ -126,6 +128,40 @@ enum hwmon_in_attributes {
#define HWMON_I_LCRIT_ALARM BIT(hwmon_in_lcrit_alarm)
#define HWMON_I_CRIT_ALARM BIT(hwmon_in_crit_alarm)
+enum hwmon_curr_attributes {
+ hwmon_curr_input,
+ hwmon_curr_min,
+ hwmon_curr_max,
+ hwmon_curr_lcrit,
+ hwmon_curr_crit,
+ hwmon_curr_average,
+ hwmon_curr_lowest,
+ hwmon_curr_highest,
+ hwmon_curr_reset_history,
+ hwmon_curr_label,
+ hwmon_curr_alarm,
+ hwmon_curr_min_alarm,
+ hwmon_curr_max_alarm,
+ hwmon_curr_lcrit_alarm,
+ hwmon_curr_crit_alarm,
+};
+
+#define HWMON_C_INPUT BIT(hwmon_curr_input)
+#define HWMON_C_MIN BIT(hwmon_curr_min)
+#define HWMON_C_MAX BIT(hwmon_curr_max)
+#define HWMON_C_LCRIT BIT(hwmon_curr_lcrit)
+#define HWMON_C_CRIT BIT(hwmon_curr_crit)
+#define HWMON_C_AVERAGE BIT(hwmon_curr_average)
+#define HWMON_C_LOWEST BIT(hwmon_curr_lowest)
+#define HWMON_C_HIGHEST BIT(hwmon_curr_highest)
+#define HWMON_C_RESET_HISTORY BIT(hwmon_curr_reset_history)
+#define HWMON_C_LABEL BIT(hwmon_curr_label)
+#define HWMON_C_ALARM BIT(hwmon_curr_alarm)
+#define HWMON_C_MIN_ALARM BIT(hwmon_curr_min_alarm)
+#define HWMON_C_MAX_ALARM BIT(hwmon_curr_max_alarm)
+#define HWMON_C_LCRIT_ALARM BIT(hwmon_curr_lcrit_alarm)
+#define HWMON_C_CRIT_ALARM BIT(hwmon_curr_crit_alarm)
+
/**
* struct hwmon_ops - hwmon device operations
* @is_visible: Callback to return attribute visibility.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/7] hwmon: (core) Add power attribute support to new API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
` (2 preceding siblings ...)
2016-06-26 3:26 ` [PATCH 3/7] hwmon: (core) Add current " Guenter Roeck
@ 2016-06-26 3:26 ` Guenter Roeck
2016-06-26 3:26 ` [PATCH 5/7] hwmon: (core) Add energy and humidity " Guenter Roeck
` (4 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/hwmon.c | 30 ++++++++++++++++++++++++++++
include/linux/hwmon.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 8c3c7ffe828b..dff07e8f9509 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -252,6 +252,7 @@ static const char * const hwmon_chip_attr_templates[] = {
[hwmon_chip_temp_reset_history] = "temp_reset_history",
[hwmon_chip_in_reset_history] = "in_reset_history",
[hwmon_chip_curr_reset_history] = "curr_reset_history",
+ [hwmon_chip_power_reset_history] = "power_reset_history",
[hwmon_chip_update_interval] = "update_interval",
[hwmon_chip_alarms] = "alarms",
};
@@ -319,11 +320,39 @@ static const char * const hwmon_curr_attr_templates[] = {
[hwmon_curr_crit_alarm] = "curr%d_crit_alarm",
};
+static const char * const hwmon_power_attr_templates[] = {
+ [hwmon_power_average] = "power%d_average",
+ [hwmon_power_average_interval] = "power%d_average_interval",
+ [hwmon_power_average_interval_max] = "power%d_interval_max",
+ [hwmon_power_average_interval_min] = "power%d_interval_min",
+ [hwmon_power_average_highest] = "power%d_average_highest",
+ [hwmon_power_average_lowest] = "power%d_average_lowest",
+ [hwmon_power_average_max] = "power%d_average_max",
+ [hwmon_power_average_min] = "power%d_average_min",
+ [hwmon_power_input] = "power%d_input",
+ [hwmon_power_input_highest] = "power%d_input_highest",
+ [hwmon_power_input_lowest] = "power%d_input_lowest",
+ [hwmon_power_reset_history] = "power%d_reset_history",
+ [hwmon_power_accuracy] = "power%d_accuracy",
+ [hwmon_power_cap] = "power%d_cap",
+ [hwmon_power_cap_hyst] = "power%d_cap_hyst",
+ [hwmon_power_cap_max] = "power%d_cap_max",
+ [hwmon_power_cap_min] = "power%d_cap_min",
+ [hwmon_power_max] = "power%d_max",
+ [hwmon_power_crit] = "power%d_crit",
+ [hwmon_power_label] = "power%d_label",
+ [hwmon_power_alarm] = "power%d_alarm",
+ [hwmon_power_cap_alarm] = "power%d_cap_alarm",
+ [hwmon_power_max_alarm] = "power%d_max_alarm",
+ [hwmon_power_crit_alarm] = "power%d_crit_alarm",
+};
+
static const char * const *__templates[] = {
[hwmon_chip] = hwmon_chip_attr_templates,
[hwmon_temp] = hwmon_temp_attr_templates,
[hwmon_in] = hwmon_in_attr_templates,
[hwmon_curr] = hwmon_curr_attr_templates,
+ [hwmon_power] = hwmon_power_attr_templates,
};
static const int __templates_size[] = {
@@ -331,6 +360,7 @@ static const int __templates_size[] = {
[hwmon_temp] = ARRAY_SIZE(hwmon_temp_attr_templates),
[hwmon_in] = ARRAY_SIZE(hwmon_in_attr_templates),
[hwmon_curr] = ARRAY_SIZE(hwmon_curr_attr_templates),
+ [hwmon_power] = ARRAY_SIZE(hwmon_power_attr_templates),
};
static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index bac3c5dc672f..4de6e2af29ea 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -32,6 +32,7 @@ enum hwmon_chip_attributes {
hwmon_chip_temp_reset_history,
hwmon_chip_in_reset_history,
hwmon_chip_curr_reset_history,
+ hwmon_chip_power_reset_history,
hwmon_chip_register_tz,
hwmon_chip_update_interval,
hwmon_chip_alarms,
@@ -40,6 +41,7 @@ enum hwmon_chip_attributes {
#define HWMON_C_TEMP_RESET_HISTORY BIT(hwmon_chip_temp_reset_history)
#define HWMON_C_IN_RESET_HISTORY BIT(hwmon_chip_in_reset_history)
#define HWMON_C_CURR_RESET_HISTORY BIT(hwmon_chip_curr_reset_history)
+#define HWMON_C_POWER_RESET_HISTORY BIT(hwmon_chip_power_reset_history)
#define HWMON_C_REGISTER_TZ BIT(hwmon_chip_register_tz)
#define HWMON_C_UPDATE_INTERVAL BIT(hwmon_chip_update_interval)
#define HWMON_C_ALARMS BIT(hwmon_chip_alarms)
@@ -162,6 +164,58 @@ enum hwmon_curr_attributes {
#define HWMON_C_LCRIT_ALARM BIT(hwmon_curr_lcrit_alarm)
#define HWMON_C_CRIT_ALARM BIT(hwmon_curr_crit_alarm)
+enum hwmon_power_attributes {
+ hwmon_power_average,
+ hwmon_power_average_interval,
+ hwmon_power_average_interval_max,
+ hwmon_power_average_interval_min,
+ hwmon_power_average_highest,
+ hwmon_power_average_lowest,
+ hwmon_power_average_max,
+ hwmon_power_average_min,
+ hwmon_power_input,
+ hwmon_power_input_highest,
+ hwmon_power_input_lowest,
+ hwmon_power_reset_history,
+ hwmon_power_accuracy,
+ hwmon_power_cap,
+ hwmon_power_cap_hyst,
+ hwmon_power_cap_max,
+ hwmon_power_cap_min,
+ hwmon_power_max,
+ hwmon_power_crit,
+ hwmon_power_label,
+ hwmon_power_alarm,
+ hwmon_power_cap_alarm,
+ hwmon_power_max_alarm,
+ hwmon_power_crit_alarm,
+};
+
+#define HWMON_P_AVERAGE BIT(hwmon_power_average)
+#define HWMON_P_AVERAGE_INTERVAL BIT(hwmon_power_average_interval)
+#define HWMON_P_AVERAGE_INTERVAL_MAX BIT(hwmon_power_average_interval_max)
+#define HWMON_P_AVERAGE_INTERVAL_MIN BIT(hwmon_power_average_interval_min)
+#define HWMON_P_AVERAGE_HIGHEST BIT(hwmon_power_average_highest)
+#define HWMON_P_AVERAGE_LOWEST BIT(hwmon_power_average_lowest)
+#define HWMON_P_AVERAGE_MAX BIT(hwmon_power_average_max)
+#define HWMON_P_AVERAGE_MIN BIT(hwmon_power_average_min)
+#define HWMON_P_INPUT BIT(hwmon_power_input)
+#define HWMON_P_INPUT_HIGHEST BIT(hwmon_power_input_highest)
+#define HWMON_P_INPUT_LOWEST BIT(hwmon_power_input_lowest)
+#define HWMON_P_RESET_HISTORY BIT(hwmon_power_reset_history)
+#define HWMON_P_ACCURACY BIT(hwmon_power_accuracy)
+#define HWMON_P_CAP BIT(hwmon_power_cap)
+#define HWMON_P_CAP_HYST BIT(hwmon_power_cap_hyst)
+#define HWMON_P_CAP_MAX BIT(hwmon_power_cap_max)
+#define HWMON_P_CAP_MIN BIT(hwmon_power_cap_min)
+#define HWMON_P_MAX BIT(hwmon_power_max)
+#define HWMON_P_CRIT BIT(hwmon_power_crit)
+#define HWMON_P_LABEL BIT(hwmon_power_label)
+#define HWMON_P_ALARM BIT(hwmon_power_alarm)
+#define HWMON_P_CAP_ALARM BIT(hwmon_power_cap_alarm)
+#define HWMON_P_MAX_ALARM BIT(hwmon_power_max_alarm)
+#define HWMON_P_CRIT_ALARM BIT(hwmon_power_crit_alarm)
+
/**
* struct hwmon_ops - hwmon device operations
* @is_visible: Callback to return attribute visibility.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/7] hwmon: (core) Add energy and humidity attribute support to new API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
` (3 preceding siblings ...)
2016-06-26 3:26 ` [PATCH 4/7] hwmon: (core) Add power " Guenter Roeck
@ 2016-06-26 3:26 ` Guenter Roeck
2016-06-26 3:26 ` [PATCH 6/7] hwmon: (core) Add fan " Guenter Roeck
` (3 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/hwmon.c | 20 ++++++++++++++++++++
include/linux/hwmon.h | 29 +++++++++++++++++++++++++++++
2 files changed, 49 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index dff07e8f9509..363ec5660d0c 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -347,12 +347,30 @@ static const char * const hwmon_power_attr_templates[] = {
[hwmon_power_crit_alarm] = "power%d_crit_alarm",
};
+static const char * const hwmon_energy_attr_templates[] = {
+ [hwmon_energy_input] = "energy%d_input",
+ [hwmon_energy_label] = "energy%d_label",
+};
+
+static const char * const hwmon_humidity_attr_templates[] = {
+ [hwmon_humidity_input] = "humidity%d_input",
+ [hwmon_humidity_label] = "humidity%d_label",
+ [hwmon_humidity_min] = "humidity%d_min",
+ [hwmon_humidity_min_hyst] = "humidity%d_min_hyst",
+ [hwmon_humidity_max] = "humidity%d_max",
+ [hwmon_humidity_max_hyst] = "humidity%d_max_hyst",
+ [hwmon_humidity_alarm] = "humidity%d_alarm",
+ [hwmon_humidity_fault] = "humidity%d_fault",
+};
+
static const char * const *__templates[] = {
[hwmon_chip] = hwmon_chip_attr_templates,
[hwmon_temp] = hwmon_temp_attr_templates,
[hwmon_in] = hwmon_in_attr_templates,
[hwmon_curr] = hwmon_curr_attr_templates,
[hwmon_power] = hwmon_power_attr_templates,
+ [hwmon_energy] = hwmon_energy_attr_templates,
+ [hwmon_humidity] = hwmon_humidity_attr_templates,
};
static const int __templates_size[] = {
@@ -361,6 +379,8 @@ static const int __templates_size[] = {
[hwmon_in] = ARRAY_SIZE(hwmon_in_attr_templates),
[hwmon_curr] = ARRAY_SIZE(hwmon_curr_attr_templates),
[hwmon_power] = ARRAY_SIZE(hwmon_power_attr_templates),
+ [hwmon_energy] = ARRAY_SIZE(hwmon_energy_attr_templates),
+ [hwmon_humidity] = ARRAY_SIZE(hwmon_humidity_attr_templates),
};
static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 4de6e2af29ea..a0b82027ae07 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -26,6 +26,7 @@ enum hwmon_sensor_types {
hwmon_curr,
hwmon_power,
hwmon_energy,
+ hwmon_humidity,
};
enum hwmon_chip_attributes {
@@ -216,6 +217,34 @@ enum hwmon_power_attributes {
#define HWMON_P_MAX_ALARM BIT(hwmon_power_max_alarm)
#define HWMON_P_CRIT_ALARM BIT(hwmon_power_crit_alarm)
+enum hwmon_energy_attributes {
+ hwmon_energy_input,
+ hwmon_energy_label,
+};
+
+#define HWMON_E_INPUT BIT(hwmon_energy_input)
+#define HWMON_E_LABEL BIT(hwmon_energy_label)
+
+enum hwmon_humidity_attributes {
+ hwmon_humidity_input,
+ hwmon_humidity_label,
+ hwmon_humidity_min,
+ hwmon_humidity_min_hyst,
+ hwmon_humidity_max,
+ hwmon_humidity_max_hyst,
+ hwmon_humidity_alarm,
+ hwmon_humidity_fault,
+};
+
+#define HWMON_H_INPUT BIT(hwmon_humidity_input)
+#define HWMON_H_LABEL BIT(hwmon_humidity_label)
+#define HWMON_H_MIN BIT(hwmon_humidity_min)
+#define HWMON_H_MIN_HYST BIT(hwmon_humidity_min_hyst)
+#define HWMON_H_MAX BIT(hwmon_humidity_max)
+#define HWMON_H_MAX_HYST BIT(hwmon_humidity_max_hyst)
+#define HWMON_H_ALARM BIT(hwmon_humidity_alarm)
+#define HWMON_H_FAULT BIT(hwmon_humidity_fault)
+
/**
* struct hwmon_ops - hwmon device operations
* @is_visible: Callback to return attribute visibility.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/7] hwmon: (core) Add fan attribute support to new API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
` (4 preceding siblings ...)
2016-06-26 3:26 ` [PATCH 5/7] hwmon: (core) Add energy and humidity " Guenter Roeck
@ 2016-06-26 3:26 ` Guenter Roeck
2016-06-26 3:26 ` [PATCH 7/7] hwmon: (core) Document new kernel API Guenter Roeck
` (2 subsequent siblings)
8 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/hwmon/hwmon.c | 16 ++++++++++++++++
include/linux/hwmon.h | 27 +++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 363ec5660d0c..ca18a6f4f9bc 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -363,6 +363,20 @@ static const char * const hwmon_humidity_attr_templates[] = {
[hwmon_humidity_fault] = "humidity%d_fault",
};
+static const char * const hwmon_fan_attr_templates[] = {
+ [hwmon_fan_input] = "fan%d_input",
+ [hwmon_fan_label] = "fan%d_label",
+ [hwmon_fan_min] = "fan%d_min",
+ [hwmon_fan_max] = "fan%d_max",
+ [hwmon_fan_div] = "fan%d_div",
+ [hwmon_fan_pulses] = "fan%d_pulses",
+ [hwmon_fan_target] = "fan%d_target",
+ [hwmon_fan_alarm] = "fan%d_alarm",
+ [hwmon_fan_min_alarm] = "fan%d_min_alarm",
+ [hwmon_fan_max_alarm] = "fan%d_max_alarm",
+ [hwmon_fan_fault] = "fan%d_fault",
+};
+
static const char * const *__templates[] = {
[hwmon_chip] = hwmon_chip_attr_templates,
[hwmon_temp] = hwmon_temp_attr_templates,
@@ -371,6 +385,7 @@ static const char * const *__templates[] = {
[hwmon_power] = hwmon_power_attr_templates,
[hwmon_energy] = hwmon_energy_attr_templates,
[hwmon_humidity] = hwmon_humidity_attr_templates,
+ [hwmon_fan] = hwmon_fan_attr_templates,
};
static const int __templates_size[] = {
@@ -381,6 +396,7 @@ static const int __templates_size[] = {
[hwmon_power] = ARRAY_SIZE(hwmon_power_attr_templates),
[hwmon_energy] = ARRAY_SIZE(hwmon_energy_attr_templates),
[hwmon_humidity] = ARRAY_SIZE(hwmon_humidity_attr_templates),
+ [hwmon_fan] = ARRAY_SIZE(hwmon_fan_attr_templates),
};
static int hwmon_num_channel_attrs(const struct hwmon_channel_info *info)
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index a0b82027ae07..b2fdaaa7b7ba 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -27,6 +27,7 @@ enum hwmon_sensor_types {
hwmon_power,
hwmon_energy,
hwmon_humidity,
+ hwmon_fan,
};
enum hwmon_chip_attributes {
@@ -245,6 +246,32 @@ enum hwmon_humidity_attributes {
#define HWMON_H_ALARM BIT(hwmon_humidity_alarm)
#define HWMON_H_FAULT BIT(hwmon_humidity_fault)
+enum hwmon_fan_attributes {
+ hwmon_fan_input,
+ hwmon_fan_label,
+ hwmon_fan_min,
+ hwmon_fan_max,
+ hwmon_fan_div,
+ hwmon_fan_pulses,
+ hwmon_fan_target,
+ hwmon_fan_alarm,
+ hwmon_fan_min_alarm,
+ hwmon_fan_max_alarm,
+ hwmon_fan_fault,
+};
+
+#define HWMON_F_INPUT BIT(hwmon_fan_input)
+#define HWMON_F_LABEL BIT(hwmon_fan_label)
+#define HWMON_F_MIN BIT(hwmon_fan_min)
+#define HWMON_F_MAX BIT(hwmon_fan_max)
+#define HWMON_F_DIV BIT(hwmon_fan_div)
+#define HWMON_F_PULSES BIT(hwmon_fan_pulses)
+#define HWMON_F_TARGET BIT(hwmon_fan_target)
+#define HWMON_F_ALARM BIT(hwmon_fan_alarm)
+#define HWMON_F_MIN_ALARM BIT(hwmon_fan_min_alarm)
+#define HWMON_F_MAX_ALARM BIT(hwmon_fan_max_alarm)
+#define HWMON_F_FAULT BIT(hwmon_fan_fault)
+
/**
* struct hwmon_ops - hwmon device operations
* @is_visible: Callback to return attribute visibility.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 7/7] hwmon: (core) Document new kernel API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
` (5 preceding siblings ...)
2016-06-26 3:26 ` [PATCH 6/7] hwmon: (core) Add fan " Guenter Roeck
@ 2016-06-26 3:26 ` Guenter Roeck
2016-07-10 15:56 ` Jonathan Cameron
2016-07-08 9:31 ` [PATCH 0/5] hwmon: New hwmon registration API Punit Agrawal
2016-07-10 16:00 ` Jonathan Cameron
8 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2016-06-26 3:26 UTC (permalink / raw)
To: Jean Delvare
Cc: Jonathan Cameron, Zhang Rui, Eduardo Valentin, linux-pm,
linux-iio, linux-hwmon, linux-kernel, Guenter Roeck
Describe the new registration API function as well as the data
structures it requires.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Documentation/hwmon/hwmon-kernel-api.txt | 229 ++++++++++++++++++++++++++++++-
1 file changed, 227 insertions(+), 2 deletions(-)
diff --git a/Documentation/hwmon/hwmon-kernel-api.txt b/Documentation/hwmon/hwmon-kernel-api.txt
index 2ecdbfc85ecf..1a96c36532f2 100644
--- a/Documentation/hwmon/hwmon-kernel-api.txt
+++ b/Documentation/hwmon/hwmon-kernel-api.txt
@@ -34,6 +34,19 @@ devm_hwmon_device_register_with_groups(struct device *dev,
const char *name, void *drvdata,
const struct attribute_group **groups);
+struct device *
+hwmon_device_register_with_info(struct device *dev,
+ const char *name, void *drvdata,
+ const struct hwmon_chip_info *info,
+ const struct attribute_group **groups);
+
+struct device *
+devm_hwmon_device_register_with_info(struct device *dev,
+ const char *name,
+ void *drvdata,
+ const struct hwmon_chip_info *info,
+ const struct attribute_group **groups);
+
void hwmon_device_unregister(struct device *dev);
void devm_hwmon_device_unregister(struct device *dev);
@@ -60,15 +73,227 @@ devm_hwmon_device_register_with_groups is similar to
hwmon_device_register_with_groups. However, it is device managed, meaning the
hwmon device does not have to be removed explicitly by the removal function.
+hwmon_device_register_with_info is the most comprehensive and preferred means
+to register a hardware monitoring device. It creates the standard sysfs
+attributes in the hardware monitoring core, letting the driver focus on reading
+from and writing to the chip instead of having to bother with sysfs attributes.
+Its parameters are described in more detail below.
+
+devm_hwmon_device_register_with_info is similar to
+hwmon_device_register_with_info. However, it is device managed, meaning the
+hwmon device does not have to be removed explicitly by the removal function.
+
hwmon_device_unregister deregisters a registered hardware monitoring device.
The parameter of this function is the pointer to the registered hardware
monitoring device structure. This function must be called from the driver
remove function if the hardware monitoring device was registered with
-hwmon_device_register or with hwmon_device_register_with_groups.
+hwmon_device_register, hwmon_device_register_with_groups, or
+hwmon_device_register_with_info.
devm_hwmon_device_unregister does not normally have to be called. It is only
needed for error handling, and only needed if the driver probe fails after
-the call to devm_hwmon_device_register_with_groups.
+the call to devm_hwmon_device_register_with_groups and if the automatic
+(device managed) removal would be too late.
+
+Using devm_hwmon_device_register_with_info()
+--------------------------------------------
+
+hwmon_device_register_with_info() registers a hardware monitoring device.
+The parameters to this function are
+
+struct device *dev Pointer to parent device
+const char *name Device name
+void *drvdata Driver private data
+const struct hwmon_chip_info *info
+ Pointer to chip description.
+const struct attribute_group **groups
+ Null-terminated list of additional sysfs attribute
+ groups.
+
+This function returns a pointer to the created hardware monitoring device
+on success and a negative error code for failure.
+
+The hwmon_chip_info structure looks as follows.
+
+struct hwmon_chip_info {
+ const struct hwmon_ops *ops;
+ const struct hwmon_channel_info **info;
+};
+
+It contains the following fields:
+
+* ops: Pointer to device operations.
+* info: NULL-terminated list of device channel descriptors.
+
+The list of hwmon operations is defined as:
+
+struct hwmon_ops {
+ umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
+ u32 attr, int);
+ int (*read)(struct device *, enum hwmon_sensor_types type,
+ u32 attr, int, long *);
+ int (*write)(struct device *, enum hwmon_sensor_types type,
+ u32 attr, int, long);
+};
+
+It defines the following operations.
+
+* is_visible: Pointer to a function to return the file mode for each supported
+ attribute. This function is mandatory.
+
+* read: Pointer to a function for reading a value from the chip. This function
+ is optional, but must be provided if any readable attributes exist.
+
+* write: Pointer to a function for writing a value to the chip. This function is
+ optional, but must be priovided if any writeable attributes exist.
+
+Each sensor channel is described with struct hwmon_channel_info, which is
+defined as follows.
+
+struct hwmon_channel_info {
+ enum hwmon_sensor_types type;
+ u32 *config;
+};
+
+It contains following fields:
+
+* type: The hardware monitoring sensor type.
+ Supported sensor types are
+ * hwmon_chip A virtual sensor type, used to describe attributes
+ which apply to the entire chip.
+ * hwmon_temp Temperature sensor
+ * hwmon_in Voltage sensor
+ * hwmon_curr Current sensor
+ * hwmon_power Power sensor
+ * hwmon_energy Energy sensor
+ * hwmon_humidity Humidity sensor
+ * hwmon_fan Fan speed sensor
+
+* config: Pointer to a 0-terminated list of configuration values for each
+ sensor of the given type. Each value is a combination of bit values
+ describing the attributes supposed by a single sensor.
+
+As an example, here is the complete description file for a LM75 compatible
+sensor chip. The chip has a single temperature sensor. The driver wants to
+register with the thermal subsystem (HWMON_C_REGISTER_TZ), and it supports
+the update_interval attribute (HWMON_C_UPDATE_INTERVAL). The chip supports
+reading the temperature (HWMON_T_INPUT), it has a maximum temperature
+register (HWMON_T_MAX) as well as a maximum temperature hysteresis register
+(HWMON_T_MAX_HYST).
+
+static const u32 lm75_chip_config[] = {
+ HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL,
+ 0
+};
+
+static const struct hwmon_channel_info lm75_chip = {
+ .type = hwmon_chip,
+ .config = lm75_chip_config,
+};
+
+static const u32 lm75_temp_config[] = {
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST,
+ 0
+};
+
+static const struct hwmon_channel_info lm75_temp = {
+ .type = hwmon_temp,
+ .config = lm75_temp_config,
+};
+
+static const struct hwmon_channel_info *lm75_info[] = {
+ &lm75_chip,
+ &lm75_temp,
+ NULL
+};
+
+static const struct hwmon_ops lm75_hwmon_ops = {
+ .is_visible = lm75_is_visible,
+ .read = lm75_read,
+ .write = lm75_write,
+};
+
+static const struct hwmon_chip_info lm75_chip_info = {
+ .ops = &lm75_hwmon_ops,
+ .info = lm75_info,
+};
+
+A complete list of bit values indicating individual attribute support
+is defined in include/linux/hwmon.h. Definition prefixes are as follows.
+
+HWMON_C_xxxx Chip attributes, for use with hwmon_chip.
+HWMON_T_xxxx Temperature attributes, for use with hwmon_temp.
+HWMON_I_xxxx Voltage attributes, for use with hwmon_in.
+HWMON_C_xxxx Current attributes, for use with hwmon_curr.
+ Notice the prefix overlap with chip attributes.
+HWMON_P_xxxx Power attributes, for use with hwmon_power.
+HWMON_E_xxxx Energy attributes, for use with hwmon_energy.
+HWMON_H_xxxx Humidity attributes, for use with hwmon_humidity.
+HWMON_F_xxxx Fan speed attributes, for use with hwmon_fan.
+
+Driver callback functions
+-------------------------
+
+Each driver provides is_visible, read, and write functions. Parameters
+and return values for those functions are as follows.
+
+umode_t is_visible_func(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+
+Parameters:
+ data: Pointer to device private data structure.
+ type: The sensor type.
+ attr: Attribute identifier associated with a specific attribute.
+ For example, the attribute value for HWMON_T_INPUT would be
+ hwmon_temp_input. For complete mappings of bit fields to
+ attribute values please see include/linux/hwmon.h.
+ channel:The sensor channel number.
+
+Return value:
+ The file mode for this attribute. Typically, this will be 0 (the
+ attribute will not be created), S_IRUGO, or 'S_IRUGO | S_IWUSR'.
+
+int read_func(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+
+Parameters:
+ dev: Pointer to the hardware monitoring device.
+ type: The sensor type.
+ attr: Attribute identifier associated with a specific attribute.
+ For example, the attribute value for HWMON_T_INPUT would be
+ hwmon_temp_input. For complete mappings please see
+ include/linux/hwmon.h.
+ channel:The sensor channel number.
+ val: Pointer to attribute value.
+
+Return value:
+ 0 on success, a negative error number otherwise.
+
+int write_func(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+
+Parameters:
+ dev: Pointer to the hardware monitoring device.
+ type: The sensor type.
+ attr: Attribute identifier associated with a specific attribute.
+ For example, the attribute value for HWMON_T_INPUT would be
+ hwmon_temp_input. For complete mappings please see
+ include/linux/hwmon.h.
+ channel:The sensor channel number.
+ val: The value to write to the chip.
+
+Return value:
+ 0 on success, a negative error number otherwise.
+
+
+Driver-provided sysfs attributes
+--------------------------------
+
+If the hardware monitoring device is registered with
+hwmon_device_register_with_info or devm_hwmon_device_register_with_info,
+it is most likely not necessary to provide sysfs atributes. Only non-standard
+sysfs attributes need to be provided when one of those registration functions
+is used.
The header file linux/hwmon-sysfs.h provides a number of useful macros to
declare and use hardware monitoring sysfs attributes.
--
2.5.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 7/7] hwmon: (core) Document new kernel API
2016-06-26 3:26 ` [PATCH 7/7] hwmon: (core) Document new kernel API Guenter Roeck
@ 2016-07-10 15:56 ` Jonathan Cameron
0 siblings, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2016-07-10 15:56 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm, linux-iio, linux-hwmon,
linux-kernel
On 26/06/16 04:26, Guenter Roeck wrote:
> Describe the new registration API function as well as the data
> structures it requires.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Nice docs. Couple of typos inline.
Jonathan
> ---
> Documentation/hwmon/hwmon-kernel-api.txt | 229 ++++++++++++++++++++++++++++++-
> 1 file changed, 227 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/hwmon/hwmon-kernel-api.txt b/Documentation/hwmon/hwmon-kernel-api.txt
> index 2ecdbfc85ecf..1a96c36532f2 100644
> --- a/Documentation/hwmon/hwmon-kernel-api.txt
> +++ b/Documentation/hwmon/hwmon-kernel-api.txt
> @@ -34,6 +34,19 @@ devm_hwmon_device_register_with_groups(struct device *dev,
> const char *name, void *drvdata,
> const struct attribute_group **groups);
>
> +struct device *
> +hwmon_device_register_with_info(struct device *dev,
> + const char *name, void *drvdata,
> + const struct hwmon_chip_info *info,
> + const struct attribute_group **groups);
> +
> +struct device *
> +devm_hwmon_device_register_with_info(struct device *dev,
> + const char *name,
> + void *drvdata,
> + const struct hwmon_chip_info *info,
> + const struct attribute_group **groups);
> +
> void hwmon_device_unregister(struct device *dev);
> void devm_hwmon_device_unregister(struct device *dev);
>
> @@ -60,15 +73,227 @@ devm_hwmon_device_register_with_groups is similar to
> hwmon_device_register_with_groups. However, it is device managed, meaning the
> hwmon device does not have to be removed explicitly by the removal function.
>
> +hwmon_device_register_with_info is the most comprehensive and preferred means
> +to register a hardware monitoring device. It creates the standard sysfs
> +attributes in the hardware monitoring core, letting the driver focus on reading
> +from and writing to the chip instead of having to bother with sysfs attributes.
> +Its parameters are described in more detail below.
> +
> +devm_hwmon_device_register_with_info is similar to
> +hwmon_device_register_with_info. However, it is device managed, meaning the
> +hwmon device does not have to be removed explicitly by the removal function.
> +
> hwmon_device_unregister deregisters a registered hardware monitoring device.
> The parameter of this function is the pointer to the registered hardware
> monitoring device structure. This function must be called from the driver
> remove function if the hardware monitoring device was registered with
> -hwmon_device_register or with hwmon_device_register_with_groups.
> +hwmon_device_register, hwmon_device_register_with_groups, or
> +hwmon_device_register_with_info.
>
> devm_hwmon_device_unregister does not normally have to be called. It is only
> needed for error handling, and only needed if the driver probe fails after
> -the call to devm_hwmon_device_register_with_groups.
> +the call to devm_hwmon_device_register_with_groups and if the automatic
> +(device managed) removal would be too late.
> +
> +Using devm_hwmon_device_register_with_info()
> +--------------------------------------------
> +
> +hwmon_device_register_with_info() registers a hardware monitoring device.
> +The parameters to this function are
> +
> +struct device *dev Pointer to parent device
> +const char *name Device name
> +void *drvdata Driver private data
> +const struct hwmon_chip_info *info
> + Pointer to chip description.
> +const struct attribute_group **groups
> + Null-terminated list of additional sysfs attribute
> + groups.
> +
> +This function returns a pointer to the created hardware monitoring device
> +on success and a negative error code for failure.
> +
> +The hwmon_chip_info structure looks as follows.
> +
> +struct hwmon_chip_info {
> + const struct hwmon_ops *ops;
> + const struct hwmon_channel_info **info;
> +};
> +
> +It contains the following fields:
> +
> +* ops: Pointer to device operations.
> +* info: NULL-terminated list of device channel descriptors.
> +
> +The list of hwmon operations is defined as:
> +
> +struct hwmon_ops {
> + umode_t (*is_visible)(const void *, enum hwmon_sensor_types type,
> + u32 attr, int);
> + int (*read)(struct device *, enum hwmon_sensor_types type,
> + u32 attr, int, long *);
> + int (*write)(struct device *, enum hwmon_sensor_types type,
> + u32 attr, int, long);
> +};
> +
> +It defines the following operations.
> +
> +* is_visible: Pointer to a function to return the file mode for each supported
> + attribute. This function is mandatory.
> +
> +* read: Pointer to a function for reading a value from the chip. This function
> + is optional, but must be provided if any readable attributes exist.
> +
> +* write: Pointer to a function for writing a value to the chip. This function is
> + optional, but must be priovided if any writeable attributes exist.
provided
> +
> +Each sensor channel is described with struct hwmon_channel_info, which is
> +defined as follows.
> +
> +struct hwmon_channel_info {
> + enum hwmon_sensor_types type;
> + u32 *config;
> +};
> +
> +It contains following fields:
> +
> +* type: The hardware monitoring sensor type.
> + Supported sensor types are
> + * hwmon_chip A virtual sensor type, used to describe attributes
> + which apply to the entire chip.
> + * hwmon_temp Temperature sensor
> + * hwmon_in Voltage sensor
> + * hwmon_curr Current sensor
> + * hwmon_power Power sensor
> + * hwmon_energy Energy sensor
> + * hwmon_humidity Humidity sensor
> + * hwmon_fan Fan speed sensor
> +
> +* config: Pointer to a 0-terminated list of configuration values for each
> + sensor of the given type. Each value is a combination of bit values
> + describing the attributes supposed by a single sensor.
> +
> +As an example, here is the complete description file for a LM75 compatible
> +sensor chip. The chip has a single temperature sensor. The driver wants to
> +register with the thermal subsystem (HWMON_C_REGISTER_TZ), and it supports
> +the update_interval attribute (HWMON_C_UPDATE_INTERVAL). The chip supports
> +reading the temperature (HWMON_T_INPUT), it has a maximum temperature
> +register (HWMON_T_MAX) as well as a maximum temperature hysteresis register
> +(HWMON_T_MAX_HYST).
> +
> +static const u32 lm75_chip_config[] = {
> + HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL,
> + 0
> +};
> +
> +static const struct hwmon_channel_info lm75_chip = {
> + .type = hwmon_chip,
> + .config = lm75_chip_config,
> +};
> +
> +static const u32 lm75_temp_config[] = {
> + HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST,
> + 0
> +};
> +
> +static const struct hwmon_channel_info lm75_temp = {
> + .type = hwmon_temp,
> + .config = lm75_temp_config,
> +};
> +
> +static const struct hwmon_channel_info *lm75_info[] = {
> + &lm75_chip,
> + &lm75_temp,
> + NULL
> +};
> +
> +static const struct hwmon_ops lm75_hwmon_ops = {
> + .is_visible = lm75_is_visible,
> + .read = lm75_read,
> + .write = lm75_write,
> +};
> +
> +static const struct hwmon_chip_info lm75_chip_info = {
> + .ops = &lm75_hwmon_ops,
> + .info = lm75_info,
> +};
> +
> +A complete list of bit values indicating individual attribute support
> +is defined in include/linux/hwmon.h. Definition prefixes are as follows.
> +
> +HWMON_C_xxxx Chip attributes, for use with hwmon_chip.
> +HWMON_T_xxxx Temperature attributes, for use with hwmon_temp.
> +HWMON_I_xxxx Voltage attributes, for use with hwmon_in.
> +HWMON_C_xxxx Current attributes, for use with hwmon_curr.
> + Notice the prefix overlap with chip attributes.
> +HWMON_P_xxxx Power attributes, for use with hwmon_power.
> +HWMON_E_xxxx Energy attributes, for use with hwmon_energy.
> +HWMON_H_xxxx Humidity attributes, for use with hwmon_humidity.
> +HWMON_F_xxxx Fan speed attributes, for use with hwmon_fan.
> +
> +Driver callback functions
> +-------------------------
> +
> +Each driver provides is_visible, read, and write functions. Parameters
> +and return values for those functions are as follows.
> +
> +umode_t is_visible_func(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +
> +Parameters:
> + data: Pointer to device private data structure.
> + type: The sensor type.
> + attr: Attribute identifier associated with a specific attribute.
> + For example, the attribute value for HWMON_T_INPUT would be
> + hwmon_temp_input. For complete mappings of bit fields to
> + attribute values please see include/linux/hwmon.h.
> + channel:The sensor channel number.
> +
> +Return value:
> + The file mode for this attribute. Typically, this will be 0 (the
> + attribute will not be created), S_IRUGO, or 'S_IRUGO | S_IWUSR'.
> +
> +int read_func(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +
> +Parameters:
> + dev: Pointer to the hardware monitoring device.
> + type: The sensor type.
> + attr: Attribute identifier associated with a specific attribute.
> + For example, the attribute value for HWMON_T_INPUT would be
> + hwmon_temp_input. For complete mappings please see
> + include/linux/hwmon.h.
> + channel:The sensor channel number.
> + val: Pointer to attribute value.
> +
> +Return value:
> + 0 on success, a negative error number otherwise.
> +
> +int write_func(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +
> +Parameters:
> + dev: Pointer to the hardware monitoring device.
> + type: The sensor type.
> + attr: Attribute identifier associated with a specific attribute.
> + For example, the attribute value for HWMON_T_INPUT would be
> + hwmon_temp_input. For complete mappings please see
> + include/linux/hwmon.h.
> + channel:The sensor channel number.
> + val: The value to write to the chip.
> +
> +Return value:
> + 0 on success, a negative error number otherwise.
> +
> +
> +Driver-provided sysfs attributes
> +--------------------------------
> +
> +If the hardware monitoring device is registered with
> +hwmon_device_register_with_info or devm_hwmon_device_register_with_info,
> +it is most likely not necessary to provide sysfs atributes. Only non-standard
attributes
> +sysfs attributes need to be provided when one of those registration functions
> +is used.
>
> The header file linux/hwmon-sysfs.h provides a number of useful macros to
> declare and use hardware monitoring sysfs attributes.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
` (6 preceding siblings ...)
2016-06-26 3:26 ` [PATCH 7/7] hwmon: (core) Document new kernel API Guenter Roeck
@ 2016-07-08 9:31 ` Punit Agrawal
2016-07-08 13:48 ` Guenter Roeck
2016-07-10 16:00 ` Jonathan Cameron
8 siblings, 1 reply; 22+ messages in thread
From: Punit Agrawal @ 2016-07-08 9:31 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Jonathan Cameron, Zhang Rui, Eduardo Valentin,
linux-pm, linux-iio, linux-hwmon, linux-kernel
Hi Guenter,
Guenter Roeck <linux@roeck-us.net> writes:
> Up to now, each hwmon driver has to implement its own sysfs attributes.
> This requires a lot of template code, and distracts from the driver's
> core function to read and write chip registers.
>
> To be able to reduce driver complexity, move sensor attribute handling
> and thermal zone registration into the hwmon core. By using the new API,
> driver size is typically reduced by 20-50% depending on driver complexity
> and the number of sysfs attributes supported.
>
> The first patch of the series introduces the API as well as support
> for temperature sensors. Subsequent patches introduce support for
> voltage, current, power, energy, humidity, and fan speed sensors.
>
> The series was tested by converting several drivers (lm75, lm90, tmp102,
> tmp421, ltc4245) to the new API. Testing was done with with real chips
> as well as with the hwmon driver module test code available at
> https://github.com/groeck/module-tests.
I like this series - it takes all of the attributes' handling out of the
individual driver code and moving it to hwmon core.
Having attempted a port of scpi-hwmon.c, I think that driver will not
gain a big savings in line count. Though it'll help separate access to
sensors from sysfs related code - which I think is worth the change.
FWIW,
Acked-by: Punit Agrawal <punit.agrawal@arm.com>
Thanks,
Punit
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
2016-07-08 9:31 ` [PATCH 0/5] hwmon: New hwmon registration API Punit Agrawal
@ 2016-07-08 13:48 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-07-08 13:48 UTC (permalink / raw)
To: Punit Agrawal
Cc: Jean Delvare, Jonathan Cameron, Zhang Rui, Eduardo Valentin,
linux-pm, linux-iio, linux-hwmon, linux-kernel
Hi Punit,
On 07/08/2016 02:31 AM, Punit Agrawal wrote:
> Hi Guenter,
>
> Guenter Roeck <linux@roeck-us.net> writes:
>
>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>> This requires a lot of template code, and distracts from the driver's
>> core function to read and write chip registers.
>>
>> To be able to reduce driver complexity, move sensor attribute handling
>> and thermal zone registration into the hwmon core. By using the new API,
>> driver size is typically reduced by 20-50% depending on driver complexity
>> and the number of sysfs attributes supported.
>>
>> The first patch of the series introduces the API as well as support
>> for temperature sensors. Subsequent patches introduce support for
>> voltage, current, power, energy, humidity, and fan speed sensors.
>>
>> The series was tested by converting several drivers (lm75, lm90, tmp102,
>> tmp421, ltc4245) to the new API. Testing was done with with real chips
>> as well as with the hwmon driver module test code available at
>> https://github.com/groeck/module-tests.
>
> I like this series - it takes all of the attributes' handling out of the
> individual driver code and moving it to hwmon core.
>
> Having attempted a port of scpi-hwmon.c, I think that driver will not
> gain a big savings in line count. Though it'll help separate access to
> sensors from sysfs related code - which I think is worth the change.
>
From what I have seen, savings are for the most part on the binary image size.
I have not seen much if any savings in terms of LOC, though that may in part
be because of my coding style.
Here is the result from the conversions I have done so far.
Old:
text data bss dec hex filename
5730 4056 64 9850 267a drivers/hwmon/lm95245.o
5941 4240 64 10245 2805 drivers/hwmon/lm95241.o
5361 3584 64 9009 2331 drivers/hwmon/jc42.o
8609 10872 64 19545 4c59 drivers/hwmon/max31790.o
9080 13232 64 22376 5768 drivers/hwmon/nct7904.o
6574 8498 64 15136 3b20 drivers/hwmon/ltc4245.o
4516 3464 64 8044 1f6c drivers/hwmon/tmp421.o
4223 2744 64 7031 1b77 drivers/hwmon/tmp102.o
21757 13496 64 35317 89f5 drivers/hwmon/lm90.o
6804 3240 64 10108 277c drivers/hwmon/lm75.o
New:
text data bss dec hex filename
5166 1568 128 6862 1ace drivers/hwmon/lm95245.o
4334 1664 64 6062 17ae drivers/hwmon/lm95241.o
4579 1456 64 6099 17d3 drivers/hwmon/jc42.o
3687 1312 64 5063 13c7 drivers/hwmon/max31790.o
3905 1720 64 5689 1639 drivers/hwmon/nct7904.o
3989 1658 64 5711 164f drivers/hwmon/ltc4245.o
3557 1408 64 5029 13a5 drivers/hwmon/tmp421.o
4037 2200 64 6301 189d drivers/hwmon/tmp102.o
16485 4288 64 20837 5165 drivers/hwmon/lm90.o
5762 2128 64 7954 1f12 drivers/hwmon/lm75.o
This is with x86_64.
The largest savings are in drivers with simple access methods and
a large number of attributes. The scpi driver is somewhat of an
exception since it creates its attribute data structures dynamically;
I would not expect to see much savings in that driver.
> FWIW,
>
> Acked-by: Punit Agrawal <punit.agrawal@arm.com>
>
Thanks!
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
2016-06-26 3:26 [PATCH 0/5] hwmon: New hwmon registration API Guenter Roeck
` (7 preceding siblings ...)
2016-07-08 9:31 ` [PATCH 0/5] hwmon: New hwmon registration API Punit Agrawal
@ 2016-07-10 16:00 ` Jonathan Cameron
2016-07-11 0:56 ` Guenter Roeck
8 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2016-07-10 16:00 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm, linux-iio, linux-hwmon,
linux-kernel
On 26/06/16 04:26, Guenter Roeck wrote:
> Up to now, each hwmon driver has to implement its own sysfs attributes.
> This requires a lot of template code, and distracts from the driver's
> core function to read and write chip registers.
>
> To be able to reduce driver complexity, move sensor attribute handling
> and thermal zone registration into the hwmon core. By using the new API,
> driver size is typically reduced by 20-50% depending on driver complexity
> and the number of sysfs attributes supported.
>
> The first patch of the series introduces the API as well as support
> for temperature sensors. Subsequent patches introduce support for
> voltage, current, power, energy, humidity, and fan speed sensors.
>
> The series was tested by converting several drivers (lm75, lm90, tmp102,
> tmp421, ltc4245) to the new API. Testing was done with with real chips
> as well as with the hwmon driver module test code available at
> https://github.com/groeck/module-tests.
Some cool stuff hiding in there :)
I've only reviewed 1 and 7 and the intermediate ones are really either
correct or they aren't and they look fine to me.
So what's next? (beyond presumably a lot of driver conversions).
Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
@ 2016-07-11 0:56 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-07-11 0:56 UTC (permalink / raw)
To: Jonathan Cameron, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm, linux-iio, linux-hwmon,
linux-kernel
Hi Jonathan,
On 07/10/2016 09:00 AM, Jonathan Cameron wrote:
> On 26/06/16 04:26, Guenter Roeck wrote:
>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>> This requires a lot of template code, and distracts from the driver's
>> core function to read and write chip registers.
>>
>> To be able to reduce driver complexity, move sensor attribute handling
>> and thermal zone registration into the hwmon core. By using the new API,
>> driver size is typically reduced by 20-50% depending on driver complexity
>> and the number of sysfs attributes supported.
>>
>> The first patch of the series introduces the API as well as support
>> for temperature sensors. Subsequent patches introduce support for
>> voltage, current, power, energy, humidity, and fan speed sensors.
>>
>> The series was tested by converting several drivers (lm75, lm90, tmp102,
>> tmp421, ltc4245) to the new API. Testing was done with with real chips
>> as well as with the hwmon driver module test code available at
>> https://github.com/groeck/module-tests.
> Some cool stuff hiding in there :)
>
> I've only reviewed 1 and 7 and the intermediate ones are really either
> correct or they aren't and they look fine to me.
>
Thanks a lot for your feedback - it is reassuring that it looks sane to you.
>
> So what's next? (beyond presumably a lot of driver conversions).
>
Next steps will be to address your comments, re-test, submit a new series,
add to -next after v4.8-rc1 is out, submit the conversions I have done
and add to -next. Then, obviously, only accept new drivers using the
new API.
Not sure if there is anything else we can or want to do do at this time.
Do you have anything in mind ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
@ 2016-07-11 0:56 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-07-11 0:56 UTC (permalink / raw)
To: Jonathan Cameron, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Jonathan,
On 07/10/2016 09:00 AM, Jonathan Cameron wrote:
> On 26/06/16 04:26, Guenter Roeck wrote:
>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>> This requires a lot of template code, and distracts from the driver's
>> core function to read and write chip registers.
>>
>> To be able to reduce driver complexity, move sensor attribute handling
>> and thermal zone registration into the hwmon core. By using the new API,
>> driver size is typically reduced by 20-50% depending on driver complexity
>> and the number of sysfs attributes supported.
>>
>> The first patch of the series introduces the API as well as support
>> for temperature sensors. Subsequent patches introduce support for
>> voltage, current, power, energy, humidity, and fan speed sensors.
>>
>> The series was tested by converting several drivers (lm75, lm90, tmp102,
>> tmp421, ltc4245) to the new API. Testing was done with with real chips
>> as well as with the hwmon driver module test code available at
>> https://github.com/groeck/module-tests.
> Some cool stuff hiding in there :)
>
> I've only reviewed 1 and 7 and the intermediate ones are really either
> correct or they aren't and they look fine to me.
>
Thanks a lot for your feedback - it is reassuring that it looks sane to you.
>
> So what's next? (beyond presumably a lot of driver conversions).
>
Next steps will be to address your comments, re-test, submit a new series,
add to -next after v4.8-rc1 is out, submit the conversions I have done
and add to -next. Then, obviously, only accept new drivers using the
new API.
Not sure if there is anything else we can or want to do do at this time.
Do you have anything in mind ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
@ 2016-07-20 15:11 ` Jonathan Cameron
0 siblings, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2016-07-20 15:11 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm, linux-iio, linux-hwmon,
linux-kernel
On 11/07/16 01:56, Guenter Roeck wrote:
> Hi Jonathan,
>
> On 07/10/2016 09:00 AM, Jonathan Cameron wrote:
>> On 26/06/16 04:26, Guenter Roeck wrote:
>>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>>> This requires a lot of template code, and distracts from the driver's
>>> core function to read and write chip registers.
>>>
>>> To be able to reduce driver complexity, move sensor attribute handling
>>> and thermal zone registration into the hwmon core. By using the new API,
>>> driver size is typically reduced by 20-50% depending on driver complexity
>>> and the number of sysfs attributes supported.
>>>
>>> The first patch of the series introduces the API as well as support
>>> for temperature sensors. Subsequent patches introduce support for
>>> voltage, current, power, energy, humidity, and fan speed sensors.
>>>
>>> The series was tested by converting several drivers (lm75, lm90, tmp102,
>>> tmp421, ltc4245) to the new API. Testing was done with with real chips
>>> as well as with the hwmon driver module test code available at
>>> https://github.com/groeck/module-tests.
>> Some cool stuff hiding in there :)
>>
>> I've only reviewed 1 and 7 and the intermediate ones are really either
>> correct or they aren't and they look fine to me.
>>
> Thanks a lot for your feedback - it is reassuring that it looks sane to you.
>
>>
>> So what's next? (beyond presumably a lot of driver conversions).
>>
> Next steps will be to address your comments, re-test, submit a new series,
> add to -next after v4.8-rc1 is out, submit the conversions I have done
> and add to -next. Then, obviously, only accept new drivers using the
> new API.
>
> Not sure if there is anything else we can or want to do do at this time.
> Do you have anything in mind ?
>
Was wondering about the bridge to IIO (reverse of iio to hwmon bridge)
that you mentioned - for that you'd probably need some in kernel interfaces
for all this stuff.
Anyhow, stuff for the future (perhaps distant)
Jonathan
> Thanks,
> Guenter
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
@ 2016-07-20 15:11 ` Jonathan Cameron
0 siblings, 0 replies; 22+ messages in thread
From: Jonathan Cameron @ 2016-07-20 15:11 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare
Cc: Zhang Rui, Eduardo Valentin, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 11/07/16 01:56, Guenter Roeck wrote:
> Hi Jonathan,
>
> On 07/10/2016 09:00 AM, Jonathan Cameron wrote:
>> On 26/06/16 04:26, Guenter Roeck wrote:
>>> Up to now, each hwmon driver has to implement its own sysfs attributes.
>>> This requires a lot of template code, and distracts from the driver's
>>> core function to read and write chip registers.
>>>
>>> To be able to reduce driver complexity, move sensor attribute handling
>>> and thermal zone registration into the hwmon core. By using the new API,
>>> driver size is typically reduced by 20-50% depending on driver complexity
>>> and the number of sysfs attributes supported.
>>>
>>> The first patch of the series introduces the API as well as support
>>> for temperature sensors. Subsequent patches introduce support for
>>> voltage, current, power, energy, humidity, and fan speed sensors.
>>>
>>> The series was tested by converting several drivers (lm75, lm90, tmp102,
>>> tmp421, ltc4245) to the new API. Testing was done with with real chips
>>> as well as with the hwmon driver module test code available at
>>> https://github.com/groeck/module-tests.
>> Some cool stuff hiding in there :)
>>
>> I've only reviewed 1 and 7 and the intermediate ones are really either
>> correct or they aren't and they look fine to me.
>>
> Thanks a lot for your feedback - it is reassuring that it looks sane to you.
>
>>
>> So what's next? (beyond presumably a lot of driver conversions).
>>
> Next steps will be to address your comments, re-test, submit a new series,
> add to -next after v4.8-rc1 is out, submit the conversions I have done
> and add to -next. Then, obviously, only accept new drivers using the
> new API.
>
> Not sure if there is anything else we can or want to do do at this time.
> Do you have anything in mind ?
>
Was wondering about the bridge to IIO (reverse of iio to hwmon bridge)
that you mentioned - for that you'd probably need some in kernel interfaces
for all this stuff.
Anyhow, stuff for the future (perhaps distant)
Jonathan
> Thanks,
> Guenter
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/5] hwmon: New hwmon registration API
2016-07-20 15:11 ` Jonathan Cameron
(?)
@ 2016-07-20 18:38 ` Guenter Roeck
-1 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2016-07-20 18:38 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jean Delvare, Zhang Rui, Eduardo Valentin, linux-pm, linux-iio,
linux-hwmon, linux-kernel
On Wed, Jul 20, 2016 at 04:11:04PM +0100, Jonathan Cameron wrote:
> On 11/07/16 01:56, Guenter Roeck wrote:
> > Hi Jonathan,
> >
> > On 07/10/2016 09:00 AM, Jonathan Cameron wrote:
> >> On 26/06/16 04:26, Guenter Roeck wrote:
> >>> Up to now, each hwmon driver has to implement its own sysfs attributes.
> >>> This requires a lot of template code, and distracts from the driver's
> >>> core function to read and write chip registers.
> >>>
> >>> To be able to reduce driver complexity, move sensor attribute handling
> >>> and thermal zone registration into the hwmon core. By using the new API,
> >>> driver size is typically reduced by 20-50% depending on driver complexity
> >>> and the number of sysfs attributes supported.
> >>>
> >>> The first patch of the series introduces the API as well as support
> >>> for temperature sensors. Subsequent patches introduce support for
> >>> voltage, current, power, energy, humidity, and fan speed sensors.
> >>>
> >>> The series was tested by converting several drivers (lm75, lm90, tmp102,
> >>> tmp421, ltc4245) to the new API. Testing was done with with real chips
> >>> as well as with the hwmon driver module test code available at
> >>> https://github.com/groeck/module-tests.
> >> Some cool stuff hiding in there :)
> >>
> >> I've only reviewed 1 and 7 and the intermediate ones are really either
> >> correct or they aren't and they look fine to me.
> >>
> > Thanks a lot for your feedback - it is reassuring that it looks sane to you.
> >
> >>
> >> So what's next? (beyond presumably a lot of driver conversions).
> >>
> > Next steps will be to address your comments, re-test, submit a new series,
> > add to -next after v4.8-rc1 is out, submit the conversions I have done
> > and add to -next. Then, obviously, only accept new drivers using the
> > new API.
> >
> > Not sure if there is anything else we can or want to do do at this time.
> > Do you have anything in mind ?
> >
> Was wondering about the bridge to IIO (reverse of iio to hwmon bridge)
> that you mentioned - for that you'd probably need some in kernel interfaces
> for all this stuff.
>
Depends; maybe it would be sufficient to call devm_iio_device_register() with
a minimum set of channel information. Looks like all we would need is .read_raw
and .write_raw callbacks in struct iio_info, a channel description for each
channel, and the number of channels. Implementing this would not be difficult.
Only question would be if it would be sufficient for most or at least many
use cases.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread