[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e3f2f022-aacc-41ed-8d29-b341c903ae9c@kernel.org>
Date: Fri, 23 Aug 2024 10:05:29 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Macpaul Lin <macpaul.lin@...iatek.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Matthias Brugger <matthias.bgg@...il.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Lee Jones <lee@...nel.org>
Cc: Bear Wang <bear.wang@...iatek.com>, Pablo Sun <pablo.sun@...iatek.com>,
Macpaul Lin <macpaul@...il.com>, Sen Chu <sen.chu@...iatek.com>,
Jason-ch Chen <Jason-ch.Chen@...iatek.com>,
Chris-qj chen <chris-qj.chen@...iatek.com>,
MediaTek Chromebook Upstream
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
Alexandre Mergnat <amergnat@...libre.com>, Chen-Yu Tsai <wenst@...omium.org>
Subject: Re: [PATCH] dt-bindings: mfd: mediatek: mt6397: Convert to DT schema
format
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@...iatek.com>
>>> Signed-off-by: Macpaul Lin <macpaul.lin@...iatek.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@...iatek.com>
>>> + - Macpaul Lin <macpaul.lin@...iatek.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
Powered by blists - more mailing lists