lkml.org 
[lkml]   [2024]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] dt-bindings: mfd: mediatek: mt6397: Convert to DT schema format
From

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


\
 
 \ /
  Last update: 2024-08-23 08:46    [from the cache]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and my Meterkast|Read the blog