Re: [PATCH] [media] media: rename media unregister function

[Shuah Khan]

On 03/18/2016 07:05 AM, Mauro Carvalho Chehab wrote:
> Now that media_device_unregister() also does a cleanup, rename it
> to media_device_unregister_cleanup().
> 
> Signed-off-by: Mauro Carvalho Chehab 

I think adding cleanup is redundant. media_device_unregister()
would imply that there has to be some cleanup releasing resources.
I wouldn't make this change.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] media: rename media unregister function

[Javier Martinez Canillas]

Hello Mauro,

On 03/18/2016 10:05 AM, Mauro Carvalho Chehab wrote:
> Now that media_device_unregister() also does a cleanup, rename it
> to media_device_unregister_cleanup().
>

I believe there should be a Suggested-by Sakari Ailus tag here.
 
> Signed-off-by: Mauro Carvalho Chehab 

The patch looks good and I agree that makes things more clear.

Reviewed-by: Javier Martinez Canillas 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] media: rename media unregister function

[Mauro Carvalho Chehab]

Em Fri, 18 Mar 2016 16:20:19 +0200
Sakari Ailus  escreveu:

> Shuah Khan wrote:
> > On 03/18/2016 08:12 AM, Javier Martinez Canillas wrote:  
> >> Hello Shuah,
> >>
> >> On 03/18/2016 11:01 AM, Shuah Khan wrote:  
> >>> On 03/18/2016 07:05 AM, Mauro Carvalho Chehab wrote:  
>  Now that media_device_unregister() also does a cleanup, rename it
>  to media_device_unregister_cleanup().
> 
>  Signed-off-by: Mauro Carvalho Chehab   
> >>>
> >>> I think adding cleanup is redundant. media_device_unregister()
> >>> would imply that there has to be some cleanup releasing resources.
> >>> I wouldn't make this change.
> >>>  
> >>
> >> Problem is that there is a media_device_init() and media_device_register(),
> >> so having both unregister and cleanup in this function will make very clear
> >> that a single function is the counter part of the previous two operations.
> >>
> > 
> > Yes. I realized that this change is motivated by the fact that there is
> > the media_device_init() and we had the counterpart media_device_cleanup()
> > as an exported function. I still think there is no need to make the change
> > to add _cleanup() at the end of media_device_unregister(). It can be handled
> > in API documentation that it does both.  
> 
> I think that's a bad idea. People will only read the documentation when
> something doesn't work. In this case it's easy to miss that.

After thinking about that, I guess the best is to use kref only
if the media_device_*devres functions are used. With this, we don't
need to touch at media_device_cleanup().

Just the patch to the ML:
https://patchwork.linuxtv.org/patch/33533/

It was tested with HVR-950Q.

Regards,
Mauro
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] media: rename media unregister function

[Shuah Khan]

On 03/18/2016 08:12 AM, Javier Martinez Canillas wrote:
> Hello Shuah,
> 
> On 03/18/2016 11:01 AM, Shuah Khan wrote:
>> On 03/18/2016 07:05 AM, Mauro Carvalho Chehab wrote:
>>> Now that media_device_unregister() also does a cleanup, rename it
>>> to media_device_unregister_cleanup().
>>>
>>> Signed-off-by: Mauro Carvalho Chehab 
>>
>> I think adding cleanup is redundant. media_device_unregister()
>> would imply that there has to be some cleanup releasing resources.
>> I wouldn't make this change.
>>
> 
> Problem is that there is a media_device_init() and media_device_register(),
> so having both unregister and cleanup in this function will make very clear
> that a single function is the counter part of the previous two operations.
>  

Yes. I realized that this change is motivated by the fact that there is
the media_device_init() and we had the counterpart media_device_cleanup()
as an exported function. I still think there is no need to make the change
to add _cleanup() at the end of media_device_unregister(). It can be handled
in API documentation that it does both.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] media: rename media unregister function

[Sakari Ailus]

Shuah Khan wrote:
> On 03/18/2016 08:12 AM, Javier Martinez Canillas wrote:
>> Hello Shuah,
>>
>> On 03/18/2016 11:01 AM, Shuah Khan wrote:
>>> On 03/18/2016 07:05 AM, Mauro Carvalho Chehab wrote:
 Now that media_device_unregister() also does a cleanup, rename it
 to media_device_unregister_cleanup().

 Signed-off-by: Mauro Carvalho Chehab 
>>>
>>> I think adding cleanup is redundant. media_device_unregister()
>>> would imply that there has to be some cleanup releasing resources.
>>> I wouldn't make this change.
>>>
>>
>> Problem is that there is a media_device_init() and media_device_register(),
>> so having both unregister and cleanup in this function will make very clear
>> that a single function is the counter part of the previous two operations.
>>  
> 
> Yes. I realized that this change is motivated by the fact that there is
> the media_device_init() and we had the counterpart media_device_cleanup()
> as an exported function. I still think there is no need to make the change
> to add _cleanup() at the end of media_device_unregister(). It can be handled
> in API documentation that it does both.

I think that's a bad idea. People will only read the documentation when
something doesn't work. In this case it's easy to miss that.

-- 
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [media] media: rename media unregister function

[Javier Martinez Canillas]

Hello Shuah,

On 03/18/2016 11:01 AM, Shuah Khan wrote:
> On 03/18/2016 07:05 AM, Mauro Carvalho Chehab wrote:
>> Now that media_device_unregister() also does a cleanup, rename it
>> to media_device_unregister_cleanup().
>>
>> Signed-off-by: Mauro Carvalho Chehab 
> 
> I think adding cleanup is redundant. media_device_unregister()
> would imply that there has to be some cleanup releasing resources.
> I wouldn't make this change.
>

Problem is that there is a media_device_init() and media_device_register(),
so having both unregister and cleanup in this function will make very clear
that a single function is the counter part of the previous two operations.
 
> thanks,
> -- Shuah
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel