[PATCH] media: i2c: tvp514x: add support for asynchronous probing

[PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Prabhakar Lad]

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>

Both synchronous and asynchronous tvp514x subdevice probing is supported by
this patch.

Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 864eb14..d090caf 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -36,6 +36,7 @@
 #include <linux/module.h>
 #include <linux/v4l2-mediabus.h>
 
+#include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-mediabus.h>
@@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	/* Register with V4L2 layer as slave device */
 	sd = &decoder->sd;
 	v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
-	strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
+	strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
 	decoder->pad.flags = MEDIA_PAD_FL_SOURCE;
 	decoder->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	decoder->sd.entity.flags |= MEDIA_ENT_T_V4L2_SUBDEV_DECODER;
@@ -1176,16 +1177,22 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	sd->ctrl_handler = &decoder->hdl;
 	if (decoder->hdl.error) {
 		ret = decoder->hdl.error;
-
-		v4l2_ctrl_handler_free(&decoder->hdl);
-		return ret;
+		goto done;
 	}
 	v4l2_ctrl_handler_setup(&decoder->hdl);
 
-	v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
-
-	return 0;
+	ret = v4l2_async_register_subdev(&decoder->sd);
+	if (!ret)
+		v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
 
+done:
+	if (ret < 0) {
+		v4l2_ctrl_handler_free(&decoder->hdl);
+#if defined(CONFIG_MEDIA_CONTROLLER)
+		media_entity_cleanup(&decoder->sd.entity);
+#endif
+	}
+	return ret;
 }
 
 /**
@@ -1200,6 +1207,7 @@ static int tvp514x_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct tvp514x_decoder *decoder = to_decoder(sd);
 
+	v4l2_async_unregister_subdev(&decoder->sd);
 	v4l2_device_unregister_subdev(sd);
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	media_entity_cleanup(&decoder->sd.entity);
-- 
1.7.9.5

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Guennadi Liakhovetski]

On Sat, 22 Jun 2013, Prabhakar Lad wrote:

> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> 
> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> this patch.
> 
> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index 864eb14..d090caf 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -36,6 +36,7 @@
>  #include <linux/module.h>
>  #include <linux/v4l2-mediabus.h>
>  
> +#include <media/v4l2-async.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-mediabus.h>

Ok, but this one really does too many things in one patch:

> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	/* Register with V4L2 layer as slave device */
>  	sd = &decoder->sd;
>  	v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
> -	strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>  
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> +	strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));

This is unrelated

>  	decoder->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	decoder->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	decoder->sd.entity.flags |= MEDIA_ENT_T_V4L2_SUBDEV_DECODER;
> @@ -1176,16 +1177,22 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	sd->ctrl_handler = &decoder->hdl;
>  	if (decoder->hdl.error) {
>  		ret = decoder->hdl.error;
> -
> -		v4l2_ctrl_handler_free(&decoder->hdl);
> -		return ret;
> +		goto done;
>  	}
>  	v4l2_ctrl_handler_setup(&decoder->hdl);
>  
> -	v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
> -
> -	return 0;
> +	ret = v4l2_async_register_subdev(&decoder->sd);
> +	if (!ret)
> +		v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
>  
> +done:
> +	if (ret < 0) {
> +		v4l2_ctrl_handler_free(&decoder->hdl);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +		media_entity_cleanup(&decoder->sd.entity);

So is this - it wasn't called before in the "if (decoder->hdl.error)" case 
above.

Thanks
Guennadi

> +#endif
> +	}
> +	return ret;
>  }
>  
>  /**
> @@ -1200,6 +1207,7 @@ static int tvp514x_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  	struct tvp514x_decoder *decoder = to_decoder(sd);
>  
> +	v4l2_async_unregister_subdev(&decoder->sd);
>  	v4l2_device_unregister_subdev(sd);
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	media_entity_cleanup(&decoder->sd.entity);
> -- 
> 1.7.9.5
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Prabhakar Lad]

Hi Guennadi,

Thanks for the review.

On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Sat, 22 Jun 2013, Prabhakar Lad wrote:
>
>> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>>
>> Both synchronous and asynchronous tvp514x subdevice probing is supported by
>> this patch.
>>
>> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
>> index 864eb14..d090caf 100644
>> --- a/drivers/media/i2c/tvp514x.c
>> +++ b/drivers/media/i2c/tvp514x.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/module.h>
>>  #include <linux/v4l2-mediabus.h>
>>
>> +#include <media/v4l2-async.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-common.h>
>>  #include <media/v4l2-mediabus.h>
>
> Ok, but this one really does too many things in one patch:
>
>> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>       /* Register with V4L2 layer as slave device */
>>       sd = &decoder->sd;
>>       v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
>> -     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>>
>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>> +     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>
> This is unrelated
>
OK I'll split the patch or may be a line in a commit message can do ?

