[PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Matching on device fwnode handles is deprecated in favour of endpoint
fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() function
to use the latter. The match code handles backward compatibility by
falling by to the device fwnode handle, so this shouldn't introduce any
regression.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0404267f1ae4..67d7f445d429 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -662,7 +662,7 @@ __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif,
 	struct v4l2_async_subdev *asd;
 	struct fwnode_handle *remote;
 
-	remote = fwnode_graph_get_remote_port_parent(endpoint);
+	remote = fwnode_graph_get_remote_endpoint(endpoint);
 	if (!remote)
 		return ERR_PTR(-ENOTCONN);
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Kieran Bingham]

Quoting Laurent Pinchart (2022-03-21 14:51:34)
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Matching on device fwnode handles is deprecated in favour of endpoint
> fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() function
> to use the latter. The match code handles backward compatibility by
> falling by to the device fwnode handle, so this shouldn't introduce any

s/by/back/ ?

It sounds like this helps move other devices towards endpoint matching
more generically too, but I expect there will still be more specifics to
handle in receiver drivers?

And indeed the whole point of the fallback case was to make them
compatible... so:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


But I suspect this needs some testing on platforms that haven't yet
moved to endpoint matching.. ?

> regression.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0404267f1ae4..67d7f445d429 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -662,7 +662,7 @@ __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif,
>         struct v4l2_async_subdev *asd;
>         struct fwnode_handle *remote;
>  
> -       remote = fwnode_graph_get_remote_port_parent(endpoint);
> +       remote = fwnode_graph_get_remote_endpoint(endpoint);
>         if (!remote)
>                 return ERR_PTR(-ENOTCONN);
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>

Re: [PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Laurent Pinchart]

Hi Kieran,

On Sat, Apr 09, 2022 at 08:35:48PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-03-21 14:51:34)
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > Matching on device fwnode handles is deprecated in favour of endpoint
> > fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() function
> > to use the latter. The match code handles backward compatibility by
> > falling by to the device fwnode handle, so this shouldn't introduce any
> 
> s/by/back/ ?

Indeed.

> It sounds like this helps move other devices towards endpoint matching
> more generically too, but I expect there will still be more specifics to
> handle in receiver drivers?
> 
> And indeed the whole point of the fallback case was to make them
> compatible... so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 
> But I suspect this needs some testing on platforms that haven't yet
> moved to endpoint matching.. ?

The more testing the better :-)

> > regression.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 0404267f1ae4..67d7f445d429 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -662,7 +662,7 @@ __v4l2_async_nf_add_fwnode_remote(struct v4l2_async_notifier *notif,
> >         struct v4l2_async_subdev *asd;
> >         struct fwnode_handle *remote;
> >  
> > -       remote = fwnode_graph_get_remote_port_parent(endpoint);
> > +       remote = fwnode_graph_get_remote_endpoint(endpoint);
> >         if (!remote)
> >                 return ERR_PTR(-ENOTCONN);
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Marek Szyprowski]

Hi Laurent,

On 21.03.2022 15:51, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> Matching on device fwnode handles is deprecated in favour of endpoint
> fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() function
> to use the latter. The match code handles backward compatibility by
> falling by to the device fwnode handle, so this shouldn't introduce any
> regression.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I love the last sentence of the patch description. :)

Unfortunately, recently I found that this patch breaks Exynos4 IS (FIMC) 
driver operation on Trats2 board (exynos4412-trats2.dts). After merging 
this patch I see the following errors related to the camera sensors:

[   16.038705] S5C73M3: S5C73M3 SPI probed successfully
[   16.097399] S5C73M3: Sensor type: CML0801-M017, FW version: GDFD01
[   16.106842] S5C73M3 0-003c: Consider updating driver S5C73M3 to match 
on endpoints
[   16.298323] S5C73M3: probe of 0-003c failed with error -22
[   16.343173] S5K6A3 15-0010: Consider updating driver S5K6A3 to match 
on endpoints
[   16.434968] S5K6A3: probe of 15-0010 failed with error -22

I'm a bit puzzled, because I don't see anything related to endpoint 
matching in the sensor drivers. Instead I only found that 
v4l2_async_nf_add_fwnode_remote() function (which is modified by this 
patch) is called from the 
drivers/media/platform/samsung/exynos4-is/media-dev.c code. Do you have 
any idea what is broken after this change? Could you help fixing this issue?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Sakari Ailus]

Hi, Marek!

Good to hear from you! And it's so nice you're testing the Samsung Exynos
drivers! :-)

