[RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Sakari Ailus]

Rework the macros for accessing subdev try formats to work meaningfully
and relatively safely without V4L2 sub-device uAPI (and without MC). This
is done by simply reverting to accessing the pad number zero if
CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel warning
if the pad is non-zero in that case.

The functions are seen useful if subdev uAPI is disabled for drivers that
can work without the Kconfig option but benefit from the option if it's
enabled.

As a by-product, the patch simplifies individual inline functions by
removing two lines of code from each of them and moving the functionality
to a common macro.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi guys,

This might not be pretty but should provide some comfort for drivers
working with different Kconfig options. Comments are welcome...

 include/media/v4l2-subdev.h | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e1e3c18c3fd6..3328f302326b 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
 	container_of(fh, struct v4l2_subdev_fh, vfh)
 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
+	(WARN_ON(!(__cfg)) ? NULL :		\
+	 ((__sd)->entity.graph_obj.mdev ?				\
+	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
+	   NULL : &(__cfg)[__pad].__field) :				\
+	  &(__cfg)[WARN_ON(__pad) && 0].__field))
+#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
+#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
+	(WARN_ON(!(__cfg)) ? NULL :					\
+	 &(__cfg)[WARN_ON(__pad) && 0].__field)
+#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */
 
 /**
  * v4l2_subdev_get_try_format - ancillary routine to call
@@ -944,9 +955,7 @@ static inline struct v4l2_mbus_framefmt
 			    struct v4l2_subdev_pad_config *cfg,
 			    unsigned int pad)
 {
-	if (WARN_ON(pad >= sd->entity.num_pads))
-		pad = 0;
-	return &cfg[pad].try_fmt;
+	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_fmt);
 }
 
 /**
@@ -962,9 +971,7 @@ static inline struct v4l2_rect
 			  struct v4l2_subdev_pad_config *cfg,
 			  unsigned int pad)
 {
-	if (WARN_ON(pad >= sd->entity.num_pads))
-		pad = 0;
-	return &cfg[pad].try_crop;
+	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_crop);
 }
 
 /**
@@ -980,11 +987,8 @@ static inline struct v4l2_rect
 			     struct v4l2_subdev_pad_config *cfg,
 			     unsigned int pad)
 {
-	if (WARN_ON(pad >= sd->entity.num_pads))
-		pad = 0;
-	return &cfg[pad].try_compose;
+	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_compose);
 }
-#endif
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;
 
-- 
2.11.0

Re: [RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Janusz Krzysztofik]

Hi Sakari,

On Tuesday, June 4, 2019 7:57:31 PM CEST Sakari Ailus wrote:
> Rework the macros for accessing subdev try formats to work meaningfully
> and relatively safely without V4L2 sub-device uAPI (and without MC). This
> is done by simply reverting to accessing the pad number zero if
> CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel warning
> if the pad is non-zero in that case.
> 
> The functions are seen useful if subdev uAPI is disabled for drivers that
> can work without the Kconfig option but benefit from the option if it's
> enabled.

I'm not sure which drivers you (we) consider valid users of those functions.  
Subdevice drivers only? Or other drivers which interact with subdevices as 
well?  An answer to that question determines my possible other comments.

Thanks,
Janusz

> As a by-product, the patch simplifies individual inline functions by
> removing two lines of code from each of them and moving the functionality
> to a common macro.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi guys,
> 
> This might not be pretty but should provide some comfort for drivers
> working with different Kconfig options. Comments are welcome...
> 
>  include/media/v4l2-subdev.h | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index e1e3c18c3fd6..3328f302326b 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
>  	container_of(fh, struct v4l2_subdev_fh, vfh)
>  
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> +	(WARN_ON(!(__cfg)) ? NULL :		\
> +	 ((__sd)->entity.graph_obj.mdev ?				
\
> +	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
> +	   NULL : &(__cfg)[__pad].__field) :				
\
> +	  &(__cfg)[WARN_ON(__pad) && 0].__field))
> +#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> +	(WARN_ON(!(__cfg)) ? NULL :				
	\
> +	 &(__cfg)[WARN_ON(__pad) && 0].__field)
> +#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */
>  
>  /**
>   * v4l2_subdev_get_try_format - ancillary routine to call
> @@ -944,9 +955,7 @@ static inline struct v4l2_mbus_framefmt
>  			    struct v4l2_subdev_pad_config *cfg,
>  			    unsigned int pad)
>  {
> -	if (WARN_ON(pad >= sd->entity.num_pads))
> -		pad = 0;
> -	return &cfg[pad].try_fmt;
> +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_fmt);
>  }
>  
>  /**
> @@ -962,9 +971,7 @@ static inline struct v4l2_rect
>  			  struct v4l2_subdev_pad_config *cfg,
>  			  unsigned int pad)
>  {
> -	if (WARN_ON(pad >= sd->entity.num_pads))
> -		pad = 0;
> -	return &cfg[pad].try_crop;
> +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_crop);
>  }
>  
>  /**
> @@ -980,11 +987,8 @@ static inline struct v4l2_rect
>  			     struct v4l2_subdev_pad_config *cfg,
>  			     unsigned int pad)
>  {
> -	if (WARN_ON(pad >= sd->entity.num_pads))
> -		pad = 0;
> -	return &cfg[pad].try_compose;
> +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_compose);
>  }
> -#endif
>  
>  extern const struct v4l2_file_operations v4l2_subdev_fops;
>  
> 

Re: [RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Sakari Ailus]

Hi Janusz,

On Wed, Jun 05, 2019 at 09:33:41PM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
> 
> On Tuesday, June 4, 2019 7:57:31 PM CEST Sakari Ailus wrote:
> > Rework the macros for accessing subdev try formats to work meaningfully
> > and relatively safely without V4L2 sub-device uAPI (and without MC). This
> > is done by simply reverting to accessing the pad number zero if
> > CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel warning
> > if the pad is non-zero in that case.
> > 
> > The functions are seen useful if subdev uAPI is disabled for drivers that
> > can work without the Kconfig option but benefit from the option if it's
> > enabled.
> 
> I'm not sure which drivers you (we) consider valid users of those functions.  
> Subdevice drivers only? Or other drivers which interact with subdevices as 
> well?  An answer to that question determines my possible other comments.

These functions are only by drivers for the devices they control directly
only.

> 
> Thanks,
> Janusz
> 
> > As a by-product, the patch simplifies individual inline functions by
> > removing two lines of code from each of them and moving the functionality
> > to a common macro.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi guys,
> > 
> > This might not be pretty but should provide some comfort for drivers
> > working with different Kconfig options. Comments are welcome...
> > 
> >  include/media/v4l2-subdev.h | 24 ++++++++++++++----------
> >  1 file changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index e1e3c18c3fd6..3328f302326b 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
> >  	container_of(fh, struct v4l2_subdev_fh, vfh)
> >  
> >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > +	(WARN_ON(!(__cfg)) ? NULL :		\
> > +	 ((__sd)->entity.graph_obj.mdev ?				
> \
> > +	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
> > +	   NULL : &(__cfg)[__pad].__field) :				
> \
> > +	  &(__cfg)[WARN_ON(__pad) && 0].__field))
> > +#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > +	(WARN_ON(!(__cfg)) ? NULL :				
> 	\
> > +	 &(__cfg)[WARN_ON(__pad) && 0].__field)
> > +#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */
> >  
> >  /**
> >   * v4l2_subdev_get_try_format - ancillary routine to call
> > @@ -944,9 +955,7 @@ static inline struct v4l2_mbus_framefmt
> >  			    struct v4l2_subdev_pad_config *cfg,
> >  			    unsigned int pad)
> >  {
> > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > -		pad = 0;
> > -	return &cfg[pad].try_fmt;
> > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_fmt);
> >  }
> >  
> >  /**
> > @@ -962,9 +971,7 @@ static inline struct v4l2_rect
> >  			  struct v4l2_subdev_pad_config *cfg,
> >  			  unsigned int pad)
> >  {
> > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > -		pad = 0;
> > -	return &cfg[pad].try_crop;
> > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_crop);
> >  }
> >  
> >  /**
> > @@ -980,11 +987,8 @@ static inline struct v4l2_rect
> >  			     struct v4l2_subdev_pad_config *cfg,
> >  			     unsigned int pad)
> >  {
> > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > -		pad = 0;
> > -	return &cfg[pad].try_compose;
> > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_compose);
> >  }
> > -#endif
> >  
> >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> >  
> > 
> 
> 
> 
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Janusz Krzysztofik]

Hi Sakari,

On Thursday, June 6, 2019 3:56:42 PM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Wed, Jun 05, 2019 at 09:33:41PM +0200, Janusz Krzysztofik wrote:
> > Hi Sakari,
> > 
> > On Tuesday, June 4, 2019 7:57:31 PM CEST Sakari Ailus wrote:
> > > Rework the macros for accessing subdev try formats to work meaningfully
> > > and relatively safely without V4L2 sub-device uAPI (and without MC). This
> > > is done by simply reverting to accessing the pad number zero if
> > > CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel warning
> > > if the pad is non-zero in that case.
> > > 
> > > The functions are seen useful if subdev uAPI is disabled for drivers that
> > > can work without the Kconfig option but benefit from the option if it's
> > > enabled.
> > 
> > I'm not sure which drivers you (we) consider valid users of those functions.  
> > Subdevice drivers only? Or other drivers which interact with subdevices as 
> > well?  An answer to that question determines my possible other comments.
> 
> These functions are only by drivers for the devices they control directly
> only.

That's what I expected.

Exposing those functions to drivers not supporting V4L2 subdev uAPI is 
not a bad idea but it would make more sense to me if we also updated potential 
users.  Otherwise I just don't believe anyone will care for as long as a 
driver is not refreshed to support MC and V4L2 subdev uAPI.

> > ...
> > > As a by-product, the patch simplifies individual inline functions by
> > > removing two lines of code from each of them and moving the functionality
> > > to a common macro.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > Hi guys,
> > > 
> > > This might not be pretty but should provide some comfort for drivers
> > > working with different Kconfig options. Comments are welcome...
> > > 
> > >  include/media/v4l2-subdev.h | 24 ++++++++++++++----------
> > >  1 file changed, 14 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index e1e3c18c3fd6..3328f302326b 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
> > >  	container_of(fh, struct v4l2_subdev_fh, vfh)
> > >  
> > >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > > +	(WARN_ON(!(__cfg)) ? NULL :		\
> > > +	 ((__sd)->entity.graph_obj.mdev ?				
> > \strange
> > > +	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
> > > +	   NULL : &(__cfg)[__pad].__field) :				
> > \
> > > +	  &(__cfg)[WARN_ON(__pad) && 0].__field))
> > > +#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > > +	(WARN_ON(!(__cfg)) ? NULL :				
> > 	\
> > > +	 &(__cfg)[WARN_ON(__pad) && 0].__field)
> > > +#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */

