Re: [PATCHv3 1/8] extcon: Add resource-managed extcon register function

[Chanwoo Choi]

Hi,

On Sat, Apr 19, 2014 at 6:50 PM, Sangjung <sangjung.woo@xxxxxxxxxxx> wrote:
> Hi Chanwoo.
>
> Thanks for your comments. I also add my opinion too.
>
>
>
> On 04/19/2014 04:13 PM, Chanwoo Choi wrote:
>>
>> Hi Sangjung,
>>
>> On Fri, Apr 18, 2014 at 9:32 AM, Sangjung Woo <sangjung.woo@xxxxxxxxxxx>
>> wrote:
>>>
>>> Add resource-managed extcon device register function for convenience.
>>> For example, if a extcon device is attached with new
>>> devm_extcon_dev_register(), that extcon device is automatically
>>> unregistered on driver detach.
>>>
>>> Signed-off-by: Sangjung Woo <sangjung.woo@xxxxxxxxxxx>
>>> ---
>>>   drivers/extcon/extcon-class.c |   72
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/extcon.h        |   17 ++++++++++
>>>   2 files changed, 89 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-class.c
>>> b/drivers/extcon/extcon-class.c
>>> index 7ab21aa..e177edb6 100644
>>> --- a/drivers/extcon/extcon-class.c
>>> +++ b/drivers/extcon/extcon-class.c
>>> @@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>>>   }
>>>   EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>>>
>>> +
>>> +/*
>>> + * Device resource management
>>> + */
>>
>> This comment is un-necessary because this patchset(v3) remove 'struct
>> extcon_devres'.
>>
>>> +static void devm_extcon_dev_release(struct device *dev, void *res)
>>> +{
>>> +       struct extcon_dev *devres = res;
>>> +
>>> +       extcon_dev_unregister(devres);
>>
>> I prefer following function call withou defining separate 'devres'
>> variable.
>> But, this casting on the first argument is only for readability.
>>    extcon_dev_unregister((strcut extcon_dev *)res);
>>
> OK. I'll fix it.
>
>
>>> +}
>>> +
>>> +static int devm_extcon_dev_match(struct device *dev, void *res, void
>>> *data)
>>> +{
>>> +       struct extcon_dev *devres = res;
>>> +       struct extcon_dev *match = data;
>>> +       return devres == match;
>>
>> To simplify code, I think you could change checking code as following:
>>              return res == data;
>
> Right. Simple is better than others.
>
>
>>> +}
>>> +
>>> +/**
>>> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
>>> + * @dev:       device to allocate extcon device
>>> + * @edev:      the new extcon device to register
>>> + *
>>> + * Managed extcon_dev_register() function. If extcon device is attached
>>> with
>>> + * this function, that extcon device is automatically unregistered on
>>> driver
>>> + * detach. Internally this function calls extcon_dev_register()
>>> function.
>>> + * To get more information, refer that function.
>>> + *
>>> + * If extcon device is registered with this function and the device
>>> needs to be
>>> + * unregistered separately, devm_extcon_dev_unregister() should be used.
>>> + *
>>> + * RETURNS:
>>> + * 0 on success, negative error number on failure.
>>> + */
>>> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev
>>> *edev)
>>> +{
>>> +       struct extcon_dev *devres;
>>
>> The 'devres' in this function don't mean 'device resource structure'.
>> So I think it is not proper name.
>> I think you should use other general name (e.g.,  'ptr' or 'res'
>> instead of 'devres')
>>
>>> +       int ret;
>>> +
>>> +       devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres)
>>
>> Other subsytem used double pointer to get device resource from
>> devres_alloc() instead of
>> single pointer.(devres is single pointer)  I can't find subsystem
>> using single pointer of devm function.
>> First of all, We have to analyze the correct reason using only double
>> pointer instead of single pointer whether single pointer use is good
>> or not.
>
>
> IMO, other subsystem should return the memory pointer that is allocated by
> devres_alloc().
> However, in our case, we need not do that since the pointer is used only in
> extcon core.
> You can refer the way that I did to gpio subsystem (devm_gpio_request() in
> /drivers/gpio/devres.c).

As you comment, I checked 'devm_gpio_request' in drivers/gpio/devres.c
. There are a little difference between devm_extcon_dev_register and
devm_gpio_request.

The second argument (unsigned gpio) is not pointer type in
devm_gpio_request() But, devm_extcon_dev_register() needs the pointer
type for second argument(struct extcon_dev *edev).

>
>
>>> +                       GFP_KERNEL);
>>> +       if (!devres)
>>> +               return -ENOMEM;
>>> +
>>> +       ret = extcon_dev_register(edev);
>>> +       if (ret) {
>>> +               devres_free(devres);
>>> +               return ret;
>>> +       }
>>> +
>>> +       devres = edev;
>>> +       devres_add(dev, devres);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
>>> +
>>> +/**
>>> + * devm_extcon_dev_unregister() - Resource-managed
>>> extcon_dev_unregister()
>>> + * @dev:       device the extcon belongs to
>>> + * @edev:      the extcon device to unregister
>>> + *
>>> + * Unregister extcon device that is registered with
>>> devm_extcon_dev_register()
>>> + * function.
>>> + */
>>> +void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev
>>> *edev)
>>> +{
>>> +       WARN_ON(devres_release(dev, devm_extcon_dev_release,
>>> +                       devm_extcon_dev_match, edev));
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
>>> +
>>>   #ifdef CONFIG_OF
>>>   /*
>>>    * extcon_get_edev_by_phandle - Get the extcon device from devicetree
>>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>>> index f488145..35f3343 100644
>>> --- a/include/linux/extcon.h
>>> +++ b/include/linux/extcon.h
>>> @@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev
>>> *edev);
>>>   extern struct extcon_dev *extcon_get_extcon_dev(const char
>>> *extcon_name);
>>>
>>>   /*
>>> + * Resource-managed extcon device register function.
>>> + */
>>> +extern int devm_extcon_dev_register(struct device *dev,
>>> +                               struct extcon_dev *edev);
>>> +extern void devm_extcon_dev_unregister(struct device *dev,
>>> +                               struct extcon_dev *edev);
>>
>> I prefer that this function meet indentation of function definition
>> needs as following:
>>
>> extern int devm_extcon_dev_register(struct device *dev,
>>
>> struct extcon_dev *edev);
>> extern void devm_extcon_dev_unregister(struct device *dev,
>>
>>    struct extcon_dev *edev);
>
>
> I have a question about the indentation issue
> since my Thunderbird email client does not show me the below line tidy.
>
> You want me to adjust the start point of the second line
> to the right after parentheses of the first line. right?

Right.

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/