Message-ID: <Z6tr414dKK56atDw@lizhi-Precision-Tower-5810>
Date: Tue, 11 Feb 2025 10:25:23 -0500
From: Frank Li <Frank.li@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Vinod Koul <vkoul@...nel.org>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rui Miguel Silva <rmfrfs@...il.com>,
	Martin Kepplinger <martink@...teo.de>,
	Purism Kernel Team <kernel@...i.sm>, linux-phy@...ts.infradead.org,
	devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-media@...r.kernel.org, "Guoniu.zhou" <guoniu.zhou@....com>,
	Robby Cai <robby.cai@....com>,
	Robert Chiras <robert.chiras@....com>
Subject: Re: [PATCH v2 01/14] dt-bindings: phy: Add MIPI CSI PHY for i.MX8Q

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@....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@....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@...21000 {
> > > > +            compatible = "fsl,imx8qxp-mipi-cphy";
> > > > +            reg = <0x58221000 0x10000>;
> > > > +            #phy-cells = <0>;
> > > > +            power-domains = <&pd 0>;
> > > > +    };
> > > > +
>
> --
> Regards,
>
> Laurent Pinchart

Powered by blists - more mailing lists