RE: [PATCH v2 1/2] dt-bindings: phy: samsung,usb3-drd-phy: add dt-schema for ExynosAutov920

["Pritam Manohar Sutar" <pritam.sutar@samsung.com>]

Hi Krzysztof, 

Thank you for the reviewing the patches.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 20 May 2025 01:15 PM
> To: Pritam Manohar Sutar <pritam.sutar@samsung.com>; vkoul@kernel.org;
> kishon@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; alim.akhtar@samsung.com; andre.draszik@linaro.org;
> peter.griffin@linaro.org; kauschluss@disroot.org;
> m.szyprowski@samsung.com; s.nawrocki@samsung.com
> Cc: linux-phy@lists.infradead.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org; rosa.pila@samsung.com; dev.tailor@samsung.com;
> faraz.ata@samsung.com; muhammed.ali@samsung.com;
> selvarasu.g@samsung.com
> Subject: Re: [PATCH v2 1/2] dt-bindings: phy: samsung,usb3-drd-phy: add dt-
> schema for ExynosAutov920
> 
> On 16/05/2025 12:26, Pritam Manohar Sutar wrote:
> > Add a dedicated compatible for USB phy found in this SoC
> >
> > Signed-off-by: Pritam Manohar Sutar <pritam.sutar@samsung.com>
> > ---
> >  .../bindings/phy/samsung,usb3-drd-phy.yaml    | 27 +++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> 
> A nit, subject: drop second/last, redundant "dt-schema for". The "dt-bindings"
> prefix is already stating that these are bindings in dtschema format.
> See also:
> https://protect2.fireeye.com/v1/url?k=e78ee20f-b815db03-e78f6940-
> 000babff3563-8256a76d4c2ebd67&q=1&e=1643274a-07fa-4563-af27-
> b2f2eff30417&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.7-
> rc8%2Fsource%2FDocumentation%2Fdevicetree%2Fbindings%2Fsubmitting-
> patches.rst%23L18
> 

Will update the commit title as below
"dt-bindings: phy: samsung,usb3-drd-phy: add ExynosAutov920 compatible"

> >
> > diff --git
> > a/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml
> > b/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml
> > index fdddddc7d611..c50f4218ded9 100644
> > --- a/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/samsung,usb3-drd-phy.yaml
> > @@ -32,6 +32,7 @@ properties:
> >        - samsung,exynos7-usbdrd-phy
> >        - samsung,exynos7870-usbdrd-phy
> >        - samsung,exynos850-usbdrd-phy
> > +      - samsung,exynosautov920-usb31drd-phy
> >
> >    clocks:
> >      minItems: 2
> > @@ -204,6 +205,32 @@ allOf:
> >          reg-names:
> >            maxItems: 1
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: samsung,exynosautov920-usb31drd-phy
> > +    then:
> > +      $ref: /schemas/usb/usb-switch.yaml#
> > +
> > +      properties:
> > +        clocks:
> > +          items:
> 
> Why there is no main PHY clock?

external crystal clk (ext_xtal) is used as main phy clk.

> 
> > +            - description: ext_xtal clock
> > +            - description: reference clock
> 
> Both external oscillator and reference clocks? What are these clocks?

ext_xtal is used as PHY clock to access register and reference clock 
for PHY operations.

Will add more description in patch.

> 
> > +
> > +        clock-names:
> > +          items:
> > +            - const: ext_xtal
> > +            - const: ref
> > +
> > +        reg:
> > +          minItems: 1
> 
> No, there is no such syntax. Drop.

Will remove this

> 
> > +          maxItems: 1
> > +
> > +        reg-names:
> > +          minItems: 1
> 
> No, look at existing code and do the same.

Will replace "minItems"  by "maxItems"

> 
> > +
> >  unevaluatedProperties: false
> >
> >  examples:
> 
> 
> Best regards,
> Krzysztof

Thank you.