I think that as long as we already have in place the same checks performed by 
v4l2_subdev_call() which seems the only user entry point to a subdevice 
driver, invalid cfg or pad values you are trying to catch here will never be 
passed unless the driver performs unusual operations and, moreover, is 
internally broken.

> > >  
> > >  /**
> > >   * v4l2_subdev_get_try_format - ancillary routine to call
> > > @@ -944,9 +955,7 @@ static inline struct v4l2_mbus_framefmt
> > >  			    struct v4l2_subdev_pad_config *cfg,
> > >  			    unsigned int pad)
> > >  {
> > > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > > -		pad = 0;

Dropping this check from here and below makes sense to me anyway, for the same 
reason as explained above.

> > > -	return &cfg[pad].try_fmt;
> > > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_fmt);

If we agree that correctness of cfg and pad has already been verified, this 
change and similar below, perfectly correct otherwise, seem of limited value 
to me.

Thanks,
Janusz

> > >  }
> > >  
> > >  /**
> > > @@ -962,9 +971,7 @@ static inline struct v4l2_rect
> > >  			  struct v4l2_subdev_pad_config *cfg,
> > >  			  unsigned int pad)
> > >  {
> > > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > > -		pad = 0;
> > > -	return &cfg[pad].try_crop;
> > > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_crop);
> > >  }
> > >  
> > >  /**
> > > @@ -980,11 +987,8 @@ static inline struct v4l2_rect
> > >  			     struct v4l2_subdev_pad_config *cfg,
> > >  			     unsigned int pad)
> > >  {
> > > -	if (WARN_ON(pad >= sd->entity.num_pads))
> > > -		pad = 0;
> > > -	return &cfg[pad].try_compose;
> > > +	return __v4l2_subdev_get_try_field(sd, cfg, pad, try_compose);
> > >  }
> > > -#endif
> > >  
> > >  extern const struct v4l2_file_operations v4l2_subdev_fops;
> > >  
> > > 
> > 
> > 
> > 
> > 
> 
> 

Re: [RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Sakari Ailus]

Hi Janusz,

On Thu, Jun 06, 2019 at 08:13:36PM +0200, Janusz Krzysztofik wrote:
> Hi Sakari,
> 
> On Thursday, June 6, 2019 3:56:42 PM CEST Sakari Ailus wrote:
> > Hi Janusz,
> > 
> > On Wed, Jun 05, 2019 at 09:33:41PM +0200, Janusz Krzysztofik wrote:
> > > Hi Sakari,
> > > 
> > > On Tuesday, June 4, 2019 7:57:31 PM CEST Sakari Ailus wrote:
> > > > Rework the macros for accessing subdev try formats to work meaningfully
> > > > and relatively safely without V4L2 sub-device uAPI (and without MC). This
> > > > is done by simply reverting to accessing the pad number zero if
> > > > CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel warning
> > > > if the pad is non-zero in that case.
> > > > 
> > > > The functions are seen useful if subdev uAPI is disabled for drivers that
> > > > can work without the Kconfig option but benefit from the option if it's
> > > > enabled.
> > > 
> > > I'm not sure which drivers you (we) consider valid users of those functions.  
> > > Subdevice drivers only? Or other drivers which interact with subdevices as 
> > > well?  An answer to that question determines my possible other comments.
> > 
> > These functions are only by drivers for the devices they control directly
> > only.
> 
> That's what I expected.
> 
> Exposing those functions to drivers not supporting V4L2 subdev uAPI is 
> not a bad idea but it would make more sense to me if we also updated potential 
> users.  Otherwise I just don't believe anyone will care for as long as a 
> driver is not refreshed to support MC and V4L2 subdev uAPI.

The primary users of these functions are drivers that do support subdev
uAPI. Some drivers can function that disabled, so making these functions
usable to those drivers in all cases simplifies those drivers.

> 
> > > ...
> > > > As a by-product, the patch simplifies individual inline functions by
> > > > removing two lines of code from each of them and moving the functionality
> > > > to a common macro.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > Hi guys,
> > > > 
> > > > This might not be pretty but should provide some comfort for drivers
> > > > working with different Kconfig options. Comments are welcome...
> > > > 
> > > >  include/media/v4l2-subdev.h | 24 ++++++++++++++----------
> > > >  1 file changed, 14 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index e1e3c18c3fd6..3328f302326b 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
> > > >  	container_of(fh, struct v4l2_subdev_fh, vfh)
> > > >  
> > > >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > > > +	(WARN_ON(!(__cfg)) ? NULL :		\
> > > > +	 ((__sd)->entity.graph_obj.mdev ?				
> > > \strange
> > > > +	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
> > > > +	   NULL : &(__cfg)[__pad].__field) :				
> > > \
> > > > +	  &(__cfg)[WARN_ON(__pad) && 0].__field))
> > > > +#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> > > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	\
> > > > +	(WARN_ON(!(__cfg)) ? NULL :				
> > > 	\
> > > > +	 &(__cfg)[WARN_ON(__pad) && 0].__field)
> > > > +#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */
> 
> I think that as long as we already have in place the same checks performed by 
> v4l2_subdev_call() which seems the only user entry point to a subdevice 
> driver, invalid cfg or pad values you are trying to catch here will never be 
> passed unless the driver performs unusual operations and, moreover, is 
> internally broken.

