Re: [PATCH 06/22] [media] tvp5150: add FORMAT_TRY support for get/set selection handlers

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Mauro,

during my work for a v2 series, I prepared two patches which fix this in a
common way. The first will fix the compiler break and the second will
fix a missing dependency, since those helper functions require the
media_entity which is only available if MEDIA_CONTROLLER is enabled.

I attached both inline as an RFC, can give me your feedback?

I removed the devicetree guys to aviod noise.

On 18-07-30 21:01, Mauro Carvalho Chehab wrote:
> Em Thu, 28 Jun 2018 18:20:38 +0200
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > Since commit 10d5509c8d50 ("[media] v4l2: remove g/s_crop from video ops")
> > the 'which' field for set/get_selection must be FORMAT_ACTIVE. There is
> > no way to try different selections. The patch adds a helper function to
> > select the correct selection memory space (sub-device file handle or
> > driver state) which will be set/returned.
> > 
> > The TVP5150 AVID will be updated if the 'which' field is FORMAT_ACTIVE
> > and the requested selection rectangle differs from the already set one.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/tvp5150.c | 107 ++++++++++++++++++++++++------------
> >  1 file changed, 73 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > index d150487cc2d1..29eaf8166f25 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -18,6 +18,7 @@
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-mc.h>
> > +#include <media/v4l2-rect.h>
> >  
> >  #include "tvp5150_reg.h"
> >  
> > @@ -846,20 +847,38 @@ static v4l2_std_id tvp5150_read_std(struct v4l2_subdev *sd)
> >  	}
> >  }
> >  
> > +static struct v4l2_rect *
> > +__tvp5150_get_pad_crop(struct tvp5150 *decoder,
> > +		       struct v4l2_subdev_pad_config *cfg, unsigned int pad,
> > +		       enum v4l2_subdev_format_whence which)
> > +{
> > +	switch (which) {
> > +	case V4L2_SUBDEV_FORMAT_TRY:
> > +		return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad);
> 
> This is not ok. It causes compilation breakage if the subdev API is not
> selected:
> 
> drivers/media/i2c/tvp5150.c: In function ‘__tvp5150_get_pad_crop’:
> drivers/media/i2c/tvp5150.c:857:10: error: implicit declaration of function ‘v4l2_subdev_get_try_crop’; did you mean ‘v4l2_subdev_has_op’? [-Werror=implicit-function-declaration]
>    return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad);
>           ^~~~~~~~~~~~~~~~~~~~~~~~
>           v4l2_subdev_has_op
> drivers/media/i2c/tvp5150.c:857:10: warning: returning ‘int’ from a function with return type ‘struct v4l2_rect *’ makes pointer from integer without a cast [-Wint-conversion]
>    return v4l2_subdev_get_try_crop(&decoder->sd, cfg, pad);
>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> The logic should keep working both with and without subdev API.
> 

[ snip ]

>From d87554df5d1c6d609e44852a1cb998a6f5d99601 Mon Sep 17 00:00:00 2001
From: Marco Felsch <m.felsch@pengutronix.de>
Date: Wed, 8 Aug 2018 14:27:46 +0200
Subject: [RFC] [media] v4l2-subdev: add stubs for v4l2_subdev_get_try_*

In case of missing CONFIG_VIDEO_V4L2_SUBDEV_API those helpers aren't
available. So each driver have to add ifdefs around those helpers or
add the CONFIG_VIDEO_V4L2_SUBDEV_API as dependcy.

