Re: [PATCH] driver core: auxiliary bus: add device creation helper

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 10 Dec 2024 at 15:05, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Dec 10, 2024 at 02:43:12PM +0100, Jerome Brunet wrote:
>> Add an function helper to create a device on the auxiliary bus.
>> This should avoid having the same code repeated in the different drivers
>> registering auxiliary devices.
>> 
>> Suggested-by: Stephen Boyd <sboyd@kernel.org>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>> The suggestion for this change was initially discussed here: [1]
>> 
>> I was not sure if the managed variant should return the auxiliary device or
>> just the error. This initial version returns the auxiliary device, allowing
>> it to be further (ab)used. Please let me know if you prefer to just return
>> the error code instead.
>> 
>> Also the non managed variant of the helper is not exported but it could
>> easily be, if necessary.
>> 
>> [1]: https://lore.kernel.org/linux-clk/df0a53ee859e450d84e81547099f5f36.sboyd@kernel.org
>> ---
>>  drivers/base/auxiliary.c      | 89 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/auxiliary_bus.h |  4 ++
>>  2 files changed, 93 insertions(+)
>
> We can't add new functions like this without a real user of it.  Please
> submit that at the same time.

Sure. There is some prep work ongoing in the user. It will get used once
that's done. I'll resubmit once this is ready, assuming the rest is fine.

>
> And are you ok with sharing the id range with multiple aux bus
> implementations?
>

In the initial discussion, a global id was thought to sufficient [2]
It also helps to make things simpler on the user side, which is good I think.

Do you think we've overlooked something ?

[2]: https://lore.kernel.org/linux-clk/c9556de589e289cb1d278d41014791a6.sboyd@kernel.org

>
>> 
>> diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
>> index afa4df4c5a3f371b91d8dd8c4325495d32ad1291..60ca3f0da329fb7f8e69ecdf703b505e7cf5085c 100644
>> --- a/drivers/base/auxiliary.c
>> +++ b/drivers/base/auxiliary.c
>> @@ -385,6 +385,95 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv)
>>  }
>>  EXPORT_SYMBOL_GPL(auxiliary_driver_unregister);
>>  
>> +static DEFINE_IDA(auxiliary_device_ida);
>> +
>> +static void auxiliary_device_release(struct device *dev)
>> +{
>> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
>> +
>> +	ida_free(&auxiliary_device_ida, auxdev->id);
>> +	kfree(auxdev);
>> +}
>> +
>> +static struct auxiliary_device *auxiliary_device_create(struct device *dev,
>> +							const char *name,
>> +							void *platform_data)
>> +{
>> +	struct auxiliary_device *auxdev;
>> +	int ret;
>> +
>> +	auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
>> +	if (!auxdev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	ret = ida_alloc(&auxiliary_device_ida, GFP_KERNEL);
>> +	if (ret < 0)
>> +		goto auxdev_free;
>> +
>> +	auxdev->id = ret;
>> +	auxdev->name = name;
>> +	auxdev->dev.parent = dev;
>> +	auxdev->dev.platform_data = platform_data;
>> +	auxdev->dev.release = auxiliary_device_release;
>> +	device_set_of_node_from_dev(&auxdev->dev, dev);
>> +
>> +	ret = auxiliary_device_init(auxdev);
>> +	if (ret)
>> +		goto ida_free;
>> +
>> +	ret = __auxiliary_device_add(auxdev, dev->driver->name);
>> +	if (ret) {
>> +		auxiliary_device_uninit(auxdev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return auxdev;
>> +
>> +ida_free:
>> +	ida_free(&auxiliary_device_ida, auxdev->id);
>> +auxdev_free:
>> +	kfree(auxdev);
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +static void auxiliary_device_destroy(void *_auxdev)
>> +{
>> +	struct auxiliary_device *auxdev = _auxdev;
>> +
>> +	auxiliary_device_delete(auxdev);
>> +	auxiliary_device_uninit(auxdev);
>> +}
>> +
>> +/**
>> + * devm_auxiliary_device_create - create a device on the auxiliary bus
>> + * @dev: parent device
>> + * @name: auxiliary bus driver name
>> + * @platform_data: auxiliary bus device platform data
>> + *
>> + * Device managed helper to create an auxiliary bus device.
>> + * The parent device KBUILD_MODNAME is automatically inserted before the
>
> KBUILD_MODNAME doesn't make sense here, as that's the aux bus file.

I'll change to "parent device driver name" 

>
> thanks,
>
> greg k-h

-- 
Jerome