linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Any reason why media_entity_pads_init() isn't void?
@ 2016-03-14  8:25 Hans Verkuil
  2016-03-14 10:24 ` Mauro Carvalho Chehab
  2016-03-14 10:36 ` Sakari Ailus
  0 siblings, 2 replies; 8+ messages in thread
From: Hans Verkuil @ 2016-03-14  8:25 UTC (permalink / raw)
  To: Linux Media Mailing List, Laurent Pinchart, Mauro Carvalho Chehab

I was fixing a sparse warning in media_entity_pads_init() and I noticed that that
function always returns 0. Any reason why this can't be changed to a void function?

That return value is checked a zillion times in the media code. By making it void
it should simplify code all over.

See e.g. uvc_mc_init_entity in drivers/media/usb/uvc/uvc_entity.c: that whole
function can become a void function itself.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14  8:25 Any reason why media_entity_pads_init() isn't void? Hans Verkuil
@ 2016-03-14 10:24 ` Mauro Carvalho Chehab
  2016-03-14 10:36 ` Sakari Ailus
  1 sibling, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2016-03-14 10:24 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Linux Media Mailing List, Laurent Pinchart

Em Mon, 14 Mar 2016 09:25:51 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> I was fixing a sparse warning in media_entity_pads_init() and I noticed that that
> function always returns 0. Any reason why this can't be changed to a void function?
> 
> That return value is checked a zillion times in the media code. By making it void
> it should simplify code all over.
> 
> See e.g. uvc_mc_init_entity in drivers/media/usb/uvc/uvc_entity.c: that whole
> function can become a void function itself.


This function were called media_entity_init(), and it used to have a
code that would allocate the links array. The way it is right now, I
don't see any reason to keep returning an error code.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14  8:25 Any reason why media_entity_pads_init() isn't void? Hans Verkuil
  2016-03-14 10:24 ` Mauro Carvalho Chehab
@ 2016-03-14 10:36 ` Sakari Ailus
  2016-03-14 11:27   ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2016-03-14 10:36 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Linux Media Mailing List, Laurent Pinchart, Mauro Carvalho Chehab

Hi Hans,

On Mon, Mar 14, 2016 at 09:25:51AM +0100, Hans Verkuil wrote:
> I was fixing a sparse warning in media_entity_pads_init() and I noticed
> that that function always returns 0. Any reason why this can't be changed
> to a void function?

I was thinking of the same function but I had a different question: why
would one call this *after* entity->graph_obj.mdev is set? It is set by
media_device_register_entity(), but once mdev it's set, you're not expected
to call pads_init anymore...

I'm fine making this return void.

