| Message ID | 28f92a8e63ce4b0e1071e8fdb2315a057daee9c6.1554934898.git.krzysztof.adamski@nokia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pmbus: extend configurability via sysfs | expand |
On 4/10/19 3:38 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > This patch makes it possible to pass custom struct attribute_group array > via the pmbus_driver_info struct so that those can be added to the > attribute groups passed to hwmon_device_register_with_groups(). > > This makes it possible to register custom sysfs attributes by PMBUS > drivers similar to how you can do this with most other busses/classes. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> > --- > drivers/hwmon/pmbus/pmbus.h | 3 +++ > drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > index 1d24397d36ec..fb267ec11623 100644 > --- a/drivers/hwmon/pmbus/pmbus.h > +++ b/drivers/hwmon/pmbus/pmbus.h > @@ -417,6 +417,9 @@ struct pmbus_driver_info { > /* Regulator functionality, if supported by this chip driver. */ > int num_regulators; > const struct regulator_desc *reg_desc; > + > + /* custom attributes */ > + const struct attribute_group **groups; I can understand the need and desire for one additional group. More than one is highly questionable. Please explain why you think that more than one extra attribute would ever be needed. It does add substantial complexity, so there should be a good reason. Thanks, Guenter > }; > > /* Regulator ops */ > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 2e2b5851139c..7a7dcdc8acc9 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -103,7 +103,7 @@ struct pmbus_data { > int max_attributes; > int num_attributes; > struct attribute_group group; > - const struct attribute_group *groups[2]; > + const struct attribute_group **groups; > struct dentry *debugfs; /* debugfs device directory */ > > struct pmbus_sensor *sensors; > @@ -2305,6 +2305,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, > struct device *dev = &client->dev; > const struct pmbus_platform_data *pdata = dev_get_platdata(dev); > struct pmbus_data *data; > + size_t groups_num = 0; > int ret; > > if (!info) > @@ -2319,6 +2320,15 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, > if (!data) > return -ENOMEM; > > + if (info->groups) > + while (info->groups[groups_num]) > + groups_num++; > + > + data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *), > + GFP_KERNEL); > + if (!data->groups) > + return -ENOMEM; > + > i2c_set_clientdata(client, data); > mutex_init(&data->update_lock); > data->dev = dev; > @@ -2346,6 +2356,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, > } > > data->groups[0] = &data->group; > + memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num); > data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name, > data, data->groups); > if (IS_ERR(data->hwmon_dev)) { >
On Wed, Apr 10, 2019 at 05:35:21PM -0700, Guenter Roeck wrote: >On 4/10/19 3:38 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>This patch makes it possible to pass custom struct attribute_group array >>via the pmbus_driver_info struct so that those can be added to the >>attribute groups passed to hwmon_device_register_with_groups(). >> >>This makes it possible to register custom sysfs attributes by PMBUS >>drivers similar to how you can do this with most other busses/classes. >> >>Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>--- >> drivers/hwmon/pmbus/pmbus.h | 3 +++ >> drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++- >> 2 files changed, 15 insertions(+), 1 deletion(-) >> >>diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h >>index 1d24397d36ec..fb267ec11623 100644 >>--- a/drivers/hwmon/pmbus/pmbus.h >>+++ b/drivers/hwmon/pmbus/pmbus.h >>@@ -417,6 +417,9 @@ struct pmbus_driver_info { >> /* Regulator functionality, if supported by this chip driver. */ >> int num_regulators; >> const struct regulator_desc *reg_desc; >>+ >>+ /* custom attributes */ >>+ const struct attribute_group **groups; > >I can understand the need and desire for one additional group. More than one >is highly questionable. Please explain why you think that more than one extra >attribute would ever be needed. It does add substantial complexity, so >there should be a good reason. The only situation I could come up is if the driver would want to group attributes in different directories by setting different name for each of them. One other reason I choose to use this approach is that this seems to be standard way for passing this information on other layers/frameworks. For example, this is the same "format" you would pass this kind of data when creating a class, a bus, a driver or when you use any of the *_register_with_groups(). This approach is simply more generic with (to my opinion) low cost of implementation. But if we don't want to support that, I'm fine to change this to single custom group. Krzysztof
On 4/11/19 12:53 AM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > On Wed, Apr 10, 2019 at 05:35:21PM -0700, Guenter Roeck wrote: >> On 4/10/19 3:38 PM, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: >>> This patch makes it possible to pass custom struct attribute_group array >>> via the pmbus_driver_info struct so that those can be added to the >>> attribute groups passed to hwmon_device_register_with_groups(). >>> >>> This makes it possible to register custom sysfs attributes by PMBUS >>> drivers similar to how you can do this with most other busses/classes. >>> >>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> >>> --- >>> drivers/hwmon/pmbus/pmbus.h | 3 +++ >>> drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++- >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h >>> index 1d24397d36ec..fb267ec11623 100644 >>> --- a/drivers/hwmon/pmbus/pmbus.h >>> +++ b/drivers/hwmon/pmbus/pmbus.h >>> @@ -417,6 +417,9 @@ struct pmbus_driver_info { >>> /* Regulator functionality, if supported by this chip driver. */ >>> int num_regulators; >>> const struct regulator_desc *reg_desc; >>> + >>> + /* custom attributes */ >>> + const struct attribute_group **groups; >> >> I can understand the need and desire for one additional group. More than one >> is highly questionable. Please explain why you think that more than one extra >> attribute would ever be needed. It does add substantial complexity, so >> there should be a good reason. > > The only situation I could come up is if the driver would want to group > attributes in different directories by setting different name for each > of them. One other reason I choose to use this approach is that this > seems to be standard way for passing this information on other > layers/frameworks. For example, this is the same "format" you would > pass this kind of data when creating a class, a bus, a driver or when > you use any of the *_register_with_groups(). > > This approach is simply more generic with (to my opinion) low cost of > implementation. But if we don't want to support that, I'm fine to change > this to single custom group. > I think we can postpone this discussion for now, as we'll go with standard attributes to be implemented in the pmbus core. Thanks, Guenter
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h index 1d24397d36ec..fb267ec11623 100644 --- a/drivers/hwmon/pmbus/pmbus.h +++ b/drivers/hwmon/pmbus/pmbus.h @@ -417,6 +417,9 @@ struct pmbus_driver_info { /* Regulator functionality, if supported by this chip driver. */ int num_regulators; const struct regulator_desc *reg_desc; + + /* custom attributes */ + const struct attribute_group **groups; }; /* Regulator ops */ diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 2e2b5851139c..7a7dcdc8acc9 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -103,7 +103,7 @@ struct pmbus_data { int max_attributes; int num_attributes; struct attribute_group group; - const struct attribute_group *groups[2]; + const struct attribute_group **groups; struct dentry *debugfs; /* debugfs device directory */ struct pmbus_sensor *sensors; @@ -2305,6 +2305,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, struct device *dev = &client->dev; const struct pmbus_platform_data *pdata = dev_get_platdata(dev); struct pmbus_data *data; + size_t groups_num = 0; int ret; if (!info) @@ -2319,6 +2320,15 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, if (!data) return -ENOMEM; + if (info->groups) + while (info->groups[groups_num]) + groups_num++; + + data->groups = devm_kcalloc(dev, groups_num + 2, sizeof(void *), + GFP_KERNEL); + if (!data->groups) + return -ENOMEM; + i2c_set_clientdata(client, data); mutex_init(&data->update_lock); data->dev = dev; @@ -2346,6 +2356,7 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, } data->groups[0] = &data->group; + memcpy(data->groups + 1, info->groups, sizeof(void *) * groups_num); data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name, data, data->groups); if (IS_ERR(data->hwmon_dev)) {
This patch makes it possible to pass custom struct attribute_group array via the pmbus_driver_info struct so that those can be added to the attribute groups passed to hwmon_device_register_with_groups(). This makes it possible to register custom sysfs attributes by PMBUS drivers similar to how you can do this with most other busses/classes. Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com> --- drivers/hwmon/pmbus/pmbus.h | 3 +++ drivers/hwmon/pmbus/pmbus_core.c | 13 ++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-)