On Sat, Jun 17, 2023 at 10:48:41AM +0200, Krzysztof Kozlowski wrote:

On 15/06/2023 07:27, Varadarajan Narayanan wrote:

quoted

quoted

quoted

+          - enum:
+              - qcom,m31-usb-hsphy

I am confused what's this. If m31 is coming from some IP block provider,
then you are using wrong vendor prefix.
https://www.m31tech.com/download_file/M31_USB.pdf

quoted

+              - qcom,ipq5332-m31-usb-hsphy

This confuses me even more. IPQ m31?

Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
Will that be acceptable?

m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
device have some name/version/model? If it is not really known, then I
would just propose to go with qcom,ipq5332-usb-hsphy.

Skip generic compatible ("usb-hsphy") entirely.

Ok.

And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
something similar with difference in the hyphen. Just use device
specific compatible thus device specific filename.

qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
driver we are introducing is for UTMI. We would have to
modify phy-qcom-usb-hs.c to accomodate M31. Will that be
acceptable to phy-qcom-usb-hs.c owners/maintainers?

quoted

quoted

quoted

+
+  reg:
+    description:
+      Offset and length of the M31 PHY register set

Drop description, obvious.

Ok.

quoted

quoted

+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: m31usb_phy_base
+      - const: qscratch_base

Drop "_base" from both.

Ok. Will drop qscratch_base. This is in the controller space.
Should not come here.

Then drop reg-names entirely.

Ok.

quoted

quoted

quoted

+
+  phy_type:
+    oneOf:
+      - items:
+          - enum:
+              - utmi
+              - ulpi

This does not belong to phy, but to USB node.

This is used by the driver to set a bit during phy init. Hence
have it as a replication of the USB node's entry. If this is not
permissible, is there some way to get this from the USB node,
or any other alternative mechanism?

Shouldn't USB controller choose what type of PHY type it wants?

Will remove this. IPQ5332 uses it in UTMI mode only.

quoted

quoted

quoted

+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+    hs_m31phy_0: hs_m31phy@5b00 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, no underscores in node names.

Will change this as usbphy0:hs_m31phy@7b000

This does not solve my comments. I did not write "label" but "node name".

Sorry. will fix it.

Thanks
Varada

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy