| Message ID | 20230525091615.2324824-5-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:15:47PM +0300, Sakari Ailus wrote: > There's no need to check for a sub-device's notifier as we only register > one notifier (and one V4L2 device). Remove this check and prepare for > removing this field in struct v4l2_subdev. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/platform/ti/omap3isp/isp.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c > index f3aaa9e76492e..c2b222f7df892 100644 > --- a/drivers/media/platform/ti/omap3isp/isp.c > +++ b/drivers/media/platform/ti/omap3isp/isp.c > @@ -2039,9 +2039,6 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async) > } > > list_for_each_entry(sd, &v4l2_dev->subdevs, list) { > - if (sd->notifier != &isp->notifier) > - continue; I don't think this is quite right. You could have a chain of external subdevs, in which case only the one connected directly to the ISP should be linked to the ISP. > - > ret = isp_link_entity(isp, &sd->entity, > v4l2_subdev_to_bus_cfg(sd)->interface); > if (ret < 0) {
Hi Laurent, Thanks for the review. On Tue, May 30, 2023 at 05:23:23AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:47PM +0300, Sakari Ailus wrote: > > There's no need to check for a sub-device's notifier as we only register > > one notifier (and one V4L2 device). Remove this check and prepare for > > removing this field in struct v4l2_subdev. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/platform/ti/omap3isp/isp.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c > > index f3aaa9e76492e..c2b222f7df892 100644 > > --- a/drivers/media/platform/ti/omap3isp/isp.c > > +++ b/drivers/media/platform/ti/omap3isp/isp.c > > @@ -2039,9 +2039,6 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async) > > } > > > > list_for_each_entry(sd, &v4l2_dev->subdevs, list) { > > - if (sd->notifier != &isp->notifier) > > - continue; > > I don't think this is quite right. You could have a chain of external > subdevs, in which case only the one connected directly to the ISP should > be linked to the ISP. You're right, in principle this could take place. Going forward sub-devices may be bound to multiple notifiers so this field should go. I'll see if we could have another test for this.
diff --git a/drivers/media/platform/ti/omap3isp/isp.c b/drivers/media/platform/ti/omap3isp/isp.c index f3aaa9e76492e..c2b222f7df892 100644 --- a/drivers/media/platform/ti/omap3isp/isp.c +++ b/drivers/media/platform/ti/omap3isp/isp.c @@ -2039,9 +2039,6 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async) } list_for_each_entry(sd, &v4l2_dev->subdevs, list) { - if (sd->notifier != &isp->notifier) - continue; - ret = isp_link_entity(isp, &sd->entity, v4l2_subdev_to_bus_cfg(sd)->interface); if (ret < 0) {
There's no need to check for a sub-device's notifier as we only register one notifier (and one V4L2 device). Remove this check and prepare for removing this field in struct v4l2_subdev. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/platform/ti/omap3isp/isp.c | 3 --- 1 file changed, 3 deletions(-)