>>       decoder->pad.flags = MEDIA_PAD_FL_SOURCE;
>>       decoder->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>       decoder->sd.entity.flags |= MEDIA_ENT_T_V4L2_SUBDEV_DECODER;
>> @@ -1176,16 +1177,22 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>       sd->ctrl_handler = &decoder->hdl;
>>       if (decoder->hdl.error) {
>>               ret = decoder->hdl.error;
>> -
>> -             v4l2_ctrl_handler_free(&decoder->hdl);
>> -             return ret;
>> +             goto done;
>>       }
>>       v4l2_ctrl_handler_setup(&decoder->hdl);
>>
>> -     v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
>> -
>> -     return 0;
>> +     ret = v4l2_async_register_subdev(&decoder->sd);
>> +     if (!ret)
>> +             v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
>>
>> +done:
>> +     if (ret < 0) {
>> +             v4l2_ctrl_handler_free(&decoder->hdl);
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +             media_entity_cleanup(&decoder->sd.entity);
>
> So is this - it wasn't called before in the "if (decoder->hdl.error)" case
> above.
>
Yes so fixed it up here.

Regards,
--Prabhakar Lad

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Hans Verkuil]

On Sun June 23 2013 17:48:20 Prabhakar Lad wrote:
> Hi Guennadi,
> 
> Thanks for the review.
> 
> On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> >
> >> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> >>
> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> >> this patch.
> >>
> >> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
> >> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> >> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> >> ---
> >>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> >> index 864eb14..d090caf 100644
> >> --- a/drivers/media/i2c/tvp514x.c
> >> +++ b/drivers/media/i2c/tvp514x.c
> >> @@ -36,6 +36,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/v4l2-mediabus.h>
> >>
> >> +#include <media/v4l2-async.h>
> >>  #include <media/v4l2-device.h>
> >>  #include <media/v4l2-common.h>
> >>  #include <media/v4l2-mediabus.h>
> >
> > Ok, but this one really does too many things in one patch:
> >
> >> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >>       /* Register with V4L2 layer as slave device */
> >>       sd = &decoder->sd;
> >>       v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
> >> -     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> >>
> >>  #if defined(CONFIG_MEDIA_CONTROLLER)
> >> +     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> >
> > This is unrelated
> >
> OK I'll split the patch or may be a line in a commit message can do ?

Please split it up in two patches.

Why is sd->name set anyway? And why is it moved under CONFIG_MEDIA_CONTROLLER?
It's not obvious to me.

The remainder of the patch makes sense.

Regards,

	Hans

> 
> >>       decoder->pad.flags = MEDIA_PAD_FL_SOURCE;
> >>       decoder->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >>       decoder->sd.entity.flags |= MEDIA_ENT_T_V4L2_SUBDEV_DECODER;
> >> @@ -1176,16 +1177,22 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >>       sd->ctrl_handler = &decoder->hdl;
> >>       if (decoder->hdl.error) {
> >>               ret = decoder->hdl.error;
> >> -
> >> -             v4l2_ctrl_handler_free(&decoder->hdl);
> >> -             return ret;
> >> +             goto done;
> >>       }
> >>       v4l2_ctrl_handler_setup(&decoder->hdl);
> >>
> >> -     v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
> >> -
> >> -     return 0;
> >> +     ret = v4l2_async_register_subdev(&decoder->sd);
> >> +     if (!ret)
> >> +             v4l2_info(sd, "%s decoder driver registered !!\n", sd->name);
> >>
> >> +done:
> >> +     if (ret < 0) {
> >> +             v4l2_ctrl_handler_free(&decoder->hdl);
> >> +#if defined(CONFIG_MEDIA_CONTROLLER)
> >> +             media_entity_cleanup(&decoder->sd.entity);
> >
> > So is this - it wasn't called before in the "if (decoder->hdl.error)" case
> > above.
> >
> Yes so fixed it up here.
> 
> Regards,
> --Prabhakar Lad
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Prabhakar Lad]

