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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/08/2024 08:44, Macpaul Lin wrote:
> 
> 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"

What exactly is confusing? The other example is wrong. It's not a
schema, but free form text. Write schema so it would be applied and
would validate the DTS.

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

So investigate and fix this, instead of hiding the problem.

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

Where did you add the $ref? The child node should have it, not parent,
obviously. Just look at one of many other examples having children.

...

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

Really? Then checkout the maintainer tree, apply this patch and test
again. You know, it is impossible for us to apply a patch on top of
linux-next...


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

Or just use compatibles. There are also examples of this - in MFD
devices, Qcom display.

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

Oh, look here, I even mentioned Qualcomm to use as an 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.

Then I don't know what you want to express here.

> 
>>> +    description:
>>> +      Clock
>>
>> Your descriptions are useless. You just said "clk" node is "clock". Really?
> 
> Will improve it in next version.
Best regards,
Krzysztof