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

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

On 8/8/24 20:04, Krzysztof Kozlowski wrote:
> 	
> 
> External email : Please do not click links or open attachments until you 
> have verified the sender or the content.
> 
> On 08/08/2024 12:57, Macpaul Lin wrote:
>> Convert the mfd: mediatek: mt6397 binding to DT schema format.
>> 
>> New updates in this conversion:
>>  - Align generic names of DT schema "audio-codec" and "regulators".
>>  - mt6397-regulators: Replace the "txt" reference with newly added DT
>>    schema.
>> 
>> Signed-off-by: Sen Chu <sen.chu@mediatek.com>
>> Signed-off-by: Macpaul Lin <macpaul.lin@mediatek.com>
>> ---
>>  .../bindings/mfd/mediatek,mt6397.yaml         | 202 ++++++++++++++++++
>>  .../devicetree/bindings/mfd/mt6397.txt        | 110 ----------
> 
> You are doing conversions in odd order... and ignore my comments. The
> example from your regulator binding is supposed to be here - I wrote it
> last time.
> 
> Due to doing changes totally unsynchronized, this CANNOT be merged
> without unnecessary maintainer coordination, because of dependency.
> 
> Sorry, that's not how it works for MFD devices.
> 
> Perform conversion of entire device in ONE patchset.

Okay, will collect the conversion of mt6323-regulator.txt and 
rtc-mt6397.txt in the next version.

>>  2 files changed, 202 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/

[snip]

>> +$id: http://devicetree.org/schemas/mfd/mediatek,mt6397.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek MT6397/MT6323 Multifunction Device
>> +
>> +maintainers:
>> +  - Sen Chu <sen.chu@mediatek.com>
>> +  - Macpaul Lin <macpaul.lin@mediatek.com>
>> +
>> +description: |
>> +  MT6397/MT6323 is a multifunction device with the following sub modules:
> 
> MFD is Linuxism, avoid it.

Will replace MFD with "power management system chip with sub-modules" 
something like this in next version.

>> +  - Regulator
>> +  - 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 MFD is a child device of pwrap.
>> +  See the following for pwarp node definitions:
>> +  ../soc/mediatek/mediatek,pwrap.yaml
> 
> Drop, instead add proper ref or compatible in parent node.

I'm confused here. I've checked mediatek,mt6357.yaml as a reference
.
It uses the similar method here.
     "See the following for pwarp node definitions:"
     "Documentation/devicetree/bindings/soc/mediatek/mediatek,pwrap.yaml"

If "$ref: /schemas/soc/mediatek/mediatek,pwrap.yaml" is added here,
dt_bindings_check will complain the following errors and more.

Documentation/devicetree/bindings/mfd/mediatek,mt6397.example.dtb: pmic: 
compatible: 'oneOf' conditional failed, one must be fixed:
         ['mediatek,mt6397'] is too short
         'mediatek,mt6397' is not one of ['mediatek,mt2701-pwrap', 
'mediatek,mt6765-pwrap', 'mediatek,mt6779-pwrap', 
'mediatek,mt6795-pwrap', 'mediatek,mt6797-pwrap', 
'mediatek,mt6873-pwrap', 'mediatek,mt7622-pwrap', 
'mediatek,mt8135-pwrap', 'mediatek,mt8173-pwrap', 
'mediatek,mt8183-pwrap', 'mediatek,mt8186-pwrap', 
'mediatek,mt8195-pwrap', 'mediatek,mt8365-pwrap', 'mediatek,mt8516-pwrap']
         'mediatek,mt6397' is not one of ['mediatek,mt8186-pwrap', 
'mediatek,mt8195-pwrap']
         'mediatek,mt6397' is not one of ['mediatek,mt8188-pwrap']
         from schema $id: 
http://devicetree.org/schemas/mfd/mediatek,mt6397.yaml#

