Re: [PATCH v3 2/6] dt-bindings: soc: imx93-media-blk-ctrl: Add LDB subnode into schema and example

[Alexander Stein]

Hi,

Am Mittwoch, 5. März 2025, 08:13:04 CET schrieb Krzysztof Kozlowski:
> On Tue, Mar 04, 2025 at 04:49:21PM +0100, Alexander Stein wrote:
> > Document the LDB bridge subnode and add the subnode into the example.
> > For the subnode to work, the block control must scan its subnodes and
> 
> Don't describe drivers, but describe the hardware.

Thanks, I'll rephrase to describe the hardware better regarding LVDS.

> 
> > bind drivers to them, do not misuse either simple-bus or simple-mfd
> > here.
> 
> I don't understand that simple-bus or simple-mfd statement. There are no
> such compatibles here.

Same as above, the wording stems from 1cb0c87d27dcc ("dt-bindings: soc:
imx8mp-media-blk-ctrl: Add LDB subnode into schema and example").
I'll drop it to avoid confusion.

> 
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > ---
> >  .../soc/imx/fsl,imx93-media-blk-ctrl.yaml     | 51 +++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml
> > index b3554e7f9e76d..cd785111928bf 100644
> > --- a/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml
> > +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx93-media-blk-ctrl.yaml
> > @@ -24,6 +24,14 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > +  ranges: true
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 1
> > +
> >    '#power-domain-cells':
> >      const: 1
> >  
> > @@ -46,9 +54,20 @@ properties:
> >        - const: csi
> >        - const: dsi
> >  
> > +  bridge@20:
> 
> @20 looks wrong. Use 'ranges;' and try again your DTS...
> 
> Binding is supposed to be complete. We have several examples when people
> added children one-by-one, everytime with different reasoning about
> child addressing.
> 
> So please confirm: this is complete and no other children will ever be
> added here... or you are 100% sure that all future children will be
> unit-addressable (will have unit address and appropriate properties).

This block control is a collection of registers for different purposes:
* MIPI-DSI
* MIPI-CSI
* Parallel camera
* LVDS
* CAMERA_MUX

At lease for parallel camera, another subnode is expected ([1]).

[1] https://lore.kernel.org/all/20240819024001.850065-1-victor.liu@xxxxxxx/

> BTW, I don't quite get why this is both syscon and has translation for
> child addresses. Does it mean your child does not use the same MMIO as
> parent, thus leading to unsynchronized reg access?

I'm not sure what the best practices are. This LDB has two registers
inside this block. So it seems reasonable to me to indicate this using
a reg property. On the other hand, access is solely done by accessing
via syscon, so unsynchronized reg access is not an issue.

What I am getting from your comments this node should not have 'reg'
property, as it uses syscon anyway.

> > +    type: object
> > +    additionalProperties: true
> > +    properties:
> > +      compatible:
> > +        contains:
> > +          const: fsl,imx93-ldb
> > +
> >  required:
> >    - compatible
> >    - reg
> > +  - ranges
> > +  - '#address-cells'
> > +  - '#size-cells'
> >    - power-domains
> >    - clocks
> >    - clock-names
> > @@ -77,4 +96,36 @@ examples:
> >                 clock-names = "apb", "axi", "nic", "disp", "cam",
> >                               "pxp", "lcdif", "isi", "csi", "dsi";
> >        #power-domain-cells = <1>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +      ranges = <0x0 0x4ac10000 0x10000>;
> > +
> > +      bridge@20 {
> > +          compatible = "fsl,imx93-ldb";
> > +          reg = <0x20 0x4>, <0x24 0x4>;
> > +          reg-names = "ldb", "lvds";
> > +          clocks = <&clk IMX93_CLK_LVDS_GATE>;
> > +          clock-names = "ldb";
> > +
> > +          ports {
> > +              #address-cells = <1>;
> > +              #size-cells = <0>;
> > +
> > +              port@0 {
> > +                  reg = <0>;
> > +
> > +                  ldb_from_lcdif2: endpoint {
> > +                      remote-endpoint = <&lcdif2_to_ldb>;
> > +                  };
> > +              };
> > +
> > +              port@1 {
> > +                  reg = <1>;
> > +
> > +                  ldb_lvds: endpoint {
> > +                      remote-endpoint = <&ldb_to_panel>;
> > +                  };
> > +              };
> > +          };
> 
> Messed indentation.

This is already from the original binding. I'll fix in a separate commit.

Best regards,
Alexander

> 
> > +        };
> 
> Best regards,
> Krzysztof
> 
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/