| Message ID | 20220619113325.21396-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dt-bindings: phy: make phy-cells description a text | expand |
Hi Krzysztof, Thank you for the patch. On Sun, Jun 19, 2022 at 01:33:25PM +0200, Krzysztof Kozlowski wrote: > The description field is a string, so using YAML inside phy-cells > description is not actually helpful. Does it hurt though ? For xlnx,zynqmp-psgtr.yaml I wrote it that way to prepare for a future where it could be described using a YAML schema (but such future may never come). > Make it a proper text. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > .../bindings/phy/mediatek,tphy.yaml | 14 ++++---- > .../bindings/phy/mediatek,xsphy.yaml | 10 +++--- > .../bindings/phy/xlnx,zynqmp-psgtr.yaml | 32 ++++++++----------- > 3 files changed, 23 insertions(+), 33 deletions(-) > > diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > index 4b638c1d4221..bd0e4c4915ed 100644 > --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml > @@ -154,14 +154,12 @@ patternProperties: > "#phy-cells": > const: 1 > description: | > - The cells contain the following arguments. > - > - - description: The PHY type > - enum: > - - PHY_TYPE_USB2 > - - PHY_TYPE_USB3 > - - PHY_TYPE_PCIE > - - PHY_TYPE_SATA > + The cells contain the following arguments:: > + - The PHY type:: > + - PHY_TYPE_USB2 > + - PHY_TYPE_USB3 > + - PHY_TYPE_PCIE > + - PHY_TYPE_SATA > > nvmem-cells: > items: > diff --git a/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml > index 598fd2b95c29..7262b8e184e2 100644 > --- a/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml > +++ b/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml > @@ -100,12 +100,10 @@ patternProperties: > "#phy-cells": > const: 1 > description: | > - The cells contain the following arguments. > - > - - description: The PHY type > - enum: > - - PHY_TYPE_USB2 > - - PHY_TYPE_USB3 > + The cells contain the following arguments:: > + - The PHY type:: > + - PHY_TYPE_USB2 > + - PHY_TYPE_USB3 > > # The following optional vendor properties are only for debug or HQA test > mediatek,eye-src: > diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml > index 79906519c652..7083eddb467c 100644 > --- a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml > +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml > @@ -18,25 +18,19 @@ properties: > "#phy-cells": > const: 4 > description: | > - The cells contain the following arguments. > - > - - description: The GTR lane > - minimum: 0 > - maximum: 3 > - - description: The PHY type > - enum: > - - PHY_TYPE_DP > - - PHY_TYPE_PCIE > - - PHY_TYPE_SATA > - - PHY_TYPE_SGMII > - - PHY_TYPE_USB3 > - - description: The PHY instance > - minimum: 0 > - maximum: 1 # for DP, SATA or USB > - maximum: 3 # for PCIE or SGMII > - - description: The reference clock number > - minimum: 0 > - maximum: 3 > + The cells contain the following arguments:: > + - The GTR lane (minimum:: 0, maximum:: 3) > + - The PHY type:: > + - PHY_TYPE_DP > + - PHY_TYPE_PCIE > + - PHY_TYPE_SATA > + - PHY_TYPE_SGMII > + - PHY_TYPE_USB3 > + - The PHY instance:: > + minimum:: 0 > + maximum:: 1 # for DP, SATA or USB > + maximum:: 3 # for PCIE or SGMII > + - The reference clock number (minimum:: 0, maximum:: 3) > > compatible: > enum: > -- > 2.34.1 >
On 19/06/2022 13:40, Laurent Pinchart wrote: > Hi Krzysztof, > > Thank you for the patch. > > On Sun, Jun 19, 2022 at 01:33:25PM +0200, Krzysztof Kozlowski wrote: >> The description field is a string, so using YAML inside phy-cells >> description is not actually helpful. > > Does it hurt though ? For xlnx,zynqmp-psgtr.yaml I wrote it that way to > prepare for a future where it could be described using a YAML schema > (but such future may never come). No, it does not hurt. It is however confusing some folks and they think schema goes into description. The description should be readable/descriptive for humans, so if you think your approach is better, I am perfectly fine with it. Best regards, Krzysztof
On Sun, Jun 19, 2022 at 01:46:17PM +0200, Krzysztof Kozlowski wrote: > On 19/06/2022 13:40, Laurent Pinchart wrote: > > Hi Krzysztof, > > > > Thank you for the patch. > > > > On Sun, Jun 19, 2022 at 01:33:25PM +0200, Krzysztof Kozlowski wrote: > >> The description field is a string, so using YAML inside phy-cells > >> description is not actually helpful. > > > > Does it hurt though ? For xlnx,zynqmp-psgtr.yaml I wrote it that way to > > prepare for a future where it could be described using a YAML schema > > (but such future may never come). > > No, it does not hurt. It is however confusing some folks and they think > schema goes into description. The description should be > readable/descriptive for humans, so if you think your approach is > better, I am perfectly fine with it. I don't mind much. If you think it would be a good idea to eventually describe the #phy-cells elements in YAML schema then I'd rather keep the current description, if there's very little chance it will happen, I don't mind changing it.
On Sun, Jun 19, 2022 at 02:40:12PM +0300, Laurent Pinchart wrote: > Hi Krzysztof, > > Thank you for the patch. > > On Sun, Jun 19, 2022 at 01:33:25PM +0200, Krzysztof Kozlowski wrote: > > The description field is a string, so using YAML inside phy-cells > > description is not actually helpful. > > Does it hurt though ? Unfortunately, I see this a bit. It's convenient because the schema passes all the checks. Doh! And I usually stare at it wondering how it passed. Though I probably did review this, so IDK... > For xlnx,zynqmp-psgtr.yaml I wrote it that way to > prepare for a future where it could be described using a YAML schema > (but such future may never come). There's 2 parts. There's the resolving the defines and then applying the schema to the cells. I actually think the latter would be easier. At least from a documenting standpoint, we just need to define our own keyword to stick the schema under. With the tools doing dtb based validation now, the tools already get the phandle node and get the cell size from the DT. It's just another step to extract the node's compatible, find it's schema, and get its cell format schema. Any volunteers? Rob
diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml index 4b638c1d4221..bd0e4c4915ed 100644 --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml @@ -154,14 +154,12 @@ patternProperties: "#phy-cells": const: 1 description: | - The cells contain the following arguments. - - - description: The PHY type - enum: - - PHY_TYPE_USB2 - - PHY_TYPE_USB3 - - PHY_TYPE_PCIE - - PHY_TYPE_SATA + The cells contain the following arguments:: + - The PHY type:: + - PHY_TYPE_USB2 + - PHY_TYPE_USB3 + - PHY_TYPE_PCIE + - PHY_TYPE_SATA nvmem-cells: items: diff --git a/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml index 598fd2b95c29..7262b8e184e2 100644 --- a/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml +++ b/Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml @@ -100,12 +100,10 @@ patternProperties: "#phy-cells": const: 1 description: | - The cells contain the following arguments. - - - description: The PHY type - enum: - - PHY_TYPE_USB2 - - PHY_TYPE_USB3 + The cells contain the following arguments:: + - The PHY type:: + - PHY_TYPE_USB2 + - PHY_TYPE_USB3 # The following optional vendor properties are only for debug or HQA test mediatek,eye-src: diff --git a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml index 79906519c652..7083eddb467c 100644 --- a/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml +++ b/Documentation/devicetree/bindings/phy/xlnx,zynqmp-psgtr.yaml @@ -18,25 +18,19 @@ properties: "#phy-cells": const: 4 description: | - The cells contain the following arguments. - - - description: The GTR lane - minimum: 0 - maximum: 3 - - description: The PHY type - enum: - - PHY_TYPE_DP - - PHY_TYPE_PCIE - - PHY_TYPE_SATA - - PHY_TYPE_SGMII - - PHY_TYPE_USB3 - - description: The PHY instance - minimum: 0 - maximum: 1 # for DP, SATA or USB - maximum: 3 # for PCIE or SGMII - - description: The reference clock number - minimum: 0 - maximum: 3 + The cells contain the following arguments:: + - The GTR lane (minimum:: 0, maximum:: 3) + - The PHY type:: + - PHY_TYPE_DP + - PHY_TYPE_PCIE + - PHY_TYPE_SATA + - PHY_TYPE_SGMII + - PHY_TYPE_USB3 + - The PHY instance:: + minimum:: 0 + maximum:: 1 # for DP, SATA or USB + maximum:: 3 # for PCIE or SGMII + - The reference clock number (minimum:: 0, maximum:: 3) compatible: enum:
The description field is a string, so using YAML inside phy-cells description is not actually helpful. Make it a proper text. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- .../bindings/phy/mediatek,tphy.yaml | 14 ++++---- .../bindings/phy/mediatek,xsphy.yaml | 10 +++--- .../bindings/phy/xlnx,zynqmp-psgtr.yaml | 32 ++++++++----------- 3 files changed, 23 insertions(+), 33 deletions(-)