Re: [PATCH] nvmem: core: add managed version of nvmem_register

From: Heiner Kallweit
Date: Wed Jun 07 2017 - 17:56:03 EST


Am 07.06.2017 um 18:19 schrieb Srinivas Kandagatla:
>
> On 04/06/17 12:06, Heiner Kallweit wrote:
>> Add a device-managed version of nvmem_register.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> Documentation/nvmem/nvmem.txt | 1 +
>> drivers/nvmem/core.c | 35 +++++++++++++++++++++++++++++++++++
>> include/linux/nvmem-provider.h | 7 +++++++
>> 3 files changed, 43 insertions(+)
>>
>
> Thanks for the patch, one comments..
>> diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt
>> index dbd40d87..b4ff7862 100644
>> --- a/Documentation/nvmem/nvmem.txt
>> +++ b/Documentation/nvmem/nvmem.txt
>> @@ -37,6 +37,7 @@ and write the non-volatile memory.
>> A NVMEM provider can register with NVMEM core by supplying relevant
>> nvmem configuration to nvmem_register(), on success core would return a valid
>> nvmem_device pointer.
>> +devm_nvmem_register() is a device-managed version of nvmem_register.
>>
>> nvmem_unregister(nvmem) is used to unregister a previously registered provider.
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 783eb431..55db219f 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -531,6 +531,41 @@ int nvmem_unregister(struct nvmem_device *nvmem)
>> }
>> EXPORT_SYMBOL_GPL(nvmem_unregister);
>>
>> +static void devm_nvmem_release(struct device *dev, void *res)
>> +{
>> + nvmem_unregister(*(struct nvmem_device **)res);
>
> nvmem_unregister() can fail, how are you going to deal with this error cases?
>
As stated in my answer to your other review comment:
Currently no caller of nvmem_unregister checks the return code.
Checking the refcount I see more as a debug feature and I think making
nvmem_unregister return void plus a WARN() if refcount != 0 would be better.

Rgds, Heiner

>
>> +}
>> +
>> +/**
>> + * devm_nvmem_register() - managed version of nvmem_register
>> + *
>> + * @config: nvmem device configuration with which nvmem device is created.
>> + *
>> + * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device
>> + * on success.
>> + */
>> +
>> +struct nvmem_device *devm_nvmem_register(const struct nvmem_config *config)
>
> For consistency reasons, devm versions of apis should always have dev at as first argument.
>
>> +{
>> + struct nvmem_device *nv, **dr;
>> +
>> + dr = devres_alloc(devm_nvmem_release, sizeof(*dr), GFP_KERNEL);
>> + if (!dr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + nv = nvmem_register(config);
>> + if (IS_ERR(nv)) {
>> + devres_free(dr);
>> + return nv;
>> + }
>> +
>> + *dr = nv;
>> + devres_add(config->dev, dr);
>> +
>> + return nv;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_nvmem_register);
>> +
>