Re: [PATCHv2] dt-bindings: mfd: omap-usb-tll: convert to YAML

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/12/2024 16:31, Karol Przybylski wrote:
> Conversion of omap-usb-tll.txt into yaml format, inspired by discussion in [1]
> 

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

Also, your commit msg should explain changes made to the binding during
conversion.

> Changes after v1:
> Added Roger Quadros as maintainer
> Removed deprecated property 'ti,hwmod'
> Renamed .yaml file

Changelog goes after ---.

> 
> 1 - https://lore.kernel.org/all/cd915c18-7230-4c38-a860-d2a777223147@kernel.org/
> 
> Signed-off-by: Karol Przybylski <karprzy7@gmail.com>
> ---

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

This wasn't tested, so just brief review:

>  .../devicetree/bindings/mfd/omap-usb-tll.txt  | 27 -----------
>  .../devicetree/bindings/mfd/ti,usbhs-tll.yaml | 46 +++++++++++++++++++
>  2 files changed, 46 insertions(+), 27 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-tll.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/ti,usbhs-tll.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-tll.txt b/Documentation/devicetree/bindings/mfd/omap-usb-tll.txt
> deleted file mode 100644
> index c58d70437..000000000
> --- a/Documentation/devicetree/bindings/mfd/omap-usb-tll.txt
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -OMAP HS USB Host TLL (Transceiver-Less Interface)
> -
> -Required properties:
> -
> -- compatible : should be "ti,usbhs-tll"
> -- reg : should contain one register range i.e. start and length
> -- interrupts : should contain the TLL module's interrupt
> -- ti,hwmod : must contain "usb_tll_hs"
> -
> -Optional properties:
> -
> -- clocks: a list of phandles and clock-specifier pairs, one for each entry in
> -  clock-names.
> -
> -- clock-names: should include:
> -  * "usb_tll_hs_usb_ch0_clk" - USB TLL channel 0 clock
> -  * "usb_tll_hs_usb_ch1_clk" - USB TLL channel 1 clock
> -  * "usb_tll_hs_usb_ch2_clk" - USB TLL channel 2 clock
> -
> -Example:
> -
> -	usbhstll: usbhstll@4a062000 {
> -		compatible = "ti,usbhs-tll";
> -		reg = <0x4a062000 0x1000>;
> -		interrupts = <78>;
> -		ti,hwmods = "usb_tll_hs";
> -	  };
> diff --git a/Documentation/devicetree/bindings/mfd/ti,usbhs-tll.yaml b/Documentation/devicetree/bindings/mfd/ti,usbhs-tll.yaml
> new file mode 100644
> index 000000000..d666d3e23
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ti,usbhs-tll.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/ti,usbhs-tll.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OMAP HS USB Host TLL (Transceiver-Less Interface)
> +
> +maintainers:
> +  - <rogerq@kernel.org>

Use full name from the maintainers.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,usbhs-tll
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

missing maxItems.

minItems incorrect - below you have two.

> +
> +  clock-names:
> +    items:
> +      - const: usb_tll_hs_usb_ch0_clk
> +      - const: usb_tll_hs_usb_ch1_clk
> +      - const: usb_tll_hs_usb_ch2_clk
> +    minItems: 2

Nut anyway, why is this flexible?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    usbhstll@4a062000 {
> +      compatible = "ti,usbhs-tll";
> +      reg = <0x4a062000 0x1000>;
> +      interrupts = <78>;

Make example complete.

> +    };


Best regards,
Krzysztof