On Thu, Sep 22, 2022 at 04:06:58PM +0200, Marek Szyprowski wrote:
> Hi Laurent,
> 
> On 21.03.2022 15:51, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Matching on device fwnode handles is deprecated in favour of endpoint
> > fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() function
> > to use the latter. The match code handles backward compatibility by
> > falling by to the device fwnode handle, so this shouldn't introduce any
> > regression.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> I love the last sentence of the patch description. :)
> 
> Unfortunately, recently I found that this patch breaks Exynos4 IS (FIMC) 
> driver operation on Trats2 board (exynos4412-trats2.dts). After merging 
> this patch I see the following errors related to the camera sensors:
> 
> [   16.038705] S5C73M3: S5C73M3 SPI probed successfully
> [   16.097399] S5C73M3: Sensor type: CML0801-M017, FW version: GDFD01
> [   16.106842] S5C73M3 0-003c: Consider updating driver S5C73M3 to match 
> on endpoints
> [   16.298323] S5C73M3: probe of 0-003c failed with error -22
> [   16.343173] S5K6A3 15-0010: Consider updating driver S5K6A3 to match 
> on endpoints
> [   16.434968] S5K6A3: probe of 15-0010 failed with error -22

Have you checked what exactly caused the probe to fail? Laurent's patch
changes how matching works but if that fails, the result should be a bunch
of waiting async sub-devices and notifier(s), not a failure on probe.

> 
> I'm a bit puzzled, because I don't see anything related to endpoint 
> matching in the sensor drivers. Instead I only found that 
> v4l2_async_nf_add_fwnode_remote() function (which is modified by this 
> patch) is called from the 
> drivers/media/platform/samsung/exynos4-is/media-dev.c code. Do you have 
> any idea what is broken after this change? Could you help fixing this issue?

You can assign the endpoint node to subdev->fwnode instead of the device
fwnode. No regular sensor driver appears to be doing that though.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Daniel Scally]

Hi all

On 22/09/2022 23:48, Sakari Ailus wrote:
> Hi, Marek!
>
> Good to hear from you! And it's so nice you're testing the Samsung Exynos
> drivers! :-)
>
> On Thu, Sep 22, 2022 at 04:06:58PM +0200, Marek Szyprowski wrote:
>> Hi Laurent,
>>
>> On 21.03.2022 15:51, Laurent Pinchart wrote:
>>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>
>>> Matching on device fwnode handles is deprecated in favour of endpoint
>>> fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() function
>>> to use the latter. The match code handles backward compatibility by
>>> falling by to the device fwnode handle, so this shouldn't introduce any
>>> regression.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> I love the last sentence of the patch description. :)
>>
>> Unfortunately, recently I found that this patch breaks Exynos4 IS (FIMC)
>> driver operation on Trats2 board (exynos4412-trats2.dts). After merging
>> this patch I see the following errors related to the camera sensors:
>>
>> [   16.038705] S5C73M3: S5C73M3 SPI probed successfully
>> [   16.097399] S5C73M3: Sensor type: CML0801-M017, FW version: GDFD01
>> [   16.106842] S5C73M3 0-003c: Consider updating driver S5C73M3 to match
>> on endpoints
>> [   16.298323] S5C73M3: probe of 0-003c failed with error -22
>> [   16.343173] S5K6A3 15-0010: Consider updating driver S5K6A3 to match
>> on endpoints
>> [   16.434968] S5K6A3: probe of 15-0010 failed with error -22
> Have you checked what exactly caused the probe to fail? Laurent's patch
> changes how matching works but if that fails, the result should be a bunch
> of waiting async sub-devices and notifier(s), not a failure on probe.


Might be some other effects too...I can't test it but in 
subdev_notifier_bound() in 
drivers/media/platform/samsung/exynos4-is/media-dev.c there's the 
following check:


     /* Find platform data for this sensor subdev */
     for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
         if (fmd->sensor[i].asd &&
             fmd->sensor[i].asd->match.fwnode ==
             of_fwnode_handle(subdev->dev->of_node))
             si = &fmd->sensor[i];

     if (si == NULL)
         return -EINVAL;


And I think following that patch of Laurent's asd->match.fwnode will 
never be the dev->of_node anymore, because it's now the endpoint that's 
assigned as asd->match.fwnode rather than that of the device. That will 
return -EINVAL for the notifier's .bound() callback...I'm not sure if 
that would cause the whole probe to fail, but thought it might be worth 
mentioning :)

