Messages in this thread |  | | Date | Fri, 23 Aug 2024 14:44:57 +0800 | | Subject | Re: [PATCH] dt-bindings: mfd: mediatek: mt6397: Convert to DT schema format | | From | Macpaul Lin <> |
| |
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
|  |