| Message ID | 20240328051320.2428125-2-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: ov2680: Allow probing if link-frequencies is absent | expand |
Hi Fabio, On 3/28/24 6:13 AM, Fabio Estevam wrote: > From: Fabio Estevam <festevam@denx.de> > > When passing the correct 'link-frequencies' in the DT, the > driver should report success on the match case: > > port { > ov2680_to_mipi: endpoint { > remote-endpoint = <&mipi_from_sensor>; > clock-lanes = <0>; > data-lanes = <1>; > link-frequencies = /bits/ 64 <330000000>; > }; > }; > > However, this does not happen and the probe fails like this: > > ov2680 1-0036: probe with driver ov2680 failed with error -22 > > Fix it by returning success upon link-frequency match. > > Also tested passing a wrong link-frequencies value in th DT and > confirmed that the driver correctly rejects it. > > Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification") > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > drivers/media/i2c/ov2680.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index f611ce3a749c..37c21749dc14 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -1128,7 +1128,7 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) > > for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) > if (bus_cfg.link_frequencies[i] == sensor->link_freq[0]) > - break; > + return 0; If you need this then that suggests that bus_cfg.nr_of_link_frequencies != 0 otherwise this patch will not make a difference. So that suggests that patch 1/2 is not necessary ? And if bus_cfg.nr_of_link_frequencies != 0 and we break then: > > if (bus_cfg.nr_of_link_frequencies == 0 || > bus_cfg.nr_of_link_frequencies == i) { This will never be true (neither condition is true) so we will continue with a clean exit of the function. Except that that clean exit does "return ret" and I see there may some paths where that is not 0 even though we are doing a clean exit. I think that what is necessary for your case with fixed dts file is: diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index bcd031882a37..5c789b5a4bfb 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -1179,6 +1179,8 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) goto out_free_bus_cfg; } + ret = 0; + out_free_bus_cfg: v4l2_fwnode_endpoint_free(&bus_cfg); return ret; and that then replaces both your patches, can you give this a try ? Regards, Hans p.s. Your early return 0 in this patch also leaks the bus_cfg.
Hi Hans, [Adding DT folks] On Thu, Mar 28, 2024 at 8:27 AM Hans de Goede <hdegoede@redhat.com> wrote: > I think that what is necessary for your case with fixed dts file is: > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index bcd031882a37..5c789b5a4bfb 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -1179,6 +1179,8 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) > goto out_free_bus_cfg; > } > > + ret = 0; > + > out_free_bus_cfg: > v4l2_fwnode_endpoint_free(&bus_cfg); > return ret; > > and that then replaces both your patches, can you give this a try ? This works fine if I pass link-frequencies in the dts, thanks. --- a/arch/arm/boot/dts/nxp/imx/imx7s-warp.dts +++ b/arch/arm/boot/dts/nxp/imx/imx7s-warp.dts @@ -210,6 +210,7 @@ ov2680_to_mipi: endpoint { remote-endpoint = <&mipi_from_sensor>; clock-lanes = <0>; data-lanes = <1>; + link-frequencies = /bits/ 64 <340000000>; }; }; }; Can we allow the probe to succeed even if 'link frequencies' is absent? That was my goal on patch 1/2: to keep existing dtb's functional. Otherwise, the old dtb's without 'link-frequencies' will be broken and I'm not sure if the DT folks will accept a patch passing link-frequencies to imx7s-warp.dts as a fix to be backported to 6.6. ovti,ov2680.yaml will also need to be changed to include 'link-frequencies' as a required property. Thoughts?
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index f611ce3a749c..37c21749dc14 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -1128,7 +1128,7 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor) for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) if (bus_cfg.link_frequencies[i] == sensor->link_freq[0]) - break; + return 0; if (bus_cfg.nr_of_link_frequencies == 0 || bus_cfg.nr_of_link_frequencies == i) {