Re: [PATCH v2 01/14] dt-bindings: phy: Add MIPI CSI PHY for i.MX8Q

[Frank Li <Frank.li@nxp.com>]

On Mon, Feb 10, 2025 at 11:10:13PM +0200, Laurent Pinchart wrote:
> On Thu, Feb 06, 2025 at 04:48:55PM -0500, Frank Li wrote:
> > On Thu, Feb 06, 2025 at 11:18:08PM +0200, Laurent Pinchart wrote:
> > > On Wed, Feb 05, 2025 at 12:18:10PM -0500, Frank Li wrote:
> > > > Add MIPI CSI phy binding doc for i.MX8QXP, i.MX8QM and i.MX8ULP.
> > >
> > > s/CSI/CSI-2/ in the subject line, here and below.
> > > s/phy/PHY/
> > >
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > > ---
> > > > change from v1 to v2
> > > > - Add missed fsl,imx8qm-mipi-cphy, which failback to fsl,imx8qxp-mipi-cphy
> > > > - Move reg to required. Previous 8ulp use fsl,offset in downstream version.
> > > > which should be reg. So move it to required
> > > > ---
> > > >  .../bindings/phy/fsl,imx8qxp-mipi-cphy.yaml        | 57 ++++++++++++++++++++++
> > > >  1 file changed, 57 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/phy/fsl,imx8qxp-mipi-cphy.yaml b/Documentation/devicetree/bindings/phy/fsl,imx8qxp-mipi-cphy.yaml
> > > > new file mode 100644
> > > > index 0000000000000..7335b9262d0e7
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/fsl,imx8qxp-mipi-cphy.yaml
> > > > @@ -0,0 +1,57 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/fsl,imx8qxp-mipi-cphy.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Freescale i.MX8 SoC MIPI CSI PHY
> > > > +
> > > > +maintainers:
> > > > +  - Frank Li <Frank.Li@nxp.com>
> > > > +
> > > > +properties:
> > > > +  "#phy-cells":
> > > > +    const: 0
> > > > +
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - enum:
> > > > +          - fsl,imx8qxp-mipi-cphy
> > > > +          - fsl,imx8ulp-mipi-cphy
> > > > +      - items:
> > > > +          - const: fsl,imx8qm-mipi-cphy
> > > > +          - const: fsl,imx8qxp-mipi-cphy
> > >
> > > Why are those called cphy when, as far as I can tell from the
> > > documentation, they are D-PHYs ? Does that stand for *C*SI PHY ?
> >
> > There are already have D-PHYS for MIPI display phy binding. cphy just means
> > for camera PHY.
>
> Ah OK. I would probably have gone for *-mipi-dphy-rx then, but I'm OK
> with the proposed "cphy". Explaining this in the description would be
> useful.
>
> > > I find
> > > it slightly confusing, but not so much that I'd ask for a change. It's
> > > just a name at the end of the day.
> > >
> > > Apart from that the binding looks fairly OK. Except maybe from the fact
> > > that this device is not a PHY :-( It has two PHY control registers, but
> > > the rest seems related to the glue logic at the output of the CSI-2
> > > receiver. I wonder if we should go the syscon route.
> >
> > Do you means use phandle to syscon node in csi-2 driver? Actually this
> > ways is not perferred by device tree team because it should be exported
> > as what actual function, such as PHY or RESET by use standard interface.
> >
> > We met similar case at other substream.
>
> I don't like syscon much either, but in this specific case I'm not sure
> what else we could do. This device really aggregates some control over
> the PHY and over the glue logic at the output of the CSI-2 controller.
> Modelling it as "just a PHY" will cause problem as soon as you'll want
> to configure the other parameters.

I think your comments is quite good and it's one hack. I check register
layout of csr at imx8qm/imx8qxp. It is dedicated for each csi2 moudule.
This glue layer should be second 'reg' resource, which direct control in
csi2's driver.

v3 should be simpler than this version.
https://lore.kernel.org/imx/20250210-8qxp_camera-v3-0-324f5105accc@nxp.com/T/#t

Frank

>
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  power-domains:
> > > > +    maxItems: 1
> > > > +
> > > > +required:
> > > > +  - "#phy-cells"
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +allOf:
> > > > +  - if:
> > > > +      properties:
> > > > +        compatible:
> > > > +          contains:
> > > > +            enum:
> > > > +              - fsl,imx8qxp-mipi-cphy
> > > > +    then:
> > > > +      required:
> > > > +        - power-domains
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    phy@58221000 {
> > > > +            compatible = "fsl,imx8qxp-mipi-cphy";
> > > > +            reg = <0x58221000 0x10000>;
> > > > +            #phy-cells = <0>;
> > > > +            power-domains = <&pd 0>;
> > > > +    };
> > > > +
>
> --
> Regards,
>
> Laurent Pinchart