Re: [PATCH 1/7] hwmon: (core) New hwmon registration API
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
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
+#include
#include
#include
-#include
-#include
-#include
#include
-#include
-#include
+#include
+#include
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
+#include
#include
+#include
#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,
+
Re: [PATCH 1/7] hwmon: (core) New hwmon registration API
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
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
> +#include
> #include
> #include
> -#include
> -#include
> -#include
> #include
> -#include
> -#include
> +#include
> +#include
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
> +#include
> #include
> +#include
>
> #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,
[PATCH 1/7] hwmon: (core) New hwmon registration API
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
---
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
+#include
#include
#include
-#include
-#include
-#include
#include
-#include
-#include
+#include
+#include
#include
+#include
#include
+#include
#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,
+
