[1/7] tuner-core: properly initialize media controller subdev

Commit Message

Mauro Carvalho Chehab Jan. 3, 2015, 8:09 p.m. UTC

Properly initialize tuner core subdev at the media controller.

That requires a new subtype at the media controller API.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

Comments

Sakari Ailus Jan. 4, 2015, 12:39 a.m. UTC | #1

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Properly initialize tuner core subdev at the media controller.
>
> That requires a new subtype at the media controller API.
>
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
>
> diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
> index 559f8372e2eb..114715ed0110 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -134,6 +134,9 @@ struct tuner {
>   	unsigned int        type; /* chip type id */
>   	void                *config;
>   	const char          *name;
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	struct media_pad	pad;
> +#endif
>   };
>
>   /*
> @@ -434,6 +437,8 @@ static void set_type(struct i2c_client *c, unsigned int type,
>   		t->name = analog_ops->info.name;
>   	}
>
> +	t->sd.entity.name = t->name;
> +
>   	tuner_dbg("type set to %s\n", t->name);
>
>   	t->mode_mask = new_mode_mask;
> @@ -592,6 +597,7 @@ static int tuner_probe(struct i2c_client *client,
>   	struct tuner *t;
>   	struct tuner *radio;
>   	struct tuner *tv;
> +	int ret;

This will emit a compiler warning if CONFIG_MEDIA_CONTROLLER isn't defined.

>   	t = kzalloc(sizeof(struct tuner), GFP_KERNEL);
>   	if (NULL == t)
> @@ -696,6 +702,15 @@ register_client:
>   		   t->type,
>   		   t->mode_mask & T_RADIO ? " Radio" : "",
>   		   t->mode_mask & T_ANALOG_TV ? " TV" : "");
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	t->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
> +	t->sd.entity.name = t->name;
> +
> +	ret = media_entity_init(&t->sd.entity, 1, &t->pad, 0);
> +	if (ret < 0)
> +		tuner_err("failed to initialize media entity!\n");

I might return the error back to the caller. The failing initialisation 
of a media entity itself might not be a fatal problem, but someone later 
assuming it has been initialised might be.

> +#endif
>   	return 0;
>   }
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 707db275f92b..5ffde035789b 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -66,6 +66,8 @@ struct media_device_info {
>   /* A converter of analogue video to its digital representation. */
>   #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV + 4)
>
> +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV + 5)
> +
>   #define MEDIA_ENT_FL_DEFAULT		(1 << 0)
>
>   struct media_entity_desc {
>

Laurent Pinchart Jan. 11, 2015, 2:02 p.m. UTC | #2

Hi Mauro,

Thank you for the patch.

On Saturday 03 January 2015 18:09:33 Mauro Carvalho Chehab wrote:
> Properly initialize tuner core subdev at the media controller.
> 
> That requires a new subtype at the media controller API.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> 
> diff --git a/drivers/media/v4l2-core/tuner-core.c
> b/drivers/media/v4l2-core/tuner-core.c index 559f8372e2eb..114715ed0110
> 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -134,6 +134,9 @@ struct tuner {
>  	unsigned int        type; /* chip type id */
>  	void                *config;
>  	const char          *name;
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	struct media_pad	pad;
> +#endif

I'm not too familiar with tuners, do they all have a single output only and no 
input ?

>  };
> 
>  /*
> @@ -434,6 +437,8 @@ static void set_type(struct i2c_client *c, unsigned int
> type, t->name = analog_ops->info.name;
>  	}
> 
> +	t->sd.entity.name = t->name;
> +

Entity information is not supposed to change at runtime, I'm not sure to be 
comfortable with this change.

set_type() is called at probe time and in tuner_s_type_addr(). The former just 
duplicates the name initialization in tuner_probe(), so isn't really needed. 
The later bothers me.

>  	tuner_dbg("type set to %s\n", t->name);
> 
>  	t->mode_mask = new_mode_mask;
> @@ -592,6 +597,7 @@ static int tuner_probe(struct i2c_client *client,
>  	struct tuner *t;
>  	struct tuner *radio;
>  	struct tuner *tv;
> +	int ret;
> 
>  	t = kzalloc(sizeof(struct tuner), GFP_KERNEL);
>  	if (NULL == t)
> @@ -696,6 +702,15 @@ register_client:
>  		   t->type,
>  		   t->mode_mask & T_RADIO ? " Radio" : "",
>  		   t->mode_mask & T_ANALOG_TV ? " TV" : "");
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	t->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
> +	t->sd.entity.name = t->name;

v4l2_subdev_init(), called by v4l2_i2c_subdev_init(), sets sd->entity.name to 
point to sd->name. Is there a reason why the subdev name can't be used as the 
entity name ?

> +
> +	ret = media_entity_init(&t->sd.entity, 1, &t->pad, 0);
> +	if (ret < 0)
> +		tuner_err("failed to initialize media entity!\n");
> +#endif
>  	return 0;
>  }
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 707db275f92b..5ffde035789b 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -66,6 +66,8 @@ struct media_device_info {
>  /* A converter of analogue video to its digital representation. */
>  #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV + 4)
> 
> +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV + 5)
> +
>  #define MEDIA_ENT_FL_DEFAULT		(1 << 0)
> 
>  struct media_entity_desc {

Mauro Carvalho Chehab Jan. 11, 2015, 2:25 p.m. UTC | #3

Em Sun, 11 Jan 2015 16:02:41 +0200
Laurent Pinchart <[e-mail]> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Saturday 03 January 2015 18:09:33 Mauro Carvalho Chehab wrote:
> > Properly initialize tuner core subdev at the media controller.
> > 
> > That requires a new subtype at the media controller API.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > 
> > diff --git a/drivers/media/v4l2-core/tuner-core.c
> > b/drivers/media/v4l2-core/tuner-core.c index 559f8372e2eb..114715ed0110
> > 100644
> > --- a/drivers/media/v4l2-core/tuner-core.c
> > +++ b/drivers/media/v4l2-core/tuner-core.c
> > @@ -134,6 +134,9 @@ struct tuner {
> >  	unsigned int        type; /* chip type id */
> >  	void                *config;
> >  	const char          *name;
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	struct media_pad	pad;
> > +#endif
> 
> I'm not too familiar with tuners, do they all have a single output only and no 
> input ?

They have an input: the antenna connector. However, I don't see any need
to map it for most tuners, as there's generally just one input, hardwired
into the tuner chip.

There are some hardware with 2 antenna connectors, but for different
functions (FM and TV). They're selected automatically when the V4L2
driver switches between FM and TV.

In any case, the tuner-core doesn't provide any way to select the
antenna input.

So, if a driver would need to select the input, it would either need
to not use tuner-core or some patch will be required to add such
functionality inside tuner-core.

> >  };
> > 
> >  /*
> > @@ -434,6 +437,8 @@ static void set_type(struct i2c_client *c, unsigned int
> > type, t->name = analog_ops->info.name;
> >  	}
> > 
> > +	t->sd.entity.name = t->name;
> > +
> 
> Entity information is not supposed to change at runtime, I'm not sure to be 
> comfortable with this change.
> 
> set_type() is called at probe time and in tuner_s_type_addr(). The former just 
> duplicates the name initialization in tuner_probe(), so isn't really needed. 
> The later bothers me.

The tuner-core driver is a "core subdev" implementation. It handles
the ioctl logic, but the actual driver is a different one. It also
have internally a probe logic that will load the correct tuner subdev.

The tuner_s_type_addr() callback, used only at bridge probing time,
is a way for the bridge driver to provide the name of the tuner driver
that should be loaded, plus its I2C address.

So, once the board is probed, the name shouldn't change.

> 
> >  	tuner_dbg("type set to %s\n", t->name);
> > 
> >  	t->mode_mask = new_mode_mask;
> > @@ -592,6 +597,7 @@ static int tuner_probe(struct i2c_client *client,
> >  	struct tuner *t;
> >  	struct tuner *radio;
> >  	struct tuner *tv;
> > +	int ret;
> > 
> >  	t = kzalloc(sizeof(struct tuner), GFP_KERNEL);
> >  	if (NULL == t)
> > @@ -696,6 +702,15 @@ register_client:
> >  		   t->type,
> >  		   t->mode_mask & T_RADIO ? " Radio" : "",
> >  		   t->mode_mask & T_ANALOG_TV ? " TV" : "");
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	t->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
> > +	t->sd.entity.name = t->name;
> 
> v4l2_subdev_init(), called by v4l2_i2c_subdev_init(), sets sd->entity.name to 
> point to sd->name. Is there a reason why the subdev name can't be used as the 
> entity name ?

If we don't set entity.name to t->name, the sd->name will be "tuner-core",
instead of the name of the real subdev.

> > +
> > +	ret = media_entity_init(&t->sd.entity, 1, &t->pad, 0);
> > +	if (ret < 0)
> > +		tuner_err("failed to initialize media entity!\n");
> > +#endif
> >  	return 0;
> >  }
> > 
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index 707db275f92b..5ffde035789b 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -66,6 +66,8 @@ struct media_device_info {
> >  /* A converter of analogue video to its digital representation. */
> >  #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV + 4)
> > 
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV + 5)
> > +
> >  #define MEDIA_ENT_FL_DEFAULT		(1 << 0)
> > 
> >  struct media_entity_desc {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Antti Palosaari Jan. 13, 2015, 4:35 p.m. UTC | #4

On 01/11/2015 04:25 PM, Mauro Carvalho Chehab wrote:
> Em Sun, 11 Jan 2015 16:02:41 +0200
> Laurent Pinchart <[e-mail]> escreveu:

>> I'm not too familiar with tuners, do they all have a single output only and no
>> input ?
>
> They have an input: the antenna connector. However, I don't see any need
> to map it for most tuners, as there's generally just one input, hardwired
> into the tuner chip.
>
> There are some hardware with 2 antenna connectors, but for different
> functions (FM and TV). They're selected automatically when the V4L2
> driver switches between FM and TV.
>
> In any case, the tuner-core doesn't provide any way to select the
> antenna input.
>
> So, if a driver would need to select the input, it would either need
> to not use tuner-core or some patch will be required to add such
> functionality inside tuner-core.

Tuner has antenna as a input and output is intermediate frequency or 
baseband (IF/BB (zero-IF)).

I think most modern silicon tuners actually has more than one physical 
antenna inputs - but those are left unused or same physical antenna 
connector is wired to all those inputs.

Sooner or later there will be receiver having multiple antenna 
connectors which are selectable by software. So let it be at least 
option easy to add later.

regards
Antti

Patch

diff --git a/drivers/media/v4l2-core/tuner-core.c b/drivers/media/v4l2-core/tuner-core.c
index 559f8372e2eb..114715ed0110 100644
--- a/drivers/media/v4l2-core/tuner-core.c
+++ b/drivers/media/v4l2-core/tuner-core.c
@@ -134,6 +134,9 @@  struct tuner {
 	unsigned int        type; /* chip type id */
 	void                *config;
 	const char          *name;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct media_pad	pad;
+#endif
 };
 
 /*
@@ -434,6 +437,8 @@  static void set_type(struct i2c_client *c, unsigned int type,
 		t->name = analog_ops->info.name;
 	}
 
+	t->sd.entity.name = t->name;
+
 	tuner_dbg("type set to %s\n", t->name);
 
 	t->mode_mask = new_mode_mask;
@@ -592,6 +597,7 @@  static int tuner_probe(struct i2c_client *client,
 	struct tuner *t;
 	struct tuner *radio;
 	struct tuner *tv;
+	int ret;
 
 	t = kzalloc(sizeof(struct tuner), GFP_KERNEL);
 	if (NULL == t)
@@ -696,6 +702,15 @@  register_client:
 		   t->type,
 		   t->mode_mask & T_RADIO ? " Radio" : "",
 		   t->mode_mask & T_ANALOG_TV ? " TV" : "");
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	t->pad.flags = MEDIA_PAD_FL_SOURCE;
+	t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
+	t->sd.entity.name = t->name;
+
+	ret = media_entity_init(&t->sd.entity, 1, &t->pad, 0);
+	if (ret < 0)
+		tuner_err("failed to initialize media entity!\n");
+#endif
 	return 0;
 }
 
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 707db275f92b..5ffde035789b 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -66,6 +66,8 @@  struct media_device_info {
 /* A converter of analogue video to its digital representation. */
 #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER	(MEDIA_ENT_T_V4L2_SUBDEV + 4)
 
+#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER	(MEDIA_ENT_T_V4L2_SUBDEV + 5)
+
 #define MEDIA_ENT_FL_DEFAULT		(1 << 0)
 
 struct media_entity_desc {