You can't rely on checks done by the v4l2_subdev_call macro as these
parameters do not come from the caller; they are set by the driver itself.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 1/1] v4l2-subdev: Rework subdev format and selection macros to work without MC

[Janusz Krzysztofik]

Hi Sakari,

On Monday, June 10, 2019 10:54:44 AM CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Thu, Jun 06, 2019 at 08:13:36PM +0200, Janusz Krzysztofik wrote:
> > Hi Sakari,
> > 
> > On Thursday, June 6, 2019 3:56:42 PM CEST Sakari Ailus wrote:
> > > Hi Janusz,
> > > 
> > > On Wed, Jun 05, 2019 at 09:33:41PM +0200, Janusz Krzysztofik wrote:
> > > > Hi Sakari,
> > > > 
> > > > On Tuesday, June 4, 2019 7:57:31 PM CEST Sakari Ailus wrote:
> > > > > Rework the macros for accessing subdev try formats to work
> > > > > meaningfully
> > > > > and relatively safely without V4L2 sub-device uAPI (and without MC).
> > > > > This
> > > > > is done by simply reverting to accessing the pad number zero if
> > > > > CONFIG_VIDEO_V4L2_SUBDEV_API isn't enabled, and emitting a kernel
> > > > > warning
> > > > > if the pad is non-zero in that case.
> > > > > 
> > > > > The functions are seen useful if subdev uAPI is disabled for drivers
> > > > > that
> > > > > can work without the Kconfig option but benefit from the option if
> > > > > it's
> > > > > enabled.
> > > > 
> > > > I'm not sure which drivers you (we) consider valid users of those
> > > > functions. Subdevice drivers only? Or other drivers which interact
> > > > with subdevices as well?  An answer to that question determines my
> > > > possible other comments.> > 
> > > These functions are only by drivers for the devices they control
> > > directly
> > > only.
> > 
> > That's what I expected.
> > 
> > Exposing those functions to drivers not supporting V4L2 subdev uAPI is
> > not a bad idea but it would make more sense to me if we also updated
> > potential users.  Otherwise I just don't believe anyone will care for as
> > long as a driver is not refreshed to support MC and V4L2 subdev uAPI.
> 
> The primary users of these functions are drivers that do support subdev
> uAPI. Some drivers can function that disabled, so making these functions
> usable to those drivers in all cases simplifies those drivers.

