On Tue, Jul 18, 2017 at 10:03:36PM +0200, Greg Kroah-Hartman wrote: > On Tue, Jul 18, 2017 at 12:30:58PM -0700, Dmitry Torokhov wrote: > > --- a/include/linux/sysfs.h > > +++ b/include/linux/sysfs.h > > @@ -282,6 +282,16 @@ int __compat_only_sysfs_link_entry_to_kobj(struct > > kobject *kobj, > > struct kobject *target_kobj, > > const char *target_name); > > > > +struct device; > > Put this in device.h instead?
I think we normally keep devm* and non-managed APIs together, like both regulator*() and devm_regulator*() are in include/linux/regulator/consumer.h, gpiod*() and devm_gpiod*() are in include/gpio/linux/consumer.h, clk*() and devm_clk() are in include/linux/clk.h, and so forth. I think there is benefit of having these together as well. > > > +int __must_check devm_sysfs_create_group(struct device *dev, > > + const struct attribute_group *grp); > > +int __must_check devm_sysfs_create_groups(struct device *dev, > > + const struct attribute_group **groups); > > +void devm_sysfs_remove_group(struct device *dev, > > + const struct attribute_group *grp); > > +void devm_sysfs_remove_groups(struct device *dev, > > + const struct attribute_group **groups); > > I have finally moved the driver core to only accept/need "groups" not a > single "group", so we should only need devm_sysfs_create_groups and > devm_sysfs_remove_groups, right? This makes total sense for the driver core, but individual drivers usually have a single group. Requiring all of them to have array of groups just adds unneeded boilerplate that I was trying to cut down. > > And do we need/want the non-devm versions: > device_create_groups() > device_remove_groups() > ? We already have non-managed sysfs_create_groups() and sysfs_remove_groups(). > > And you can probably drop the 'sysfs' from the function name if you > want. I think there is benefit of having devm version having name matching the non-managed one: sysfs_create_groups() and devm_sysfs_create_groups(). I hope you will reconsider. Thanks! -- Dmitry
