diff mbox series

[10/14] media: atomisp: Register /dev/* nodes at the end of atomisp_pci_probe()

Message ID 20220901094626.11513-11-hdegoede@redhat.com (mailing list archive)
State Accepted
Headers
Series media: atomisp: More cleanups / code removal |

Commit Message

Hans de Goede Sept. 1, 2022, 9:46 a.m. UTC 
  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(-)

Comments

Andy Shevchenko Sept. 1, 2022, 7:56 p.m. UTC | #1 
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?
Hans de Goede Sept. 2, 2022, 9:04 a.m. UTC | #2 
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
Andy Shevchenko Sept. 2, 2022, 9:10 a.m. UTC | #3 
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.
Andy Shevchenko Sept. 2, 2022, 9:16 a.m. UTC | #4 
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 mbox series

Patch

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;