Date: Thu, 20 Jul 2017 01:12:56 -0700
User-Agent: K-9 Mail for Android
In-Reply-To: <20170720051018.GE7323@kroah.com>
References: <20170720002436.29309-1-dmitry.torokhov@gmail.com> <20170720002436.29309-5-dmitry.torokhov@gmail.com> <20170720051018.GE7323@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Subject: Re: [PATCH v2 4/7] driver core: add devm_device_add_group() and friends
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: htejun@gmail.com, linux-kernel@vger.kernel.org,
        Guenter Roeck <linux@roeck-us.net>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Message-ID: <BE1DA622-8285-4A3F-8885-4D08F3E42646@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 3270
Lines: 106

On July 19, 2017 10:10:18 PM PDT, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Wed, Jul 19, 2017 at 05:24:33PM -0700, Dmitry Torokhov wrote:
>> Many drivers create additional driver-specific device attributes when
>> binding to the device, and providing managed version of
>> device_create_group() will simplify unbinding and error handling in
>probe
>> path for such drivers.
>> 
>> Without managed version driver writers either have to mix manual and
>> managed resources, which is prone to errors, or open-code this
>function by
>> providing a wrapper to device_add_group() and use it with
>devm_add_action()
>> or devm_add_action_or_reset().
>> 
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/base/core.c    | 130
>+++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/device.h |   9 ++++
>>  2 files changed, 139 insertions(+)
>> 
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 14f8cf5c8b05..09723532725d 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1035,6 +1035,136 @@ void device_remove_groups(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(device_remove_groups);
>>  
>> +union device_attr_group_devres {
>> +	const struct attribute_group *group;
>> +	const struct attribute_group **groups;
>> +};
>> +
>> +static int devm_attr_group_match(struct device *dev, void *res, void
>*data)
>> +{
>> +	return ((union device_attr_group_devres *)res)->group == data;
>> +}
>> +
>> +static void devm_attr_group_remove(struct device *dev, void *res)
>> +{
>> +	union device_attr_group_devres *devres = res;
>> +	const struct attribute_group *group = devres->group;
>> +
>> +	dev_dbg(dev, "%s: removing group %p\n", __func__, group);
>> +	sysfs_remove_group(&dev->kobj, group);
>> +}
>> +
>> +static void devm_attr_groups_remove(struct device *dev, void *res)
>> +{
>> +	union device_attr_group_devres *devres = res;
>> +	const struct attribute_group **groups = devres->groups;
>> +
>> +	dev_dbg(dev, "%s: removing groups %p\n", __func__, groups);
>> +	sysfs_remove_groups(&dev->kobj, groups);
>> +}
>> +
>> +/**
>> + * devm_device_add_group - given a device, create a managed
>attribute group
>> + * @dev:	The device to create the group for
>> + * @grp:	The attribute group to create
>> + *
>> + * This function creates a group for the first time.  It will
>explicitly
>> + * warn and error if any of the attribute files being created
>already exist.
>> + *
>> + * Returns 0 on success or error code on failure.
>> + */
>> +int devm_device_add_group(struct device *dev, const struct
>attribute_group *grp)
>> +{
>> +	union device_attr_group_devres *devres;
>> +	int error;
>> +
>> +	devres = devres_alloc(devm_attr_group_remove,
>> +			      sizeof(*devres), GFP_KERNEL);
>> +	if (!devres)
>> +		return -ENOMEM;
>> +
>> +	error = sysfs_create_group(&dev->kobj, grp);
>
>Minor nit, this can now call device_create_group(), right?
>
>Same with below I think as well.

Right.

>
>It's fine, these look great, I'll queue them up this afternoon...
>
>Thanks for persisting with these, and sorry it took so long to convince
>me I was wrong :)

:)

Any chance you could create an unmutable branch off 4.12 so I can start using it in input drivers?


Thanks.

-- 
Dmitry