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@xxxxxxxxxxxx>
Signed-off-by: Macpaul Lin <macpaul.lin@xxxxxxxxxxxx>
---
.../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@xxxxxxxxxxxx/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@xxxxxxxxxxxx>
+ - Macpaul Lin <macpaul.lin@xxxxxxxxxxxx>
+
+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
