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

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

[Niklas Söderlund]

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>
---
 Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
index fa69bd21c8da..990acf89af8f 100644
--- 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>;
                 };
             };
-- 
2.49.0

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

[Geert Uytterhoeven]

Hi Niklas,

On Wed, 30 Apr 2025 at 14:58, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> 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?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

[Dave Stevenson]

Hi Laurent & Niklas

On Wed, 30 Apr 2025 at 14:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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()

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.

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.

  Dave

> --
> Regards,
>
> Laurent Pinchart

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

[Laurent Pinchart]

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

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

[Dave Stevenson]

On Wed, 30 Apr 2025 at 17:04, Laurent Pinchart
<laurent.pinchart@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@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

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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Apr 30, 2025 at 02:53:22PM +0200, 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>

The new values match the frequencies that the device expects for 4 lanes
operation, so

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx290.yaml
> index fa69bd21c8da..990acf89af8f 100644
> --- 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>;
>                  };
>              };

-- 
Regards,

Laurent Pinchart