| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fb1d3c99-c524-adfa-94b7-822801b98034@mediatek.com>
Date: Fri, 23 Aug 2024 14:44:57 +0800
From: Macpaul Lin <macpaul.lin@...iatek.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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 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"
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
Powered by blists - more mailing lists