Hi Hans,

On Mon, Jun 24, 2013 at 12:41 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Sun June 23 2013 17:48:20 Prabhakar Lad wrote:
>> Hi Guennadi,
>>
>> Thanks for the review.
>>
>> On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
>> <g.liakhovetski@gmx.de> wrote:
>> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
>> >
>> >> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>> >>
>> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
>> >> this patch.
>> >>
>> >> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
>> >> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>> >> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> >> ---
>> >>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
>> >>  1 file changed, 15 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
>> >> index 864eb14..d090caf 100644
>> >> --- a/drivers/media/i2c/tvp514x.c
>> >> +++ b/drivers/media/i2c/tvp514x.c
>> >> @@ -36,6 +36,7 @@
>> >>  #include <linux/module.h>
>> >>  #include <linux/v4l2-mediabus.h>
>> >>
>> >> +#include <media/v4l2-async.h>
>> >>  #include <media/v4l2-device.h>
>> >>  #include <media/v4l2-common.h>
>> >>  #include <media/v4l2-mediabus.h>
>> >
>> > Ok, but this one really does too many things in one patch:
>> >
>> >> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> >>       /* Register with V4L2 layer as slave device */
>> >>       sd = &decoder->sd;
>> >>       v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
>> >> -     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>> >>
>> >>  #if defined(CONFIG_MEDIA_CONTROLLER)
>> >> +     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>> >
>> > This is unrelated
>> >
>> OK I'll split the patch or may be a line in a commit message can do ?
>
> Please split it up in two patches.
>
> Why is sd->name set anyway? And why is it moved under CONFIG_MEDIA_CONTROLLER?
> It's not obvious to me.
>
while using tvp514x subdev with media controller based drivers, when we
enumerate entities (MEDIA_IOC_ENUM_ENTITIES) to get the index id
of the entity we compare the entity name with "tvp514x", So I moved it
under CONFIG_MEDIA_CONTROLLER config. I hope you are OK with
moving this in a separate patch.

Regards,
--Prabhakar Lad

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Hans Verkuil]

On Mon June 24 2013 10:24:02 Prabhakar Lad wrote:
> Hi Hans,
> 
> On Mon, Jun 24, 2013 at 12:41 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Sun June 23 2013 17:48:20 Prabhakar Lad wrote:
> >> Hi Guennadi,
> >>
> >> Thanks for the review.
> >>
> >> On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
> >> <g.liakhovetski@gmx.de> wrote:
> >> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> >> >
> >> >> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> >> >>
> >> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> >> >> this patch.
> >> >>
> >> >> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
> >> >> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> >> >> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> >> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> >> >> ---
> >> >>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
> >> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >> >>
> >> >> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> >> >> index 864eb14..d090caf 100644
> >> >> --- a/drivers/media/i2c/tvp514x.c
> >> >> +++ b/drivers/media/i2c/tvp514x.c
> >> >> @@ -36,6 +36,7 @@
> >> >>  #include <linux/module.h>
> >> >>  #include <linux/v4l2-mediabus.h>
> >> >>
> >> >> +#include <media/v4l2-async.h>
> >> >>  #include <media/v4l2-device.h>
> >> >>  #include <media/v4l2-common.h>
> >> >>  #include <media/v4l2-mediabus.h>
> >> >
> >> > Ok, but this one really does too many things in one patch:
> >> >
> >> >> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >> >>       /* Register with V4L2 layer as slave device */
> >> >>       sd = &decoder->sd;
> >> >>       v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
> >> >> -     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> >> >>
> >> >>  #if defined(CONFIG_MEDIA_CONTROLLER)
> >> >> +     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> >> >
> >> > This is unrelated
> >> >
> >> OK I'll split the patch or may be a line in a commit message can do ?
> >
> > Please split it up in two patches.
> >
> > Why is sd->name set anyway? And why is it moved under CONFIG_MEDIA_CONTROLLER?
> > It's not obvious to me.
> >
> while using tvp514x subdev with media controller based drivers, when we
> enumerate entities (MEDIA_IOC_ENUM_ENTITIES) to get the index id
> of the entity we compare the entity name with "tvp514x", So I moved it
> under CONFIG_MEDIA_CONTROLLER config. I hope you are OK with
> moving this in a separate patch.

