From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751634AbdGRU0L (ORCPT ); Tue, 18 Jul 2017 16:26:11 -0400 Received: from mail-pg0-f67.google.com ([74.125.83.67]:32833 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751457AbdGRU0K (ORCPT ); Tue, 18 Jul 2017 16:26:10 -0400 Date: Tue, 18 Jul 2017 13:26:06 -0700 From: Dmitry Torokhov To: Greg Kroah-Hartman Cc: htejun@gmail.com, linux-kernel@vger.kernel.org, Guenter Roeck Subject: Re: [PATCH 2/2] sysfs: add devm_sysfs_create_group() and friends Message-ID: <20170718202606.GA36888@dtor-ws> References: <20170718193058.29958-1-dmitry.torokhov@gmail.com> <20170718193058.29958-3-dmitry.torokhov@gmail.com> <20170718200336.GC3694@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170718200336.GC3694@kroah.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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