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@xxxxxxxxxxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxxxxx> >>> 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