>
>> I'm a bit puzzled, because I don't see anything related to endpoint
>> matching in the sensor drivers. Instead I only found that
>> v4l2_async_nf_add_fwnode_remote() function (which is modified by this
>> patch) is called from the
>> drivers/media/platform/samsung/exynos4-is/media-dev.c code. Do you have
>> any idea what is broken after this change? Could you help fixing this issue?
> You can assign the endpoint node to subdev->fwnode instead of the device
> fwnode. No regular sensor driver appears to be doing that though.
>

Re: [PATCH] media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()

[Marek Szyprowski]

Hi Daniel,

On 23.09.2022 01:19, Daniel Scally wrote:
> On 22/09/2022 23:48, Sakari Ailus wrote:
>> Good to hear from you! And it's so nice you're testing the Samsung 
>> Exynos
>> drivers! :-)
>>
>> On Thu, Sep 22, 2022 at 04:06:58PM +0200, Marek Szyprowski wrote:
>>> On 21.03.2022 15:51, Laurent Pinchart wrote:
>>>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>>>
>>>> Matching on device fwnode handles is deprecated in favour of endpoint
>>>> fwnode handles. Switch the __v4l2_async_nf_add_fwnode_remote() 
>>>> function
>>>> to use the latter. The match code handles backward compatibility by
>>>> falling by to the device fwnode handle, so this shouldn't introduce 
>>>> any
>>>> regression.
>>>>
>>>> Signed-off-by: Laurent Pinchart 
>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>> I love the last sentence of the patch description. :)
>>>
>>> Unfortunately, recently I found that this patch breaks Exynos4 IS 
>>> (FIMC)
>>> driver operation on Trats2 board (exynos4412-trats2.dts). After merging
>>> this patch I see the following errors related to the camera sensors:
>>>
>>> [   16.038705] S5C73M3: S5C73M3 SPI probed successfully
>>> [   16.097399] S5C73M3: Sensor type: CML0801-M017, FW version: GDFD01
>>> [   16.106842] S5C73M3 0-003c: Consider updating driver S5C73M3 to 
>>> match
>>> on endpoints
>>> [   16.298323] S5C73M3: probe of 0-003c failed with error -22
>>> [   16.343173] S5K6A3 15-0010: Consider updating driver S5K6A3 to match
>>> on endpoints
>>> [   16.434968] S5K6A3: probe of 15-0010 failed with error -22
>> Have you checked what exactly caused the probe to fail? Laurent's patch
>> changes how matching works but if that fails, the result should be a 
>> bunch
>> of waiting async sub-devices and notifier(s), not a failure on probe.
>
>
> Might be some other effects too...I can't test it but in 
> subdev_notifier_bound() in 
> drivers/media/platform/samsung/exynos4-is/media-dev.c there's the 
> following check:
>
>
>     /* Find platform data for this sensor subdev */
>     for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
>         if (fmd->sensor[i].asd &&
>             fmd->sensor[i].asd->match.fwnode ==
>             of_fwnode_handle(subdev->dev->of_node))
>             si = &fmd->sensor[i];
>
>     if (si == NULL)
>         return -EINVAL;
>
>
> And I think following that patch of Laurent's asd->match.fwnode will 
> never be the dev->of_node anymore, because it's now the endpoint 
> that's assigned as asd->match.fwnode rather than that of the device. 
> That will return -EINVAL for the notifier's .bound() callback...I'm 
> not sure if that would cause the whole probe to fail, but thought it 
> might be worth mentioning :)

Good catch, thanks!

I have no idea why that loop compared the asd->match.fwnode and 
subdev->dev->of_node, maybe it some kind of historical left-over or 
cargo-cult. The asd pointer is already available there and can be used 
to find the proper sensor. This loop works both with the new and old 
version:

         /* Find platform data for this sensor subdev */
         for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
                 if (fmd->sensor[i].asd == asd)
                         si = &fmd->sensor[i];

I will prepare a patch in a few minutes.

>>> I'm a bit puzzled, because I don't see anything related to endpoint
>>> matching in the sensor drivers. Instead I only found that
>>> v4l2_async_nf_add_fwnode_remote() function (which is modified by this
>>> patch) is called from the
>>> drivers/media/platform/samsung/exynos4-is/media-dev.c code. Do you have
>>> any idea what is broken after this change? Could you help fixing 
>>> this issue?
>> You can assign the endpoint node to subdev->fwnode instead of the device
>> fwnode. No regular sensor driver appears to be doing that though.
>>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland