Re: [PATCH v2 2/7] dt-bindings: mfd: mediatek: mt6397: Convert to DT schema format

[Rob Herring <robh@kernel.org>]

On Fri, Aug 30, 2024 at 07:07:27PM +0800, Macpaul Lin wrote:
> Convert the mfd: mediatek: mt6397 binding to DT schema format.
> 
> MT6323/MT6358/MT6397 are PMIC devices with multiple function of
> subdevices. They have some variant of the combinations of subdevices
> but share a common PMIC design.
> 
> New updates in this conversion:
>  - RTC:
>   - Convert rtc-mt6397.txt and add it into parent's mt6397 PMIC DT schema.
>  - regulators:
>   - Align generic names "regulators" instead of origin names.
>   - mt6323-regulator: Replace "txt" reference with mt6323-regulaotr.yaml
>   - mt6358-regulator: Replace "txt" reference with mt6358-regulator.yaml
>   - mt6397-regulator: Replace "txt" reference with mt6397-reuglator.yaml
>  - audio-codec:
>   - Align generic name "audio-codec" for codec and sound subdevices.
>   - Add "mediatek,dmic-mode" and "Avdd-supply".
>  - clocks:
>   - Align generic name "clocks" for clockbuffer subdevices.
>  - leds:
>   - Convert leds-mt6323.txt and add it into parent's mt6397 PMIC DT schema.
>  - keys:
>   - Add more specific descriptions for power and home keys.
>   - Add compatible: mediatek,mt6358-keys
>  - power-controller:
>   - Add property #power-domain-cells for fixing dt-binding check error.
>   - Add "Baseband power up" as the explaination of abbrevitation "BBPU".
>  - pinctrl:
>   - Align generic name "pinctrl" instead of "pin-controller".
> 
> Signed-off-by: Sen Chu <sen.chu@mediatek.com>
> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
> ---
>  .../bindings/mfd/mediatek,mt6397.yaml         | 1026 +++++++++++++++++
>  .../devicetree/bindings/mfd/mt6397.txt        |  110 --
>  2 files changed, 1026 insertions(+), 110 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml
>  delete mode 100644 Documentation/devicetree/bindings/mfd/mt6397.txt
> 
> Changes for v1:
>  - This patch depends on conversion of mediatek,mt6397-regulator.yaml
>    [1] https://lore.kernel.org/lkml/20240807091738.18387-1-macpaul.lin@mediatek.com/T/
> 
> Changes for v2:
>  - This patch has been made base on linux-next/master git repo.
>  - Keep the parent and child relationship with mediatek,pwrap in description.
>    [2] https://lore.kernel.org/all/20240826-slurp-earphone-0d5173923ae8@spud/
>  - Keep the $ref for regulators since dt_binding_check didn't report any issue
>    based on linux-next/master repo.  
>  - Fix description of mt6397/mt6323 devices, use "power management chip"
>    instead of "multifunction device"
>  - Drop unnecessary comments or description according to the review.
>  - Convert sub-modules to DT Schema:
>   - RTC, LEDs, power-controllers, regulators
>  - Drop duplicate sub node name and description for sub-modules
>   - RTC, Keys
>  - examples: 
>   - drop parent pwrap node
>   - Add examples from mediatek,mt6323-regulator.yaml
>   - Add examples from mediatek,mt6358-regulator.yaml
>   - Add examples from mediatek,mt6397-regulator.yaml
>   - Complete the examples as could as possible.
> 
> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml
> new file mode 100644
> index 0000000..f5bea33
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6397.yaml
> @@ -0,0 +1,1026 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/mediatek,mt6397.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT6397/MT6323 Multifunction Device (PMIC)
> +
> +maintainers:
> +  - Sen Chu <sen.chu@mediatek.com>
> +  - Macpaul Lin <macpaul.lin@mediatek.com>
> +
> +description: |
> +  MT6397/MT6323 is a power management system chip.
> +  Please see the sub-modules below for supported features.
> +
> +  MT6397/MT6323 is a multifunction device with the following sub modules:
> +  - Regulators
> +  - RTC
> +  - Audio codec
> +  - GPIO
> +  - Clock
> +  - LED
> +  - Keys
> +  - Power controller
> +
> +  It is interfaced to host controller using SPI interface by a proprietary hardware
> +  called PMIC wrapper or pwrap. MT6397/MT6323 PMIC is a child device of pwrap.
> +  See the following for pwrap node definitions:
> +  Documentation/devicetree/bindings/soc/mediatek/mediatek,pwrap.yaml
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - mediatek,mt6323
> +          - mediatek,mt6331 # "mediatek,mt6331" for PMIC MT6331 and MT6332.
> +          - mediatek,mt6357
> +          - mediatek,mt6358
> +          - mediatek,mt6359
> +          - mediatek,mt6397
> +      - items:
> +          - enum:
> +              - mediatek,mt6366
> +          - const: mediatek,mt6358
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  rtc:
> +    type: object
> +    $ref: /schemas/rtc/rtc.yaml#
> +    unevaluatedProperties: false
> +    description:
> +      MT6397 Real Time Clock.

Blank line

> +    properties:
> +      compatible:
> +        oneOf:
> +          - enum:
> +              - mediatek,mt6323-rtc
> +              - mediatek,mt6331-rtc
> +              - mediatek,mt6358-rtc
> +              - mediatek,mt6397-rtc
> +          - items:
> +              - enum:
> +                  - mediatek,mt6366-rtc
> +              - const: mediatek,mt6358-rtc