> 
> That return value is checked a zillion times in the media code. By making
> it void it should simplify code all over.
> 
> See e.g. uvc_mc_init_entity in drivers/media/usb/uvc/uvc_entity.c: that
> whole function can become a void function itself.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14 10:36 ` Sakari Ailus
@ 2016-03-14 11:27   ` Mauro Carvalho Chehab
  2016-03-14 11:43     ` Sakari Ailus
  0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2016-03-14 11:27 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Hans Verkuil, Linux Media Mailing List, Laurent Pinchart

Em Mon, 14 Mar 2016 12:36:44 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Hans,
> 
> On Mon, Mar 14, 2016 at 09:25:51AM +0100, Hans Verkuil wrote:
> > I was fixing a sparse warning in media_entity_pads_init() and I noticed
> > that that function always returns 0. Any reason why this can't be changed
> > to a void function?  
> 
> I was thinking of the same function but I had a different question: why
> would one call this *after* entity->graph_obj.mdev is set? It is set by
> media_device_register_entity(), but once mdev it's set, you're not expected
> to call pads_init anymore...

Ideally, drivers should *first* create mdev, and then create the
graph objects, as all objects should be registered at the mdev, in
order to get their object ID and to be registered at the mdev's object
lists.

However, some legacy drivers used to do just the reverse. So, we had
to add a code there, and at media_device_register_entity() to support
the drivers that postpone the creation of the mdev instance.

Once this gets fixed everywhere, we can remove the code that supports
the legacy behavior.

Regards,
Mauro

> 
> I'm fine making this return void.
> 
> > 
> > That return value is checked a zillion times in the media code. By making
> > it void it should simplify code all over.
> > 
> > See e.g. uvc_mc_init_entity in drivers/media/usb/uvc/uvc_entity.c: that
> > whole function can become a void function itself.  
> 


-- 
Thanks,
Mauro

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14 11:27   ` Mauro Carvalho Chehab
@ 2016-03-14 11:43     ` Sakari Ailus
  2016-03-14 11:52       ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2016-03-14 11:43 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Hans Verkuil, Linux Media Mailing List, Laurent Pinchart

Hi Mauro,

On Mon, Mar 14, 2016 at 08:27:38AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 14 Mar 2016 12:36:44 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Hans,
> > 
> > On Mon, Mar 14, 2016 at 09:25:51AM +0100, Hans Verkuil wrote:
> > > I was fixing a sparse warning in media_entity_pads_init() and I noticed
> > > that that function always returns 0. Any reason why this can't be changed
> > > to a void function?  
> > 
> > I was thinking of the same function but I had a different question: why
> > would one call this *after* entity->graph_obj.mdev is set? It is set by
> > media_device_register_entity(), but once mdev it's set, you're not expected
> > to call pads_init anymore...
> 
> Ideally, drivers should *first* create mdev, and then create the
> graph objects, as all objects should be registered at the mdev, in
> order to get their object ID and to be registered at the mdev's object
> lists.

Right. I think it wouldn't hurt to have some comment hints in what's there
for legacy use cases... I can submit patches for some of these.

Currently what works is that you can register graph objects until the media
device node is exposed to the user. We don't have proper serialisation in
place to do that, do we? At least the framework functions leave it up to the
caller.

I think it wouldn't be a bad idea either to think about the serialisation
model a bit. It's been unchanged from the day one basically.

> However, some legacy drivers used to do just the reverse. So, we had
> to add a code there, and at media_device_register_entity() to support
> the drivers that postpone the creation of the mdev instance.
> 
> Once this gets fixed everywhere, we can remove the code that supports
> the legacy behavior.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14 11:43     ` Sakari Ailus
@ 2016-03-14 11:52       ` Mauro Carvalho Chehab
  2016-03-14 13:05         ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2016-03-14 11:52 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Hans Verkuil, Linux Media Mailing List, Laurent Pinchart

Em Mon, 14 Mar 2016 13:43:33 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Mon, Mar 14, 2016 at 08:27:38AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 12:36:44 +0200
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> >   
> > > Hi Hans,
> > > 
> > > On Mon, Mar 14, 2016 at 09:25:51AM +0100, Hans Verkuil wrote:  
> > > > I was fixing a sparse warning in media_entity_pads_init() and I noticed
> > > > that that function always returns 0. Any reason why this can't be changed
> > > > to a void function?    
> > > 
> > > I was thinking of the same function but I had a different question: why
> > > would one call this *after* entity->graph_obj.mdev is set? It is set by
> > > media_device_register_entity(), but once mdev it's set, you're not expected
> > > to call pads_init anymore...  
> > 
> > Ideally, drivers should *first* create mdev, and then create the
> > graph objects, as all objects should be registered at the mdev, in
> > order to get their object ID and to be registered at the mdev's object
> > lists.  
> 
> Right. I think it wouldn't hurt to have some comment hints in what's there
> for legacy use cases... I can submit patches for some of these.

Yeah, feel free to submit a patch for it. It could be good to add a
warn_once() that would hit for the legacy case too.

> 
> Currently what works is that you can register graph objects until the media
> device node is exposed to the user.

Yes.

> We don't have proper serialisation in
> place to do that, do we? At least the framework functions leave it up to the
> caller.
> 
> I think it wouldn't be a bad idea either to think about the serialisation
> model a bit. It's been unchanged from the day one basically.

Actually, the async logic does some sort of serialization, although it
doesn't enforce it.

Javier touched on some cases where the logic was not right, but he
didn't change everything. 

I agree with you here: it would be great to have this fixed in a better
way.

That's said, this affects only non-PCI/USB devices. On PCI/USB, the
main/bridge driver is always called first, and the subdev init only
happens after it registers the I2C bus.

-- 
Thanks,
Mauro

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14 11:52       ` Mauro Carvalho Chehab
@ 2016-03-14 13:05         ` Mauro Carvalho Chehab
  2016-03-14 18:16           ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2016-03-14 13:05 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Hans Verkuil, Linux Media Mailing List, Laurent Pinchart

Em Mon, 14 Mar 2016 08:52:51 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Mon, 14 Mar 2016 13:43:33 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Mauro,
> > 
> > On Mon, Mar 14, 2016 at 08:27:38AM -0300, Mauro Carvalho Chehab wrote:  
> > > Em Mon, 14 Mar 2016 12:36:44 +0200
> > > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > >     
> > > > Hi Hans,
> > > > 
> > > > On Mon, Mar 14, 2016 at 09:25:51AM +0100, Hans Verkuil wrote:    
> > > > > I was fixing a sparse warning in media_entity_pads_init() and I noticed
> > > > > that that function always returns 0. Any reason why this can't be changed
> > > > > to a void function?      
> > > > 
> > > > I was thinking of the same function but I had a different question: why
> > > > would one call this *after* entity->graph_obj.mdev is set? It is set by
> > > > media_device_register_entity(), but once mdev it's set, you're not expected
> > > > to call pads_init anymore...    
> > > 
> > > Ideally, drivers should *first* create mdev, and then create the
> > > graph objects, as all objects should be registered at the mdev, in
> > > order to get their object ID and to be registered at the mdev's object
> > > lists.    
> > 
> > Right. I think it wouldn't hurt to have some comment hints in what's there
> > for legacy use cases... I can submit patches for some of these.  
> 
> Yeah, feel free to submit a patch for it. It could be good to add a
> warn_once() that would hit for the legacy case too.
> 
> > 
> > Currently what works is that you can register graph objects until the media
> > device node is exposed to the user.  
> 
> Yes.
> 
> > We don't have proper serialisation in
> > place to do that, do we? At least the framework functions leave it up to the
> > caller.
> > 
> > I think it wouldn't be a bad idea either to think about the serialisation
> > model a bit. It's been unchanged from the day one basically.  
> 
> Actually, the async logic does some sort of serialization, although it
> doesn't enforce it.
> 
> Javier touched on some cases where the logic was not right, but he
> didn't change everything. 
> 
> I agree with you here: it would be great to have this fixed in a better
> way.
> 
> That's said, this affects only non-PCI/USB devices. On PCI/USB, the
> main/bridge driver is always called first, and the subdev init only
> happens after it registers the I2C bus.
> 

Maybe we could do something like the patch below, to replace the I2C
probe routines, and then create some logic at the async ops that would
create the mdev and then call the SD-specific core.mc_probe() methods.

(patch doesn't compile yet, and it is not complete, but just to give
an idea on how we could do it).


diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index ff18444e19e4..36bf6c57eb64 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1214,6 +1214,9 @@ static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
 	.s_ctrl = tvp5150_s_ctrl,
 };
 
+static int tvp5150_mc_probe(struct v4l2_subdev *sd);
+
+
 static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
 	.log_status = tvp5150_log_status,
 	.reset = tvp5150_reset,
@@ -1221,6 +1224,9 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = {
 	.g_register = tvp5150_g_register,
 	.s_register = tvp5150_s_register,
 #endif
+#ifdef CONFIG_MEDIA_CONTROLLER
+	.mc_probe = tvp5150_mc_probe,
+#endif
 	.registered_async = tvp5150_registered_async,
 };
 
@@ -1438,11 +1444,33 @@ static const char * const tvp5150_test_patterns[2] = {
 	"Black screen"
 };
 
-static int tvp5150_probe(struct i2c_client *c,
-			 const struct i2c_device_id *id)
+static int __maybe_unused tvp5150_mc_probe(struct v4l2_subdev *sd)
+{
+	struct tvp5150 *core = to_tvp5150(sd);
+	int res;
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
+
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
+
+	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
+	if (res < 0)
+		return res;
+
+	sd->entity.ops = &tvp5150_sd_media_ops;
+#endif
+	return 0;
+}
+
+static int __tvp5150_probe(struct i2c_client *c,
+			   const struct i2c_device_id *id,
+			   struct v4l2_subdev **sd_store)
 {
 	struct tvp5150 *core;
-	struct v4l2_subdev *sd;
+	struct v4l2_subdev *sd; /* FIXME: get rid of it */
 	struct device_node *np = c->dev.of_node;
 	int res;
 
@@ -1460,6 +1488,7 @@ static int tvp5150_probe(struct i2c_client *c,
 		return -ENOMEM;
 
 	sd = &core->sd;
+	*sd_store = sd;
 
 	if (IS_ENABLED(CONFIG_OF) && np) {
 		res = tvp5150_parse_dt(core, np);
@@ -1475,20 +1504,6 @@ static int tvp5150_probe(struct i2c_client *c,
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
-	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
-
-	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
-
-	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
-	if (res < 0)
-		return res;
-
-	sd->entity.ops = &tvp5150_sd_media_ops;
-#endif
-
 	res = tvp5150_detect_version(core);
 	if (res < 0)
 		return res;
@@ -1542,6 +1557,8 @@ err:
 	return res;
 }
 
+DECLARE_SUBDEV_PROBE(tvp5150_probe, __tvp5150_probe);
+
 static int tvp5150_remove(struct i2c_client *c)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(c);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 11e2dfec0198..f15d63ccef30 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -183,6 +183,8 @@ struct v4l2_subdev_io_pin_config {
  * @registered_async: the subdevice has been registered async.
  */
 struct v4l2_subdev_core_ops {
+	int (*mc_probe)(struct v4l2_subdev *sd);
+
 	int (*log_status)(struct v4l2_subdev *sd);
 	int (*s_io_pin_config)(struct v4l2_subdev *sd, size_t n,
 				      struct v4l2_subdev_io_pin_config *pincfg);
@@ -795,6 +797,23 @@ static inline void *v4l2_get_subdev_hostdata(const struct v4l2_subdev *sd)
 	return sd->host_priv;
 }
 
+
+/* FIXME: need an #if defined(MEDIA_CONTROLLER) */
+#define DECLARE_SUBDEV_PROBE(name, probe_fn)				\
+static int __subdev_probe(struct i2c_client *c,				\
+			  const struct i2c_device_id *id)		\
+{									\
+	int __rc;							\
+	struct v4l2_subdev *__sd;					\
+									\
+	__rc = probe_fn(c, id, &__sd);					\
+									\
+	if (!__rc && __sd->v4l2_dev->mdev && __sd->ops->core.mc_probe)	\
+		__rc = __sd->ops->core.mc_probe(__sd);			\
+									\
+	return __rc;							\
+}
+
 #ifdef CONFIG_MEDIA_CONTROLLER
 int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
 				      struct media_link *link,


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: Any reason why media_entity_pads_init() isn't void?
  2016-03-14 13:05         ` Mauro Carvalho Chehab