Indeed.  I didn't take that group of drivers into account.

> > > > ...
> > > > 
> > > > > As a by-product, the patch simplifies individual inline functions by
> > > > > removing two lines of code from each of them and moving the
> > > > > functionality
> > > > > to a common macro.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > > Hi guys,
> > > > > 
> > > > > This might not be pretty but should provide some comfort for drivers
> > > > > working with different Kconfig options. Comments are welcome...
> > > > > 
> > > > >  include/media/v4l2-subdev.h | 24 ++++++++++++++----------
> > > > >  1 file changed, 14 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/include/media/v4l2-subdev.h
> > > > > b/include/media/v4l2-subdev.h
> > > > > index e1e3c18c3fd6..3328f302326b 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -930,6 +930,17 @@ struct v4l2_subdev_fh {
> > > > > 
> > > > >  	container_of(fh, struct v4l2_subdev_fh, vfh)
> > > > >  
> > > > >  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > > > 
> > > > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	
\
> > > > > +	(WARN_ON(!(__cfg)) ? NULL :		\
> > > > > +	 ((__sd)->entity.graph_obj.mdev ?
> > > > 
> > > > \strange
> > > > 
> > > > > +	  (WARN_ON((__pad) >= (__sd)->entity.num_pads) ?		\
> > > > 
> > > > > +	   NULL : &(__cfg)[__pad].__field) :
> > > > \
> > > > 
> > > > > +	  &(__cfg)[WARN_ON(__pad) && 0].__field))
> > > > > +#else /* CONFIG_VIDEO_V4L2_SUBDEV_API */
> > > > > +#define __v4l2_subdev_get_try_field(__sd, __cfg, __pad, __field)	
\
> > > > 	
> > > > > +	(WARN_ON(!(__cfg)) ? NULL :
> > > > 	\
> > > > 	
> > > > > +	 &(__cfg)[WARN_ON(__pad) && 0].__field)
> > > > > +#endif /* !CONFIG_VIDEO_V4L2_SUBDEV_API */
> > 
> > I think that as long as we already have in place the same checks performed
> > by v4l2_subdev_call() which seems the only user entry point to a
> > subdevice driver, invalid cfg or pad values you are trying to catch here
> > will never be passed unless the driver performs unusual operations and,
> > moreover, is internally broken.
> 
> You can't rely on checks done by the v4l2_subdev_call macro as these
> parameters do not come from the caller; they are set by the driver itself.

If that' the case, you are right.

It looks like I'm missing some knowledge on V4L2 framework needed to provide a 
valuable review of your change, please ignore my comments.

Thanks,
Janusz