From: "Lad, Prabhakar" <[email protected]>
Both synchronous and asynchronous tvp514x subdevice probing is supported by
this patch.
Signed-off-by: Prabhakar Lad <[email protected]>
Cc: Guennadi Liakhovetski <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Hans Verkuil <[email protected]>
Cc: Sakari Ailus <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
---
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
On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> From: "Lad, Prabhakar" <[email protected]>
>
> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> this patch.
>
> Signed-off-by: Prabhakar Lad <[email protected]>
> Cc: Guennadi Liakhovetski <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Hans Verkuil <[email protected]>
> Cc: Sakari Ailus <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> ---
> 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/
Hi Guennadi,
Thanks for the review.
On Sun, Jun 23, 2013 at 8:49 PM, Guennadi Liakhovetski
<[email protected]> wrote:
> On Sat, 22 Jun 2013, Prabhakar Lad wrote:
>
>> From: "Lad, Prabhakar" <[email protected]>
>>
>> Both synchronous and asynchronous tvp514x subdevice probing is supported by
>> this patch.
>>
>> Signed-off-by: Prabhakar Lad <[email protected]>
>> Cc: Guennadi Liakhovetski <[email protected]>
>> Cc: Laurent Pinchart <[email protected]>
>> Cc: Hans Verkuil <[email protected]>
>> Cc: Sakari Ailus <[email protected]>
>> Cc: Mauro Carvalho Chehab <[email protected]>
>> ---
>> 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
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
> <[email protected]> wrote:
> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> >
> >> From: "Lad, Prabhakar" <[email protected]>
> >>
> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> >> this patch.
> >>
> >> Signed-off-by: Prabhakar Lad <[email protected]>
> >> Cc: Guennadi Liakhovetski <[email protected]>
> >> Cc: Laurent Pinchart <[email protected]>
> >> Cc: Hans Verkuil <[email protected]>
> >> Cc: Sakari Ailus <[email protected]>
> >> Cc: Mauro Carvalho Chehab <[email protected]>
> >> ---
> >> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hi Hans,
On Mon, Jun 24, 2013 at 12:41 PM, Hans Verkuil <[email protected]> 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
>> <[email protected]> wrote:
>> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
>> >
>> >> From: "Lad, Prabhakar" <[email protected]>
>> >>
>> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
>> >> this patch.
>> >>
>> >> Signed-off-by: Prabhakar Lad <[email protected]>
>> >> Cc: Guennadi Liakhovetski <[email protected]>
>> >> Cc: Laurent Pinchart <[email protected]>
>> >> Cc: Hans Verkuil <[email protected]>
>> >> Cc: Sakari Ailus <[email protected]>
>> >> Cc: Mauro Carvalho Chehab <[email protected]>
>> >> ---
>> >> 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
On Mon June 24 2013 10:24:02 Prabhakar Lad wrote:
> Hi Hans,
>
> On Mon, Jun 24, 2013 at 12:41 PM, Hans Verkuil <[email protected]> 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
> >> <[email protected]> wrote:
> >> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> >> >
> >> >> From: "Lad, Prabhakar" <[email protected]>
> >> >>
> >> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> >> >> this patch.
> >> >>
> >> >> Signed-off-by: Prabhakar Lad <[email protected]>
> >> >> Cc: Guennadi Liakhovetski <[email protected]>
> >> >> Cc: Laurent Pinchart <[email protected]>
> >> >> Cc: Hans Verkuil <[email protected]>
> >> >> Cc: Sakari Ailus <[email protected]>
> >> >> Cc: Mauro Carvalho Chehab <[email protected]>
> >> >> ---
> >> >> 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
Hi Hans,
On Mon, Jun 24, 2013 at 2:09 PM, Hans Verkuil <[email protected]> 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 <[email protected]> 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
>> >> <[email protected]> wrote:
>> >> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
>> >> >
>> >> >> From: "Lad, Prabhakar" <[email protected]>
>> >> >>
>> >> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
>> >> >> this patch.
>> >> >>
>> >> >> Signed-off-by: Prabhakar Lad <[email protected]>
>> >> >> Cc: Guennadi Liakhovetski <[email protected]>
>> >> >> Cc: Laurent Pinchart <[email protected]>
>> >> >> Cc: Hans Verkuil <[email protected]>
>> >> >> Cc: Sakari Ailus <[email protected]>
>> >> >> Cc: Mauro Carvalho Chehab <[email protected]>
>> >> >> ---
>> >> >> 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
On Mon June 24 2013 10:53:37 Prabhakar Lad wrote:
> Hi Hans,
>
> On Mon, Jun 24, 2013 at 2:09 PM, Hans Verkuil <[email protected]> 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 <[email protected]> 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
> >> >> <[email protected]> wrote:
> >> >> > On Sat, 22 Jun 2013, Prabhakar Lad wrote:
> >> >> >
> >> >> >> From: "Lad, Prabhakar" <[email protected]>
> >> >> >>
> >> >> >> Both synchronous and asynchronous tvp514x subdevice probing is supported by
> >> >> >> this patch.
> >> >> >>
> >> >> >> Signed-off-by: Prabhakar Lad <[email protected]>
> >> >> >> Cc: Guennadi Liakhovetski <[email protected]>
> >> >> >> Cc: Laurent Pinchart <[email protected]>
> >> >> >> Cc: Hans Verkuil <[email protected]>
> >> >> >> Cc: Sakari Ailus <[email protected]>
> >> >> >> Cc: Mauro Carvalho Chehab <[email protected]>
> >> >> >> ---
> >> >> >> 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