@ 2016-03-14 18:16           ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2016-03-14 18:16 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: Hans Verkuil, Linux Media Mailing List, Laurent Pinchart

Em Mon, 14 Mar 2016 10:05:01 -0300
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Mon, 14 Mar 2016 08:52:51 -0300
> Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:
> 
> > Em Mon, 14 Mar 2016 13:43:33 +0200
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > On Mon, Mar 14, 2016 at 08:27:38AM -0300, Mauro Carvalho Chehab wrote:    
> > > > Em Mon, 14 Mar 2016 12:36:44 +0200
> > > > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > > >       
> > > > > Hi Hans,
> > > > > 
> > > > > On Mon, Mar 14, 2016 at 09:25:51AM +0100, Hans Verkuil wrote:      
> > > > > > I was fixing a sparse warning in media_entity_pads_init() and I noticed
> > > > > > that that function always returns 0. Any reason why this can't be changed
> > > > > > to a void function?        
> > > > > 
> > > > > I was thinking of the same function but I had a different question: why
> > > > > would one call this *after* entity->graph_obj.mdev is set? It is set by
> > > > > media_device_register_entity(), but once mdev it's set, you're not expected
> > > > > to call pads_init anymore...      
> > > > 
> > > > Ideally, drivers should *first* create mdev, and then create the
> > > > graph objects, as all objects should be registered at the mdev, in
> > > > order to get their object ID and to be registered at the mdev's object
> > > > lists.      
> > > 
> > > Right. I think it wouldn't hurt to have some comment hints in what's there
> > > for legacy use cases... I can submit patches for some of these.    
> > 
> > Yeah, feel free to submit a patch for it. It could be good to add a
> > warn_once() that would hit for the legacy case too.
> >   
> > > 
> > > Currently what works is that you can register graph objects until the media
> > > device node is exposed to the user.    
> > 
> > Yes.
> >   
> > > We don't have proper serialisation in
> > > place to do that, do we? At least the framework functions leave it up to the
> > > caller.
> > > 
> > > I think it wouldn't be a bad idea either to think about the serialisation
> > > model a bit. It's been unchanged from the day one basically.    
> > 
> > Actually, the async logic does some sort of serialization, although it
> > doesn't enforce it.
> > 
> > Javier touched on some cases where the logic was not right, but he
> > didn't change everything. 
> > 
> > I agree with you here: it would be great to have this fixed in a better
> > way.
> > 
> > That's said, this affects only non-PCI/USB devices. On PCI/USB, the
> > main/bridge driver is always called first, and the subdev init only
> > happens after it registers the I2C bus.
> >   
> 
> Maybe we could do something like the patch below, to replace the I2C
> probe routines, and then create some logic at the async ops that would
> create the mdev and then call the SD-specific core.mc_probe() methods.
> 
> (patch doesn't compile yet, and it is not complete, but just to give
> an idea on how we could do it).

