On Wed, Apr 30, 2025 at 02:52:01PM +0100, Dave Stevenson wrote:
> On Wed, 30 Apr 2025 at 14:19, Laurent Pinchart wrote:
> > On Wed, Apr 30, 2025 at 03:03:10PM +0200, Geert Uytterhoeven wrote:
> > > On Wed, 30 Apr 2025 at 14:58, Niklas Söderlund wrote:
> > > > Since commit 98e0500eadb7 ("media: i2c: imx290: Add configurable link
> > > > frequency and pixel rate") the driver expects two specific
> > > > link-frequency settings 2-lane (445500000, 297000000) and 4-lane
> > > > (222750000, 148500000) operation. The driver fails to probe without
> > > > these exact settings.
> > > >
> > > > Update the example in the bindings to match this to make it easier for
> > > > users to incorporate this sensor in their device tree descriptions
> > > > without having to read the driver sources when the driver fails to
> > > > probe.
> > > >
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> > > > @@ -136,7 +136,7 @@ examples:
> > > > port {
> > > > imx290_ep: endpoint {
> > > > data-lanes = <1 2 3 4>;
> > > > - link-frequencies = /bits/ 64 <445500000>;
> > > > + link-frequencies = /bits/ 64 <222750000 148500000>;
> > > > remote-endpoint = <&csiphy0_ep>;
> > > > };
> > > > };
> > >
> > > I guess the link-frequencies property should gain a rule that it
> > > needs two values, too?
> >
> > The driver doesn't require two frequencies (unless I'm mistaken), it
> > could operate with a single one (albeit not in all resolutions), so I
> > don't think we should require two frequencies in the bindings.
>
> The driver does require both due to 98e0500eadb7 ("media: i2c: imx290:
> Add configurable link frequency and pixel rate") and
> imx290_check_link_freqs()
I realized after sending the previous e-mail that I was indeed mistaken.
I thought the driver iterated over the DT link frequencies to check if
they're supported, but it goes the other way around.
> However I'd agree that it'd be better to make the driver accept just
> the one and make any compensations, rather than amend the binding. I'm
> happy to try and find a few minutes to make a patch for that.
That would be nice, thanks.
> My experience of this family of sensors says that we should be able to
> run any resolution at any link frequency, but it needs changes to
> HBLANK to ensure there is sufficient time per line.
> Dropping to the lower link freq for the 720p mode is only because that
> is what the datasheet describes for the precanned HD720p. The window
> cropping mode lists no such requirement, and yet could produce exactly
> that same 720p output.
And with more information about the INCKSEL registers we could possibly
even support other frequencies.
--
Regards,
Laurent Pinchart