Which also conflicts with the comments in the examples..
 >> +    pwrap {
 >
 > Drop

Please help to check if a $ref or a compatible of pwrap should be added 
here.

>> +
>> +  This document describes the binding for MFD device and its sub module.
> 
> Drop

Will fix it in next version.

>> +
>> +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 # "mediatek,mt6366", "mediatek,mt6358" for PMIC MT6366
> 
> Drop comment, it is obvious. Don't repeat constraints in free form text.

Will fix it in next version.

> 
>> +          - const: mediatek,mt6358
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  interrupt-controller: true
>> +
>> +  "#interrupt-cells":
>> +    const: 2
>> +
>> +  rtc:
>> +    type: object
>> +    unevaluatedProperties: false
>> +    description:
>> +      Real Time Clock (RTC)
>> +      See ../rtc/rtc-mt6397.txt
> 
> No, convert the binding.

Will convert it rtc-mt6397.txt and put it into
"mfd/mediatek,mt6397.yaml" together.

> 
>> +    properties:
>> +      compatible:
>> +        oneOf:
>> +          - enum:
>> +              - mediatek,mt6323-rtc
>> +              - mediatek,mt6331-rtc
>> +              - mediatek,mt6358-rtc
>> +              - mediatek,mt6397-rtc
>> +          - items:
>> +              - enum:
>> +                  - mediatek,mt6366-rtc # RTC MT6366
> 
> Drop all such comments.
> 
>> +              - const: mediatek,mt6358-rtc
>> +
>> +  regulators:
>> +    type: object
>> +    oneOf:
>> +      - $ref: /schemas/regulator/mediatek,mt6358-regulator.yaml
>> +      - $ref: /schemas/regulator/mediatek,mt6397-regulator.yaml
> 
> And how is it supposed to be tested?

The dt_bindings_check didn't complain eny thing about these.
Of course I've included the conversion patch of 
mediatek,mt6397-regulator.yaml.

>> +    unevaluatedProperties: false
>> +    description:
>> +      Regulators
>> +      For mt6323, see ../regulator/mt6323-regulator.txt
> 
> Drop, useless.
Should I convert it to DT schema and add to $ref above together?

> 
>> +    properties:
>> +      compatible:
>> +        oneOf:
>> +          - enum:
>> +              - mediatek,mt6323-regulator
>> +              - mediatek,mt6358-regulator
>> +              - mediatek,mt6397-regulator
>> +          - items:
>> +              - enum:
>> +                  - mediatek,mt6366-regulator # Regulator MT6366
>> +              - const: mediatek,mt6358-regulator
>> +
>> +  audio-codec:
>> +    type: object
>> +    unevaluatedProperties: false
> 
> This does not make sense. You do not have any ref here.

The dt_bindings_check will complain error here.
Will replace it with "additionalProperties: false".


>> +    description:
>> +      Audio codec
>> +    properties:
>> +      compatible:
>> +        oneOf:
>> +          - enum:
>> +              - mediatek,mt6397-codec
>> +              - mediatek,mt6358-sound
>> +          - items:
>> +              - enum:
>> +                  - mediatek,mt6366-sound # Codec MT6366
>> +              - const: mediatek,mt6358-sound
> 
> This wasn't in the old binding. Commit msg also does not explain why you
> are doing changes from conversion.

Will update new added item into commit message in next version.

>> +
>> +  clk:
>> +    type: object
>> +    unevaluatedProperties: false
> 
> Again, no, it does not work like this. See example schema for
> explanation of this.

Will replace it with "additionalProperties: false".

> Convert all children - entire device. Then either use ref or
> additionalProperties: true. See Qualcomm mdss bindings for example.

There is no more children available for the clock node of this PMIC.
This is a clock buffer node. However, there are no sub nodes or any
public document explain these clock buffer in public domain.
What I've got is the compatible string in the driver.

>> +    description:
>> +      Clock
> 
> Your descriptions are useless. You just said "clk" node is "clock". Really?

Will improve it in next version.

>> +    properties:
>> +      compatible:
>> +        const: mediatek,mt6397-clk
>> +
>> +  led:
>> +    type: object
>> +    unevaluatedProperties: false
>> +    description:
>> +      LED
>> +      See ../leds/leds-mt6323.txt for more information
> 
> No

Will convert "leds-mt6323.txt" and move it together with 
"mfd/mediatek,mt6397.yaml"

>> +    properties:
>> +      compatible:
>> +        const: mediatek,mt6323-led
>> +
>> +  keys:
>> +    type: object
>> +    $ref: /schemas/input/mediatek,pmic-keys.yaml
>> +    unevaluatedProperties: false
>> +    description: Keys
> 
> Keys are keys? Could keys be anything else?

Will fix it in the next version.

>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    pwrap {
> 
> Drop

Will fix it in the next version.

>> +        pmic {
>> +            compatible = "mediatek,mt6397";
>> +            interrupts-extended = <&pio 222 IRQ_TYPE_LEVEL_HIGH>;
>> +            interrupt-controller;
>> +            #interrupt-cells = <2>;
>> +
>> +            mt6397_codec: audio-codec {
>> +                compatible = "mediatek,mt6397-codec";
>> +            };
>> +
>> +            mt6397_regulators: regulators {
>> +                compatible = "mediatek,mt6397-regulator";
>> +
>> +                mt6397_vpca15_reg: buck_vpca15 {
>> +                    regulator-name = "vpca15";
>> +                    regulator-min-microvolt = <850000>;
>> +                    regulator-max-microvolt = <1400000>;
>> +                    regulator-ramp-delay = <12500>;
>> +                    regulator-always-on;
>> +                };
>> +
>> +                mt6397_vgp4_reg: ldo_vgp4 {
>> +                    regulator-name = "vgp4";
>> +                    regulator-min-microvolt = <1200000>;
>> +                    regulator-max-microvolt = <3300000>;
>> +                    regulator-enable-ramp-delay = <218>;
>> +                };
>> +            };
> 
> Incomplete.
> 
> The parent device example is supposed to be 100% complete.

Will complete the example with MT6397 and MT6323 as reference in the 
next version.

> Best regards,
> Krzysztof

Thanks for the review and still some questions listed above.
Please help to clarify the correction method for the questions.

Best regards,
Macpaul Lin