[PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
property verification") the ov2680 no longer probes on a imx7s-warp7:

ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
ov2680 1-0036: probe with driver ov2680 failed with error -2

As the 'link-frequencies' property is not mandatory, allow the probe
to succeed by skipping the link-frequency verification when the
property is absent.

Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 drivers/media/i2c/ov2680.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index 39d321e2b7f9..f611ce3a749c 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -1123,6 +1123,9 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
 		goto out_free_bus_cfg;
 	}
 
+	if (!bus_cfg.nr_of_link_frequencies)
+		return 0;
+
 	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
 		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
 			break;
-- 
2.34.1

[PATCH 2/2] media: ov2680: Report success on link-frequency match

[Fabio Estevam]

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 (bus_cfg.nr_of_link_frequencies == 0 ||
 	    bus_cfg.nr_of_link_frequencies == i) {
-- 
2.34.1

Re: [PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[Sakari Ailus]

Hi Fabio,

On Thu, Mar 28, 2024 at 02:13:19AM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
> 
> Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
> property verification") the ov2680 no longer probes on a imx7s-warp7:
> 
> ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
> ov2680 1-0036: probe with driver ov2680 failed with error -2
> 
> As the 'link-frequencies' property is not mandatory, allow the probe
> to succeed by skipping the link-frequency verification when the
> property is absent.
> 
> Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
>  drivers/media/i2c/ov2680.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index 39d321e2b7f9..f611ce3a749c 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -1123,6 +1123,9 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
>  		goto out_free_bus_cfg;
>  	}
>  
> +	if (!bus_cfg.nr_of_link_frequencies)
> +		return 0;
> +

Thanks for the patch.

I'd still rather try to avoid going to this direction as these frequencies
are hardware dependent. Add a new one to the driver and some boards may
stop working properly. For details see
<URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.

>  	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
>  		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
>  			break;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: ov2680: Report success on link-frequency match

[Hans de Goede]

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.

Re: [PATCH 2/2] media: ov2680: Report success on link-frequency match

[Fabio Estevam]

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?

Re: [PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[Sakari Ailus]

On Thu, Mar 28, 2024 at 07:54:41AM +0000, Sakari Ailus wrote:
> Hi Fabio,
> 
> On Thu, Mar 28, 2024 at 02:13:19AM -0300, Fabio Estevam wrote:
> > From: Fabio Estevam <festevam@denx.de>
> > 
> > Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
> > property verification") the ov2680 no longer probes on a imx7s-warp7:
> > 
> > ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
> > ov2680 1-0036: probe with driver ov2680 failed with error -2
> > 
> > As the 'link-frequencies' property is not mandatory, allow the probe
> > to succeed by skipping the link-frequency verification when the
> > property is absent.
> > 
> > Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
> > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > ---
> >  drivers/media/i2c/ov2680.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> > index 39d321e2b7f9..f611ce3a749c 100644
> > --- a/drivers/media/i2c/ov2680.c
> > +++ b/drivers/media/i2c/ov2680.c
> > @@ -1123,6 +1123,9 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
> >  		goto out_free_bus_cfg;
> >  	}
> >  
> > +	if (!bus_cfg.nr_of_link_frequencies)
> > +		return 0;
> > +
> 
> Thanks for the patch.
> 
> I'd still rather try to avoid going to this direction as these frequencies
> are hardware dependent. Add a new one to the driver and some boards may
> stop working properly. For details see
> <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.

While the above holds, on second thought, the driver has been around for
quite some time and indeed the added validation does break certain boards
(at least without adding the link frequencies there).

So I'm fine with the patch.

I think this should also be cc'd to stable.

> 
> >  	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> >  		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
> >  			break;
> 

-- 
Sakari Ailus

Re: [PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[Sakari Ailus]

On Thu, Mar 28, 2024 at 09:55:48PM +0000, Sakari Ailus wrote:
> On Thu, Mar 28, 2024 at 07:54:41AM +0000, Sakari Ailus wrote:
> > Hi Fabio,
> > 
> > On Thu, Mar 28, 2024 at 02:13:19AM -0300, Fabio Estevam wrote:
> > > From: Fabio Estevam <festevam@denx.de>
> > > 
> > > Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
> > > property verification") the ov2680 no longer probes on a imx7s-warp7:
> > > 
> > > ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
> > > ov2680 1-0036: probe with driver ov2680 failed with error -2
> > > 
> > > As the 'link-frequencies' property is not mandatory, allow the probe
> > > to succeed by skipping the link-frequency verification when the
> > > property is absent.
> > > 
> > > Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
> > > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > > ---
> > >  drivers/media/i2c/ov2680.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> > > index 39d321e2b7f9..f611ce3a749c 100644
> > > --- a/drivers/media/i2c/ov2680.c
> > > +++ b/drivers/media/i2c/ov2680.c
> > > @@ -1123,6 +1123,9 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
> > >  		goto out_free_bus_cfg;
> > >  	}
> > >  
> > > +	if (!bus_cfg.nr_of_link_frequencies)
> > > +		return 0;
> > > +
> > 
> > Thanks for the patch.
> > 
> > I'd still rather try to avoid going to this direction as these frequencies
> > are hardware dependent. Add a new one to the driver and some boards may
> > stop working properly. For details see
> > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.
> 
> While the above holds, on second thought, the driver has been around for
> quite some time and indeed the added validation does break certain boards
> (at least without adding the link frequencies there).
> 
> So I'm fine with the patch.
> 
> I think this should also be cc'd to stable.

And this applies to the newer version of the patch that fixes the memory
leak, of course.

> 
> > 
> > >  	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > >  		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
> > >  			break;
> > 
> 
> -- 
> Sakari Ailus

-- 
Sakari Ailus

Re: [PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[Laurent Pinchart]

On Thu, Mar 28, 2024 at 09:57:46PM +0000, Sakari Ailus wrote:
> On Thu, Mar 28, 2024 at 09:55:48PM +0000, Sakari Ailus wrote:
> > On Thu, Mar 28, 2024 at 07:54:41AM +0000, Sakari Ailus wrote:
> > > Hi Fabio,
> > > 
> > > On Thu, Mar 28, 2024 at 02:13:19AM -0300, Fabio Estevam wrote:
> > > > From: Fabio Estevam <festevam@denx.de>
> > > > 
> > > > Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
> > > > property verification") the ov2680 no longer probes on a imx7s-warp7:
> > > > 
> > > > ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
> > > > ov2680 1-0036: probe with driver ov2680 failed with error -2
> > > > 
> > > > As the 'link-frequencies' property is not mandatory, allow the probe
> > > > to succeed by skipping the link-frequency verification when the
> > > > property is absent.
> > > > 
> > > > Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
> > > > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > > > ---
> > > >  drivers/media/i2c/ov2680.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> > > > index 39d321e2b7f9..f611ce3a749c 100644
> > > > --- a/drivers/media/i2c/ov2680.c
> > > > +++ b/drivers/media/i2c/ov2680.c
> > > > @@ -1123,6 +1123,9 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
> > > >  		goto out_free_bus_cfg;
> > > >  	}
> > > >  
> > > > +	if (!bus_cfg.nr_of_link_frequencies)
> > > > +		return 0;
> > > > +
> > > 
> > > Thanks for the patch.
> > > 
> > > I'd still rather try to avoid going to this direction as these frequencies
> > > are hardware dependent. Add a new one to the driver and some boards may
> > > stop working properly. For details see
> > > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.
> > 
> > While the above holds, on second thought, the driver has been around for
> > quite some time and indeed the added validation does break certain boards
> > (at least without adding the link frequencies there).
> > 
> > So I'm fine with the patch.
> > 
> > I think this should also be cc'd to stable.
> 
> And this applies to the newer version of the patch that fixes the memory
> leak, of course.

Should we add least log a warning when link frequencies are not present
in DT ?

> > > >  	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > >  		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
> > > >  			break;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: ov2680: Allow probing if link-frequencies is absent

[Sakari Ailus]

On Fri, Mar 29, 2024 at 12:04:03AM +0200, Laurent Pinchart wrote:
> On Thu, Mar 28, 2024 at 09:57:46PM +0000, Sakari Ailus wrote:
> > On Thu, Mar 28, 2024 at 09:55:48PM +0000, Sakari Ailus wrote:
> > > On Thu, Mar 28, 2024 at 07:54:41AM +0000, Sakari Ailus wrote:
> > > > Hi Fabio,
> > > > 
> > > > On Thu, Mar 28, 2024 at 02:13:19AM -0300, Fabio Estevam wrote:
> > > > > From: Fabio Estevam <festevam@denx.de>
> > > > > 
> > > > > Since commit 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint
> > > > > property verification") the ov2680 no longer probes on a imx7s-warp7:
> > > > > 
> > > > > ov2680 1-0036: error -EINVAL: supported link freq 330000000 not found
> > > > > ov2680 1-0036: probe with driver ov2680 failed with error -2
> > > > > 
> > > > > As the 'link-frequencies' property is not mandatory, allow the probe
> > > > > to succeed by skipping the link-frequency verification when the
> > > > > property is absent.
> > > > > 
> > > > > Fixes: 63b0cd30b78e ("media: ov2680: Add bus-cfg / endpoint property verification")
> > > > > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > > > > ---
> > > > >  drivers/media/i2c/ov2680.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> > > > > index 39d321e2b7f9..f611ce3a749c 100644
> > > > > --- a/drivers/media/i2c/ov2680.c
> > > > > +++ b/drivers/media/i2c/ov2680.c
> > > > > @@ -1123,6 +1123,9 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
> > > > >  		goto out_free_bus_cfg;
> > > > >  	}
> > > > >  
> > > > > +	if (!bus_cfg.nr_of_link_frequencies)
> > > > > +		return 0;
> > > > > +
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > I'd still rather try to avoid going to this direction as these frequencies
> > > > are hardware dependent. Add a new one to the driver and some boards may
> > > > stop working properly. For details see
> > > > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>.
> > > 
> > > While the above holds, on second thought, the driver has been around for
> > > quite some time and indeed the added validation does break certain boards
> > > (at least without adding the link frequencies there).
> > > 
> > > So I'm fine with the patch.
> > > 
> > > I think this should also be cc'd to stable.
> > 
> > And this applies to the newer version of the patch that fixes the memory
> > leak, of course.
> 
> Should we add least log a warning when link frequencies are not present
> in DT ?

Sounds reasonable.

> 
> > > > >  	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > >  		if (bus_cfg.link_frequencies[i] == sensor->link_freq[0])
> > > > >  			break;

-- 
Sakari Ailus