Make these helpers available in case of CONFIG_VIDEO_V4L2_SUBDEV_API
isn't set to avoid ifdefs. This approach is less error prone too.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 include/media/v4l2-subdev.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 9102d6ca566e..ce48f1fcf295 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -912,8 +912,6 @@ struct v4l2_subdev_fh {
 #define to_v4l2_subdev_fh(fh)	\
 	container_of(fh, struct v4l2_subdev_fh, vfh)
 
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-
 /**
  * v4l2_subdev_get_try_format - ancillary routine to call
  *	&struct v4l2_subdev_pad_config->try_fmt
@@ -927,9 +925,13 @@ static inline struct v4l2_mbus_framefmt
 			    struct v4l2_subdev_pad_config *cfg,
 			    unsigned int pad)
 {
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
 	return &cfg[pad].try_fmt;
+#else
+	return NULL;
+#endif
 }
 
 /**
@@ -945,9 +947,13 @@ static inline struct v4l2_rect
 			  struct v4l2_subdev_pad_config *cfg,
 			  unsigned int pad)
 {
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
 	return &cfg[pad].try_crop;
+#else
+	return NULL;
+#endif
 }
 
 /**
@@ -963,11 +969,14 @@ static inline struct v4l2_rect
 			     struct v4l2_subdev_pad_config *cfg,
 			     unsigned int pad)
 {
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	if (WARN_ON(pad >= sd->entity.num_pads))
 		pad = 0;
 	return &cfg[pad].try_compose;
-}
+#else
+	return NULL;
 #endif
+}
 
 extern const struct v4l2_file_operations v4l2_subdev_fops;
 
-- 
2.18.0

>From 6abc7599fe6bdb2b8bef84fd90417d69aafb4a33 Mon Sep 17 00:00:00 2001
From: Marco Felsch <m.felsch@pengutronix.de>
Date: Wed, 8 Aug 2018 14:47:44 +0200
Subject: [RFC] [media] v4l2-subdev: fix v4l2_subdev_get_try_* dependency

These helpers make us of the media-controller entity which is only
available if the CONFIG_MEDIA_CONTROLLER is enabled.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 include/media/v4l2-subdev.h | 100 ++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 50 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index ce48f1fcf295..79c066934ad2 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -912,6 +912,56 @@ struct v4l2_subdev_fh {
 #define to_v4l2_subdev_fh(fh)	\
 	container_of(fh, struct v4l2_subdev_fh, vfh)
 
+extern const struct v4l2_file_operations v4l2_subdev_fops;
+
+/**
+ * v4l2_set_subdevdata - Sets V4L2 dev private device data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @p: pointer to the private device data to be stored.
+ */
+static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
+{
+	sd->dev_priv = p;
+}
+
+/**
+ * v4l2_get_subdevdata - Gets V4L2 dev private device data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ *
+ * Returns the pointer to the private device data to be stored.
+ */
+static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
+{
+	return sd->dev_priv;
+}
+
+/**
+ * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ * @p: pointer to the private data to be stored.
+ */
+static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
+{
+	sd->host_priv = p;
+}
+
+/**
+ * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
+ *
+ * @sd: pointer to &struct v4l2_subdev
+ *
+ * Returns the pointer to the private host data to be stored.
+ */
+static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
+{
+	return sd->host_priv;
+}
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+
 /**
  * v4l2_subdev_get_try_format - ancillary routine to call
  *	&struct v4l2_subdev_pad_config->try_fmt
@@ -978,56 +1028,6 @@ static inline struct v4l2_rect
 #endif
 }
 
-extern const struct v4l2_file_operations v4l2_subdev_fops;
-
-/**
- * v4l2_set_subdevdata - Sets V4L2 dev private device data
- *
- * @sd: pointer to &struct v4l2_subdev
- * @p: pointer to the private device data to be stored.
- */
-static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
-{
-	sd->dev_priv = p;
-}
-
-/**
- * v4l2_get_subdevdata - Gets V4L2 dev private device data
- *
- * @sd: pointer to &struct v4l2_subdev
- *
- * Returns the pointer to the private device data to be stored.
- */
-static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
-{
-	return sd->dev_priv;
-}
-
-/**
- * v4l2_set_subdev_hostdata - Sets V4L2 dev private host data
- *
- * @sd: pointer to &struct v4l2_subdev
- * @p: pointer to the private data to be stored.
- */
-static inline void v4l2_set_subdev_hostdata(struct v4l2_subdev *sd, void *p)
-{
-	sd->host_priv = p;
-}
-
-/**
- * v4l2_get_subdev_hostdata - Gets V4L2 dev private data
- *
- * @sd: pointer to &struct v4l2_subdev
- *
- * Returns the pointer to the private host data to be stored.
- */
-static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
-{
-	return sd->host_priv;
-}
-
-#ifdef CONFIG_MEDIA_CONTROLLER
-
 /**
  * v4l2_subdev_link_validate_default - validates a media link
  *
-- 
2.18.0

Kind regards,
Marco