Still not tested, but, IMHO, in a better shape. See enclosed.

The idea behind it is to split the PAD init on all subdevs and let the
core call it.

For now, the core is calling it early, when v4l2_i2c_subdev_init() is
called, but, after making all subdevs use the new pad_init() callback,
we can call them later. I guess the best place for the code would be
to be called only at v4l2_device_register_subdev(), but we need to
double check if this would work on all cases.

-


diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index ff18444e19e4..14004fd7d0fb 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1208,6 +1208,28 @@ static int tvp5150_registered_async(struct v4l2_subdev *sd)
 	return 0;
 }
 
+static int __maybe_unused tvp5150_pad_init(struct v4l2_subdev *sd)
+{
+	struct tvp5150 *core = to_tvp5150(sd);
+	int res;
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
+	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
+
+	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
+
+	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
+	if (res < 0)
+		return res;
+
+	sd->entity.ops = &tvp5150_sd_media_ops;
+#endif
+	return 0;
+}
+
+
 /* ----------------------------------------------------------------------- */
 
 static const struct v4l2_ctrl_ops tvp5150_ctrl_ops = {
@@ -1246,6 +1268,9 @@ static const struct v4l2_subdev_vbi_ops tvp5150_vbi_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops tvp5150_pad_ops = {
+#ifdef CONFIG_MEDIA_CONTROLLER
+	.pad_init = tvp5150_pad_init,
+#endif
 	.enum_mbus_code = tvp5150_enum_mbus_code,
 	.enum_frame_size = tvp5150_enum_frame_size,
 	.set_fmt = tvp5150_fill_fmt,
@@ -1475,20 +1500,6 @@ static int tvp5150_probe(struct i2c_client *c,
 	v4l2_i2c_subdev_init(sd, c, &tvp5150_ops);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-#if defined(CONFIG_MEDIA_CONTROLLER)
-	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
-	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
-	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
-
-	sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
-
-	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
-	if (res < 0)
-		return res;
-
-	sd->entity.ops = &tvp5150_sd_media_ops;
-#endif
-
 	res = tvp5150_detect_version(core);
 	if (res < 0)
 		return res;
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 5b808500e7e7..6bcdf557e027 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -112,6 +112,14 @@ EXPORT_SYMBOL(v4l2_ctrl_query_fill);
 void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
 		const struct v4l2_subdev_ops *ops)
 {
+	/*
+	 * Initialize the MC pads - for now, this will be called
+	 * unconditionally, since the current implementation will defer
+	 * the pads initialization until the media_dev is created.
+	 */
+	if (v4l2_subdev_has_op(sd, pad, pad_init))
+		sd->ops->pad->pad_init(sd);
+
 	v4l2_subdev_init(sd, ops);
 	sd->flags |= V4L2_SUBDEV_FL_IS_I2C;
 	/* the owner is the same as the i2c_client's driver owner */
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 11e2dfec0198..c9bb221029e4 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -572,6 +572,9 @@ struct v4l2_subdev_pad_config {
 /**
  * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations
  *
+ * @pad_init: callback that initializes the media-controller specific part
+ *	      of the subdev driver, creating its pads
+ *
  * @enum_mbus_code: callback for VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl handler
  *		    code.
  * @enum_frame_size: callback for VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl handler
@@ -607,6 +610,7 @@ struct v4l2_subdev_pad_config {
  *                  may be adjusted by the subdev driver to device capabilities.
  */
 struct v4l2_subdev_pad_ops {
+	int (*pad_init)(struct v4l2_subdev *sd);
 	int (*enum_mbus_code)(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_pad_config *cfg,
 			      struct v4l2_subdev_mbus_code_enum *code);

-- 
Thanks,
Mauro

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-03-14 18:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-14  8:25 Any reason why media_entity_pads_init() isn't void? Hans Verkuil
2016-03-14 10:24 ` Mauro Carvalho Chehab
2016-03-14 10:36 ` Sakari Ailus
2016-03-14 11:27   ` Mauro Carvalho Chehab
2016-03-14 11:43     ` Sakari Ailus
2016-03-14 11:52       ` Mauro Carvalho Chehab
2016-03-14 13:05         ` Mauro Carvalho Chehab
2016-03-14 18:16           ` Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).