Re: [PATCH 6/7] media: au0828 change to use Managed Media Controller API

2015-07-20 Thread Shuah Khan 
On 07/20/2015 01:01 PM, Dan Carpenter wrote:
> On Mon, Jul 20, 2015 at 09:55:50AM -0600, Shuah Khan wrote:
  #ifdef CONFIG_MEDIA_CONTROLLER
 -  if (dev->media_dev) {
 -  media_device_unregister(dev->media_dev);
 -  kfree(dev->media_dev);
 -  dev->media_dev = NULL;
 +  if (dev->media_dev &&
 +  media_devnode_is_registered(&dev->media_dev->devnode)) {
 +  media_device_unregister_entity_notify(dev->media_dev,
 +  &dev->entity_notify);
 +  media_device_unregister(dev->media_dev);
 +  dev->media_dev = NULL;
>>>
>>>
>>> The indenting is slightly off here.  It should be:
>>>
>>> if (dev->media_dev &&
>>> media_devnode_is_registered(&dev->media_dev->devnode)) {
>>> media_device_unregister_entity_notify(dev->media_dev,
>>>   &dev->entity_notify);
>>> media_device_unregister(dev->media_dev);
>>> dev->media_dev = NULL;
>>> }
>>>
>>> Aligning if statements using spaces like that is nicer and checkpatch.pl
>>> won't complain.
>>>
>>
>> Yeah. I try to do that whenever. In this case, if I align, the line
>> becomes longer than 80 chars adding more things to worry about. In
>> such cases, I tend to not worry about aligning.
> 
> The main thing is that the body of the if statement is intended 16
> spaces instead of 8.  Otherwise I wouldn't have commented.
> 

Yes. I see what you are saying. I will fix it in v2 series.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] media: au0828 change to use Managed Media Controller API

2015-07-20 Thread Dan Carpenter 
On Mon, Jul 20, 2015 at 09:55:50AM -0600, Shuah Khan wrote:
> >>  #ifdef CONFIG_MEDIA_CONTROLLER
> >> -  if (dev->media_dev) {
> >> -  media_device_unregister(dev->media_dev);
> >> -  kfree(dev->media_dev);
> >> -  dev->media_dev = NULL;
> >> +  if (dev->media_dev &&
> >> +  media_devnode_is_registered(&dev->media_dev->devnode)) {
> >> +  media_device_unregister_entity_notify(dev->media_dev,
> >> +  &dev->entity_notify);
> >> +  media_device_unregister(dev->media_dev);
> >> +  dev->media_dev = NULL;
> > 
> > 
> > The indenting is slightly off here.  It should be:
> > 
> > if (dev->media_dev &&
> > media_devnode_is_registered(&dev->media_dev->devnode)) {
> > media_device_unregister_entity_notify(dev->media_dev,
> >   &dev->entity_notify);
> > media_device_unregister(dev->media_dev);
> > dev->media_dev = NULL;
> > }
> > 
> > Aligning if statements using spaces like that is nicer and checkpatch.pl
> > won't complain.
> > 
> 
> Yeah. I try to do that whenever. In this case, if I align, the line
> becomes longer than 80 chars adding more things to worry about. In
> such cases, I tend to not worry about aligning.

The main thing is that the body of the if statement is intended 16
spaces instead of 8.  Otherwise I wouldn't have commented.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] media: au0828 change to use Managed Media Controller API

2015-07-20 Thread Shuah Khan 
On 07/20/2015 02:42 AM, Dan Carpenter wrote:
> Sorry for this nit-pick and some time after you sent this patch.  It's
> not a redo the patch" complaint, it's something that could be fixed
> later.
> 
> On Tue, Jul 14, 2015 at 06:34:05PM -0600, Shuah Khan wrote:
>> @@ -131,10 +132,12 @@ static void au0828_unregister_media_device(struct 
>> au0828_dev *dev)
>>  {
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>> -if (dev->media_dev) {
>> -media_device_unregister(dev->media_dev);
>> -kfree(dev->media_dev);
>> -dev->media_dev = NULL;
>> +if (dev->media_dev &&
>> +media_devnode_is_registered(&dev->media_dev->devnode)) {
>> +media_device_unregister_entity_notify(dev->media_dev,
>> +&dev->entity_notify);
>> +media_device_unregister(dev->media_dev);
>> +dev->media_dev = NULL;
> 
> 
> The indenting is slightly off here.  It should be:
> 
>   if (dev->media_dev &&
>   media_devnode_is_registered(&dev->media_dev->devnode)) {
>   media_device_unregister_entity_notify(dev->media_dev,
> &dev->entity_notify);
>   media_device_unregister(dev->media_dev);
>   dev->media_dev = NULL;
>   }
> 
> Aligning if statements using spaces like that is nicer and checkpatch.pl
> won't complain.
> 

Yeah. I try to do that whenever. In this case, if I align, the line
becomes longer than 80 chars adding more things to worry about. In
such cases, I tend to not worry about aligning.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] media: au0828 change to use Managed Media Controller API

2015-07-20 Thread Dan Carpenter 
Sorry for this nit-pick and some time after you sent this patch.  It's
not a redo the patch" complaint, it's something that could be fixed
later.

On Tue, Jul 14, 2015 at 06:34:05PM -0600, Shuah Khan wrote:
> @@ -131,10 +132,12 @@ static void au0828_unregister_media_device(struct 
> au0828_dev *dev)
>  {
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
> - if (dev->media_dev) {
> - media_device_unregister(dev->media_dev);
> - kfree(dev->media_dev);
> - dev->media_dev = NULL;
> + if (dev->media_dev &&
> + media_devnode_is_registered(&dev->media_dev->devnode)) {
> + media_device_unregister_entity_notify(dev->media_dev,
> + &dev->entity_notify);
> + media_device_unregister(dev->media_dev);
> + dev->media_dev = NULL;


The indenting is slightly off here.  It should be:

if (dev->media_dev &&
media_devnode_is_registered(&dev->media_dev->devnode)) {
media_device_unregister_entity_notify(dev->media_dev,
  &dev->entity_notify);
media_device_unregister(dev->media_dev);
dev->media_dev = NULL;
}

Aligning if statements using spaces like that is nicer and checkpatch.pl
won't complain.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html