[PATCH] media: dt-bindings: sony,imx290: Update usage example

[Dave Stevenson]

On Wed, 30 Apr 2025 at 17:04, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 at ragnatech.se>
> > > >
> > > > 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.

I have no extra information there.
The minimal changes that are made to INCKSEL 1 & 2 to switch between
the two link frequencies makes it look pretty fixed rather than
flexible PLLs, and I haven't got the time to go randomly poking to
reverse engineer it.

  Dave