Sorry, but this approach is wrong. sd->name must be a unique name, so manually
setting sd->name will fail if you have two tvp514x devices.

There is no reason to override sd->name here, and it is actually a bug. I see
that tvp7002 has the same problem (and a bunch of others as well).

When trying to find a tvp514x you can just use strstr() in your application.
That will work all the time as long as there is only one tvp514x.

Regards,

	Hans

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Prabhakar Lad]

Hi Hans,

On Mon, Jun 24, 2013 at 2:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Mon June 24 2013 10:24:02 Prabhakar Lad wrote:
>> Hi Hans,
>>
>> On Mon, Jun 24, 2013 at 12:41 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Sun June 23 2013 17:48:20 Prabhakar Lad wrote:
>> >> Hi Guennadi,
>> >>
>> >> Thanks for the review.
>> >>
>> >> On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
>> >> <g.liakhovetski@gmx.de> wrote:
>> >> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
>> >> >
>> >> >> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>> >> >>
>> >> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
>> >> >> this patch.
>> >> >>
>> >> >> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
>> >> >> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> >> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> >> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
>> >> >> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> >> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
>> >> >> ---
>> >> >>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
>> >> >>  1 file changed, 15 insertions(+), 7 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
>> >> >> index 864eb14..d090caf 100644
>> >> >> --- a/drivers/media/i2c/tvp514x.c
>> >> >> +++ b/drivers/media/i2c/tvp514x.c
>> >> >> @@ -36,6 +36,7 @@
>> >> >>  #include <linux/module.h>
>> >> >>  #include <linux/v4l2-mediabus.h>
>> >> >>
>> >> >> +#include <media/v4l2-async.h>
>> >> >>  #include <media/v4l2-device.h>
>> >> >>  #include <media/v4l2-common.h>
>> >> >>  #include <media/v4l2-mediabus.h>
>> >> >
>> >> > Ok, but this one really does too many things in one patch:
>> >> >
>> >> >> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
>> >> >>       /* Register with V4L2 layer as slave device */
>> >> >>       sd = &decoder->sd;
>> >> >>       v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
>> >> >> -     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>> >> >>
>> >> >>  #if defined(CONFIG_MEDIA_CONTROLLER)
>> >> >> +     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
>> >> >
>> >> > This is unrelated
>> >> >
>> >> OK I'll split the patch or may be a line in a commit message can do ?
>> >
>> > Please split it up in two patches.
>> >
>> > Why is sd->name set anyway? And why is it moved under CONFIG_MEDIA_CONTROLLER?
>> > It's not obvious to me.
>> >
>> while using tvp514x subdev with media controller based drivers, when we
>> enumerate entities (MEDIA_IOC_ENUM_ENTITIES) to get the index id
>> of the entity we compare the entity name with "tvp514x", So I moved it
>> under CONFIG_MEDIA_CONTROLLER config. I hope you are OK with
>> moving this in a separate patch.
>
> Sorry, but this approach is wrong. sd->name must be a unique name, so manually
> setting sd->name will fail if you have two tvp514x devices.
>
> There is no reason to override sd->name here, and it is actually a bug. I see
> that tvp7002 has the same problem (and a bunch of others as well).
>
> When trying to find a tvp514x you can just use strstr() in your application.
> That will work all the time as long as there is only one tvp514x.

