Re: [PATCH 3/3] extcon: Simplify extcon_dev_register() prototype by removing unnecessary parameter

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Greg, 
On 09/09/2013 06:51 AM, Greg KH wrote:
> On Wed, Sep 04, 2013 at 02:18:30PM +0900, Chanwoo Choi wrote:
>> On 09/04/2013 10:16 AM, Greg KH wrote:
>>> On Wed, Sep 04, 2013 at 09:17:02AM +0900, Chanwoo Choi wrote:
>>>> On 09/04/2013 12:57 AM, Greg KH wrote:
>>>>> On Mon, Sep 02, 2013 at 10:13:44AM +0900, Chanwoo Choi wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 09/02/2013 09:40 AM, Greg KH wrote:
>>>>>>> On Mon, Sep 02, 2013 at 09:20:08AM +0900, Chanwoo Choi wrote:
>>>>>>>> This patch remove extcon_dev_register()'s second parameter which means
>>>>>>>> the pointer of parent device to simplify prototype of this function.
>>>>>>>
>>>>>>> No, please don't.  You want the parent to be passed in, as the core
>>>>>>> needs it when it is registered with the system, otherwise it will not
>>>>>>> show up in sysfs properly (i.e. you can't set it afterwards.)
>>>>>>
>>>>>> Currently, each extcon driver have allocated memory for extcon device
>>>>>> by using devm_kzalloc() in each extcon device driver.
>>>>>
>>>>> That seems backwards, the extcon core should be the one doing the
>>>>> allocation, and ownership of the device, like all other subsystem cores
>>>>> do.  That makes the driver logic much simpler, and the lifetime
>>>>> ownership correct (i.e. what happens when a device is unbound from a
>>>>> driver by userspace?  The driver can't control the device memory
>>>>> anymore...)
>>>>>
>>>>
>>>> OK,
>>>> The extcon core will control memory allocation instead of extcon device driver
>>>> as following.
>>>> - devm_extcon_allocate_device(struct device *dev)
>>>
>>> Huh?  Why do you need a devm allocator?  This is a "real" child device,
>>> just create it with a "extcon_create_device()" or some such call, like
>>> all other busses do?
>>
>> I refer Input/IIO subsystem to check the process of device registration.
>> Input subsystem has following functions for memory allocation of input device
>> and input device registration.
>>
>> Input subsystem
>> - struct input_dev *devm_input_allocate_device(struct device *device)
>> - struct input_dev *input_allocate_device(void)
>> - int input_register_device(struct input_dev *dev)
>> drivers/input/input.c
>>
>> devm_input_allocate_device()/input_allocate_device() can allocate
>> memory for input device in input core. And then created input device
>> pass input_register_device() as parameter.
>>
>> So, input device driver haven't executed kmalloc() or devm_kmalloc()
>> to allocate memory of input device by using input_allocate_device()/
>> devm_input_allocate_device().
>>
>>
>> Also,IIO subsystem has separate iio_device_alloc() function
>> to allocate memory for iio device. But IIO subsystem hasn't
>> "devm_" allocator. So, If iio device driver fail initialization
>> in *_probe, should execute iio_device_free() to free allocated memory.
>>
>> IIO Subsystem
>> - iio_device_alloc(int sizeof_priv)
>> - iio_device_register(struct iio_dev *indio_dev);
>> drivers/iio/industrialio-core.c
>>
>>
>> So, I think extcon subsystem need extcon_allocate_device()/devm_extcon_allocate_device()
>> to allocate for memory extcon device. To implement devm_extcon_allocate_device() function
>> , extcon subsystem need extcon_allocate_device() because devm_extcon_allocate_device()
>> must call extcon_allocate_device for memory allocation.
>>
>> If extcon_allocate_device()/devm_extcon_allocate_device() is implemented to extcon core,
>> I think extcon core can control the memory operation of extcon device.
>>
>> EXTCON Subsystem
>> - devm_extcon_allocate_device(struct device *dev)
>> - extcon_allocate_device(void)
>> - extcon_dev_register(struct extcon_dev *edev)
>>
>> If I'm wrong, please correct me. Thanks.
> 
> No, you are totally correct, I was wrong, nevermind, continue on doing a
> great job, my mistake :)
> 
> greg k-h

Thanks for your comment always.

Best Regards,
Chanwoo Choi