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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart