| Message ID | 20240328051320.2428125-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] media: ov2680: Allow probing if link-frequencies is absent | expand |
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;
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; >
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
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;
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;
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;