OK, then I will send out a cleanup patch removing sd->name from this driver
and others too.

Regards,
--Prabhakar Lad

Re: [PATCH] media: i2c: tvp514x: add support for asynchronous probing

[Hans Verkuil]

On Mon June 24 2013 10:53:37 Prabhakar Lad wrote:
> Hi Hans,
> 
> On Mon, Jun 24, 2013 at 2:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Mon June 24 2013 10:24:02 Prabhakar Lad wrote:
> >> Hi Hans,
> >>
> >> On Mon, Jun 24, 2013 at 12:41 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> > On Sun June 23 2013 17:48:20 Prabhakar Lad wrote:
> >> >> Hi Guennadi,
> >> >>
> >> >> Thanks for the review.
> >> >>
> >> >> On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
> >> >> <g.liakhovetski@gmx.de> wrote:
> >> >> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> >> >> >
> >> >> >> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> >> >> >>
> >> >> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> >> >> >> this patch.
> >> >> >>
> >> >> >> Signed-off-by: Prabhakar Lad <prabhakar.csengg@gmail.com>
> >> >> >> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> >> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> >> >> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> >> >> >> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> >> >> >> Cc: Mauro Carvalho Chehab <mchehab@redhat.com>
> >> >> >> ---
> >> >> >>  drivers/media/i2c/tvp514x.c |   22 +++++++++++++++-------
> >> >> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> >> >> >> index 864eb14..d090caf 100644
> >> >> >> --- a/drivers/media/i2c/tvp514x.c
> >> >> >> +++ b/drivers/media/i2c/tvp514x.c
> >> >> >> @@ -36,6 +36,7 @@
> >> >> >>  #include <linux/module.h>
> >> >> >>  #include <linux/v4l2-mediabus.h>
> >> >> >>
> >> >> >> +#include <media/v4l2-async.h>
> >> >> >>  #include <media/v4l2-device.h>
> >> >> >>  #include <media/v4l2-common.h>
> >> >> >>  #include <media/v4l2-mediabus.h>
> >> >> >
> >> >> > Ok, but this one really does too many things in one patch:
> >> >> >
> >> >> >> @@ -1148,9 +1149,9 @@ tvp514x_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >> >> >>       /* Register with V4L2 layer as slave device */
> >> >> >>       sd = &decoder->sd;
> >> >> >>       v4l2_i2c_subdev_init(sd, client, &tvp514x_ops);
> >> >> >> -     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> >> >> >>
> >> >> >>  #if defined(CONFIG_MEDIA_CONTROLLER)
> >> >> >> +     strlcpy(sd->name, TVP514X_MODULE_NAME, sizeof(sd->name));
> >> >> >
> >> >> > This is unrelated
> >> >> >
> >> >> OK I'll split the patch or may be a line in a commit message can do ?
> >> >
> >> > Please split it up in two patches.
> >> >
> >> > Why is sd->name set anyway? And why is it moved under CONFIG_MEDIA_CONTROLLER?
> >> > It's not obvious to me.
> >> >
> >> while using tvp514x subdev with media controller based drivers, when we
> >> enumerate entities (MEDIA_IOC_ENUM_ENTITIES) to get the index id
> >> of the entity we compare the entity name with "tvp514x", So I moved it
> >> under CONFIG_MEDIA_CONTROLLER config. I hope you are OK with
> >> moving this in a separate patch.
> >
> > Sorry, but this approach is wrong. sd->name must be a unique name, so manually
> > setting sd->name will fail if you have two tvp514x devices.
> >
> > There is no reason to override sd->name here, and it is actually a bug. I see
> > that tvp7002 has the same problem (and a bunch of others as well).
> >
> > When trying to find a tvp514x you can just use strstr() in your application.
> > That will work all the time as long as there is only one tvp514x.
> 
> OK, then I will send out a cleanup patch removing sd->name from this driver
> and others too.

Thanks. I see that it is in tvp7002 as well. Also ov9650 and some Samsung devices,
but I will deal with those.

	Hans