| Message ID | 20220901094626.11513-11-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | eb81065b9322d6493a152665b4f0974819899c66 |
| Headers | show |
| Series | media: atomisp: More cleanups / code removal | expand |
On Thu, Sep 01, 2022 at 11:46:22AM +0200, Hans de Goede wrote: > Register /dev/* nodes at the end of atomisp_pci_probe(), this is > a prerequisite for dropping the loading mutex + ready flag kludge > for delaying open() calls on the /dev/* nodes . ... > for (; i > 0; i--) > atomisp_subdev_unregister_entities( > &isp->asd[i - 1]); This... > + for (i = 0; i < isp->num_of_streams; i++) { > + err = atomisp_subdev_register_video_nodes(&isp->asd[i], &isp->v4l2_dev); > + if (err) > + return err; > + } ...and this looks like a dup. Perhaps a helper?
Hi, Thank you for reviewing this series. On 9/1/22 21:56, Andy Shevchenko wrote: > On Thu, Sep 01, 2022 at 11:46:22AM +0200, Hans de Goede wrote: >> Register /dev/* nodes at the end of atomisp_pci_probe(), this is >> a prerequisite for dropping the loading mutex + ready flag kludge >> for delaying open() calls on the /dev/* nodes . > > ... > >> for (; i > 0; i--) >> atomisp_subdev_unregister_entities( >> &isp->asd[i - 1]); > > This... I presume you mean the few lines above that actually: @@ -1194,11 +1194,8 @@ static int atomisp_register_entities(struct atomisp_device *isp) struct atomisp_sub_device *asd = &isp->asd[i]; ret = atomisp_subdev_register_subdev(asd, &isp->v4l2_dev); - if (ret == 0) - ret = atomisp_subdev_register_video_nodes(asd, &isp->v4l2_dev); if (ret < 0) { - dev_err(isp->dev, - "atomisp_subdev_register_entities fail\n"); + dev_err(isp->dev, "atomisp_subdev_register_subdev fail\n"); for (; i > 0; i--) atomisp_subdev_unregister_entities( &isp->asd[i - 1]); Notice the atomisp_subdev_register_video_nodes() call is removed there, leaving only atomisp_subdev_register_subdev() (which is also why the dev_err msg is changed). Actually moving that call (+ a few others) to later is the whole purpose of this patch. > >> + for (i = 0; i < isp->num_of_streams; i++) { >> + err = atomisp_subdev_register_video_nodes(&isp->asd[i], &isp->v4l2_dev); >> + if (err) >> + return err; >> + } > > ...and this looks like a dup. Where as this time while looping over the stream the code is calling atomisp_subdev_register_video_nodes(). So yes we have 2 "for (i = 0; i < isp->num_of_streams; i++) {}" loops, but: Loop 1 does: atomisp_subdev_register_subdev() Loop 2 does: atomisp_subdev_register_video_nodes() So I see no duplication here? Regards, Hans
On Fri, Sep 2, 2022 at 12:04 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 9/1/22 21:56, Andy Shevchenko wrote: > > On Thu, Sep 01, 2022 at 11:46:22AM +0200, Hans de Goede wrote: ... > >> for (; i > 0; i--) > >> atomisp_subdev_unregister_entities( > >> &isp->asd[i - 1]); > > > > This... > > I presume you mean the few lines above that actually: No, I cited a not modified code in the upper part. That said, it's a remark for further improvements, but a helper can be introduced in this patch due to the below part. > >> + for (i = 0; i < isp->num_of_streams; i++) { > >> + err = atomisp_subdev_register_video_nodes(&isp->asd[i], &isp->v4l2_dev); > >> + if (err) > >> + return err; > >> + } > > > > ...and this looks like a dup.
On Fri, Sep 2, 2022 at 12:10 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Sep 2, 2022 at 12:04 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 9/1/22 21:56, Andy Shevchenko wrote: > > > On Thu, Sep 01, 2022 at 11:46:22AM +0200, Hans de Goede wrote: > > ... > > > >> for (; i > 0; i--) > > >> atomisp_subdev_unregister_entities( > > >> &isp->asd[i - 1]); > > > > > > This... > > > > I presume you mean the few lines above that actually: > > No, I cited a not modified code in the upper part. That said, it's a > remark for further improvements, but a helper can be introduced in > this patch due to the below part. > > > >> + for (i = 0; i < isp->num_of_streams; i++) { > > >> + err = atomisp_subdev_register_video_nodes(&isp->asd[i], &isp->v4l2_dev); > > >> + if (err) > > >> + return err; > > >> + } > > > > > > ...and this looks like a dup. Pushed "enter" too early to send. But with your explanation I see the difference now.
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c index 9a1eae1ba8c0..f819a6993e45 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c @@ -1194,11 +1194,8 @@ static int atomisp_register_entities(struct atomisp_device *isp) struct atomisp_sub_device *asd = &isp->asd[i]; ret = atomisp_subdev_register_subdev(asd, &isp->v4l2_dev); - if (ret == 0) - ret = atomisp_subdev_register_video_nodes(asd, &isp->v4l2_dev); if (ret < 0) { - dev_err(isp->dev, - "atomisp_subdev_register_entities fail\n"); + dev_err(isp->dev, "atomisp_subdev_register_subdev fail\n"); for (; i > 0; i--) atomisp_subdev_unregister_entities( &isp->asd[i - 1]); @@ -1248,11 +1245,7 @@ static int atomisp_register_entities(struct atomisp_device *isp) dev_warn(isp->dev, "too many atomisp inputs, TPG ignored.\n"); } - ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev); - if (ret < 0) - goto link_failed; - - return media_device_register(&isp->media_dev); + return 0; link_failed: for (i = 0; i < isp->num_of_streams; i++) @@ -1275,6 +1268,27 @@ static int atomisp_register_entities(struct atomisp_device *isp) return ret; } +static int atomisp_register_device_nodes(struct atomisp_device *isp) +{ + int i, err; + + for (i = 0; i < isp->num_of_streams; i++) { + err = atomisp_subdev_register_video_nodes(&isp->asd[i], &isp->v4l2_dev); + if (err) + return err; + } + + err = atomisp_create_pads_links(isp); + if (err) + return err; + + err = v4l2_device_register_subdev_nodes(&isp->v4l2_dev); + if (err) + return err; + + return media_device_register(&isp->media_dev); +} + static int atomisp_initialize_modules(struct atomisp_device *isp) { int ret; @@ -1687,9 +1701,6 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i dev_err(&pdev->dev, "atomisp_register_entities failed (%d)\n", err); goto register_entities_fail; } - err = atomisp_create_pads_links(isp); - if (err < 0) - goto register_entities_fail; /* init atomisp wdts */ err = init_atomisp_wdts(isp); if (err != 0) @@ -1727,8 +1738,13 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i isp->firmware = NULL; isp->css_env.isp_css_fw.data = NULL; isp->ready = true; + rt_mutex_unlock(&isp->loading); + err = atomisp_register_device_nodes(isp); + if (err) + goto css_init_fail; + atomisp_drvfs_init(isp); return 0;
Register /dev/* nodes at the end of atomisp_pci_probe(), this is a prerequisite for dropping the loading mutex + ready flag kludge for delaying open() calls on the /dev/* nodes . Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- .../staging/media/atomisp/pci/atomisp_v4l2.c | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-)