| Message ID | 1371896189-5475-1-git-send-email-prabhakar.csengg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/
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
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 >
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
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
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
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
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);