diff mbox

[v4] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

Message ID 1496829127-28375-1-git-send-email-kbingham@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kieran Bingham June 7, 2017, 9:52 a.m. UTC
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Return NULL, if a null entity is parsed for it's v4l2_subdev

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

---
Not sure if this patch ever made it out of my mailbox:

Here's the respin with the parameter evaluated only once.

v4:
 - Improve macro usage to evaluate ent only once

 include/media/v4l2-subdev.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Sakari Ailus June 7, 2017, 11:16 a.m. UTC | #1
On Wed, Jun 07, 2017 at 10:52:07AM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Return NULL, if a null entity is parsed for it's v4l2_subdev
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Mauro Carvalho Chehab June 8, 2017, 6 p.m. UTC | #2
Em Wed,  7 Jun 2017 10:52:07 +0100
Kieran Bingham <kbingham@kernel.org> escreveu:

> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Return NULL, if a null entity is parsed for it's v4l2_subdev
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Could you please improve this patch description?

I'm unsure if this is a bug fix, or some sort of feature...

On what situations would a null entity be passed to this function?

Regards,
Mauro

> 
> ---
> Not sure if this patch ever made it out of my mailbox:
> 
> Here's the respin with the parameter evaluated only once.
> 
> v4:
>  - Improve macro usage to evaluate ent only once
> 
>  include/media/v4l2-subdev.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a40760174797..0f92ebd2d710 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -826,8 +826,15 @@ struct v4l2_subdev {
>  	struct v4l2_subdev_platform_data *pdata;
>  };
>  
> -#define media_entity_to_v4l2_subdev(ent) \
> -	container_of(ent, struct v4l2_subdev, entity)
> +#define media_entity_to_v4l2_subdev(ent)				\
> +({									\
> +	typeof(ent) __me_sd_ent = (ent);				\
> +									\
> +	__me_sd_ent ?							\
> +		container_of(__me_sd_ent, struct v4l2_subdev, entity) :	\
> +		NULL;							\
> +})
> +
>  #define vdev_to_v4l2_subdev(vdev) \
>  	((struct v4l2_subdev *)video_get_drvdata(vdev))
>  



Thanks,
Mauro
Sakari Ailus June 8, 2017, 7:32 p.m. UTC | #3
Hi Mauro,

On Thu, Jun 08, 2017 at 03:00:22PM -0300, Mauro Carvalho Chehab wrote:
> Em Wed,  7 Jun 2017 10:52:07 +0100
> Kieran Bingham <kbingham@kernel.org> escreveu:
> 
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Could you please improve this patch description?
> 
> I'm unsure if this is a bug fix, or some sort of feature...
> 
> On what situations would a null entity be passed to this function?

I actually proposed this patch. This change is simply for convenience ---
the caller doesn't need to make sure the subdev is non-NULL, possibly
obtained from e.g. media_entity_remote_pad() which returns NULL all links to
the pad are disabled. This is a recurring pattern, and making this change
avoids an additional check.

Having something along these lines in the patch description wouldn't hurt.
Mauro Carvalho Chehab June 8, 2017, 8:10 p.m. UTC | #4
Em Thu, 8 Jun 2017 22:32:10 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Thu, Jun 08, 2017 at 03:00:22PM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed,  7 Jun 2017 10:52:07 +0100
> > Kieran Bingham <kbingham@kernel.org> escreveu:
> >   
> > > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > 
> > > Return NULL, if a null entity is parsed for it's v4l2_subdev
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>  
> > 
> > Could you please improve this patch description?
> > 
> > I'm unsure if this is a bug fix, or some sort of feature...
> > 
> > On what situations would a null entity be passed to this function?  
> 
> I actually proposed this patch. This change is simply for convenience ---
> the caller doesn't need to make sure the subdev is non-NULL, possibly
> obtained from e.g. media_entity_remote_pad() which returns NULL all links to
> the pad are disabled. This is a recurring pattern, and making this change
> avoids an additional check.
> 
> Having something along these lines in the patch description wouldn't hurt.

Patch added, with a description based on the above.

Thanks!

Mauro
Kieran Bingham June 9, 2017, 10:03 a.m. UTC | #5
Hi Sakari, Mauro,

On 08/06/17 21:10, Mauro Carvalho Chehab wrote:
> Em Thu, 8 Jun 2017 22:32:10 +0300
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
>> Hi Mauro,
>>
>> On Thu, Jun 08, 2017 at 03:00:22PM -0300, Mauro Carvalho Chehab wrote:
>>> Em Wed,  7 Jun 2017 10:52:07 +0100
>>> Kieran Bingham <kbingham@kernel.org> escreveu:
>>>   
>>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>>
>>>> Return NULL, if a null entity is parsed for it's v4l2_subdev
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>  
>>>
>>> Could you please improve this patch description?
>>>
>>> I'm unsure if this is a bug fix, or some sort of feature...
>>>
>>> On what situations would a null entity be passed to this function? 

Sorry for not being clear enough there ...

>>
>> I actually proposed this patch. This change is simply for convenience ---
>> the caller doesn't need to make sure the subdev is non-NULL, possibly
>> obtained from e.g. media_entity_remote_pad() which returns NULL all links to
>> the pad are disabled. This is a recurring pattern, and making this change
>> avoids an additional check.
>>
>> Having something along these lines in the patch description wouldn't hurt.

Yes, the above looks good ...

> Patch added, with a description based on the above.

And thank you :)

Regards
--
Kieran
diff mbox

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index a40760174797..0f92ebd2d710 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -826,8 +826,15 @@  struct v4l2_subdev {
 	struct v4l2_subdev_platform_data *pdata;
 };
 
-#define media_entity_to_v4l2_subdev(ent) \
-	container_of(ent, struct v4l2_subdev, entity)
+#define media_entity_to_v4l2_subdev(ent)				\
+({									\
+	typeof(ent) __me_sd_ent = (ent);				\
+									\
+	__me_sd_ent ?							\
+		container_of(__me_sd_ent, struct v4l2_subdev, entity) :	\
+		NULL;							\
+})
+
 #define vdev_to_v4l2_subdev(vdev) \
 	((struct v4l2_subdev *)video_get_drvdata(vdev))