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

[Macpaul Lin <macpaul.lin@mediatek.com>]

On 8/30/24 23:33, Rob Herring wrote:
> 	
> 
> External email : Please do not click links or open attachments until you 
> have verified the sender or the content.
> 
> 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

[snip]

>> +  rtc:
>> +    type: object
>> +    $ref: /schemas/rtc/rtc.yaml#
>> +    unevaluatedProperties: false
>> +    description:
>> +      MT6397 Real Time Clock.
> 
> Blank line

Will fix this in v3 version.

> 
>> +    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

Will fix this in v3 version.

>> +      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.

Both 'addtionalProperties: true' and 'contains' will be added to
v3 version. Since there are different regulator nodes in these
DT Schema but seems no other common nodes, $ref will be added
to if..then.. match rulesfor each compatible.


[snip]

>> +
>> +  leds:
>> +    type: object
>> +    additionalProperties: false
>> +    description:
> 
> You need '|' or '>' to preserve line breaks.

Will be fixed in v3 version.


>> +
>> +    properties:
>> +      compatible:
>> +        oneOf:
> 
> Only 1 entry, don't need oneOf.
> 

Will be fixed in v3 version.

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

Will be fixed in v3 version.

>> +      "#size-cells":
>> +        const: 0
> 
> blank line. And so on...

Will be fixed in v3 version.

>> +      reg:
>> +        description:
>> +          LED channel number (0..3)
>> +        minimum: 0
>> +        maximum: 3
> 
> Doesn't use the led binding?

Will be fixed in v3 version: using led bindings.

[snip]

>> +

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    mt6323_pmic: pmic {
> 
> Drop unused labels.

Will be fixed in v3 version.

>> +        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.

Will be fixed in v3 version.

[snip]

Thanks for the review.

Regards,
Macpaul Lin