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

[Heiner Kallweit <hkallweit1@gmail.com>]

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@gmail.com>
>> ---
>>  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);
>> +
>