Blank line between DT properties

> +      start-year: true
> +    required:
> +      - compatible
> +
> +  regulators:
> +    type: object
> +    oneOf:
> +      - $ref: /schemas/regulator/mediatek,mt6323-regulator.yaml
> +      - $ref: /schemas/regulator/mediatek,mt6358-regulator.yaml
> +      - $ref: /schemas/regulator/mediatek,mt6397-regulator.yaml
> +    unevaluatedProperties: false
> +    description:
> +      List of child nodes that specify the regulators.
> +    properties:
> +      compatible:
> +        oneOf:
> +          - enum:
> +              - mediatek,mt6323-regulator
> +              - mediatek,mt6358-regulator
> +              - mediatek,mt6397-regulator
> +          - items:
> +              - enum:
> +                  - mediatek,mt6366-regulator
> +              - const: mediatek,mt6358-regulator

You need the references or compatible, but not both. It's more efficient 
if you list the compatibles along with a 'additionalProperties: true'. 
Otherwise, the referenced schemas have to all be applied and the 
matching one will be applied twice.

Also, for compatible here, just use 'contains' and list all possible 
compatibles. The exact combinations are enforced in the regulator 
schemas.


> +
> +  audio-codec:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      Audio codec support with MT6397 and MT6358.
> +    properties:
> +      compatible:
> +        oneOf:
> +          - enum:
> +              - mediatek,mt6397-codec
> +              - mediatek,mt6358-sound
> +          - items:
> +              - enum:
> +                  - mediatek,mt6366-sound
> +              - const: mediatek,mt6358-sound
> +
> +      mediatek,dmic-mode:
> +        description: |
> +          Indicates how many data pins are used to transmit two channels of PDM
> +          signal.
> +          0 - two wires;
> +          1 - one wire;
> +          Default value is 0.
> +        enum: [0, 1]
> +        default: 0
> +
> +      Avdd-supply:
> +        description: Power source of AVDD.
> +
> +    required:
> +      - compatible
> +
> +  clocks:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      This is a clock buffer node for mt6397. However, there are no sub nodes
> +      or any public document exposed in public.
> +    properties:
> +      compatible:
> +        const: mediatek,mt6397-clk
> +      '#clock-cells':
> +        const: 1
> +    required:
> +      - compatible
> +
> +  leds:
> +    type: object
> +    additionalProperties: false
> +    description:

You need '|' or '>' to preserve line breaks.

> +      MT6323 LED controller is subfunction provided by MT6323 PMIC, so the LED
> +      controllers are defined as the subnode of the function node provided by MT6323
> +      PMIC controller that is being defined as one kind of Muti-Function Device (MFD)
> +      using shared bus called PMIC wrapper for each subfunction to access remote
> +      MT6323 PMIC hardware.
> +
> +      Each led is represented as a child node of the mediatek,mt6323-led that
> +      describes the initial behavior for each LED physically and currently only four
> +      LED child nodes can be supported.
> +
> +    properties:
> +      compatible:
> +        oneOf:

Only 1 entry, don't need oneOf.

> +          - enum:
> +              - mediatek,mt6323-led
> +              - mediatek,mt6331-led
> +              - mediatek,mt6332-led
> +      "#address-cells":
> +        const: 1

blank line

> +      "#size-cells":
> +        const: 0

blank line. And so on...

> +      reg:
> +        description:
> +          LED channel number (0..3)
> +        minimum: 0
> +        maximum: 3

Doesn't use the led binding?

> +
> +  keys:
> +    type: object
> +    $ref: /schemas/input/mediatek,pmic-keys.yaml
> +    unevaluatedProperties: false
> +    description:
> +      Power and Home keys.
> +    properties:
> +      compatible:
> +        oneOf:
> +          - enum:
> +              - mediatek,mt6323-keys
> +              - mediatek,mt6331-keys
> +              - mediatek,mt6358-keys
> +              - mediatek,mt6397-keys
> +
> +  power-controller:
> +    type: object
> +    additionalProperties: false
> +    description:
> +      The power controller which could be found on PMIC is responsible for
> +      externally powering off or on the remote MediaTek SoC through the
> +      circuit BBPU (baseband power up).
> +    properties:
> +      compatible:
> +        const: mediatek,mt6323-pwrc
> +      '#power-domain-cells':
> +        const: 0
> +
> +  pinctrl:
> +    type: object
> +    $ref: /schemas/pinctrl/mediatek,mt65xx-pinctrl.yaml
> +    unevaluatedProperties: false
> +    description:
> +      Pin controller
> +    properties:
> +      compatible:
> +        const: mediatek,mt6397-pinctrl
> +
> +required:
> +  - compatible
> +  - regulators
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    mt6323_pmic: pmic {

Drop unused labels.

> +        compatible = "mediatek,mt6323";
> +        interrupt-parent = <&pio>;
> +        interrupts = <150 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +
> +        mt6323_leds: leds {
> +                compatible = "mediatek,mt6323-led";
> +                #address-cells = <1>;
> +                status = "disabled";

Examples shouldn't be disabled.

> +        };
> +
> +        mt6323_regulator: regulators {
> +            compatible = "mediatek,mt6323-regulator";
> +            mt6323_vproc_reg: buck_vproc {
> +                regulator-name = "vproc";
> +                regulator-min-microvolt = < 700000>;
> +                regulator-max-microvolt = <1350000>;
> +                regulator-ramp-delay = <12500>;
> +                regulator-always-on;
> +                regulator-boot-on;
> +            };