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