Re: [PATCH v2 1/4] dt-bindings: mfd: Add bindings for the Mediatek MT6360 PMIC

From: Lee Jones
Date: Mon Jun 22 2020 - 06:24:27 EST

On Fri, 19 Jun 2020, Gene Chen wrote:

> From: Gene Chen <gene_chen@xxxxxxxxxxx>
>
> Add devicetree binding document support Mediatek MT6360 PMIC
>
> Signed-off-by: Gene Chen <gene_chen@xxxxxxxxxxx>
> ---
> Documentation/devicetree/bindings/mfd/mt6360.txt | 122 +++++++++++++++++++++++

This needs converting to YAML.

> include/dt-bindings/mfd/mt6360.h | 15 +++
> 2 files changed, 137 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/mt6360.txt
> create mode 100644 include/dt-bindings/mfd/mt6360.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/mt6360.txt b/Documentation/devicetree/bindings/mfd/mt6360.txt
> new file mode 100644
> index 0000000..7d7d349
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mt6360.txt
> @@ -0,0 +1,122 @@
> +MediaTek MT6360 PMIC Driver
> +
> +MT6360 is a multifunction device with the following sub modules:

Sub modules do not follow this sentence.

Please do not use the term "multi-function" in DT bindings. An MFD is
something we made up. It's a Linuxisum and has no real meaning in
DT. This device is not multi-functional. It has one function, to
control Power. The multi-function part comes from the fact that we
like things split-up into subsystem for organisation purposes and
simplicity.

> +It is interfaced to host controller using I2C interface.
> +This document describes the binding for PMIC device and its sub module.

This sentence should be at the top.

s/sub module/sub-devices/

> +- ADC
> +- Battery Charger/OTG boost
> +- Flash LED/RGB LED/moonlight LED
> +- 2-channel Buck and 6-channel LDO
> +- USB_PD

These should follow the sentence reviewing to the "sub modules".

> +Required properties:
> +- compatible: Must be "mediatek,mt6360-pmu"
> +- reg: Specifies the I2C slave address of PMIC block, Must be <0x34>
> +- interrupts: I2C device IRQ line connected to the main SoC.

Remove the '.'.

> +Optional subnodes:
> +- ADC
> + Required properties:
> + - compatible: "mediatek,mt6360-adc"
> +- battery charger/OTG boost

"Battery"

> + Required properties:
> + - compatible: "mediatek,mt6360-chg"
> +- Flash LED/RGB LED/moonlight LED
> + Required properties:
> + - compatible: "mediatek,mt6360-led"
> +- 2-channel Buck and 6-channel LDO
> + Required properties:
> + - compatible: "mediatek,mt6360-regulator"
> +- USB_PD

Remove the "_" and expand PD.

> + Required properties:
> + - compatible: "mediatek,mt6360-tcpc"
> +
> +Example:
> +
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/mfd/mt6360.h>
> +
> + mt6360@34 {
> + compatible = "mediatek,mt6360";
> + reg = <0x34>;
> + wakeup-source;
> + interrupts-extended = <&gpio26 0 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "IRQB";
> + interrupt-controller;
> + #interrupt-cells = <1>;

'\n'

> + adc {
> + compatible = "mediatek,mt6360-adc";
> + #io-channel-cells = <1>;

Where is the channel cell?

> + };

'\n'

> + regulator {
> + compatible = "mediatek,mt6360-regulator";
> + LDO_VIN3-supply = <&BUCK2>;

'\n'

> + buck1 {
> + regulator-compatible = "BUCK1";
> + regulator-name = "mt6360,buck1";
> + regulator-min-microvolt = <300000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP
> + MT6360_OPMODE_ULP>;
> + };

'\n'

Etc etc.

> + BUCK2: buck2 {
> + regulator-compatible = "BUCK2";
> + regulator-name = "mt6360,buck2";
> + regulator-min-microvolt = <300000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP
> + MT6360_OPMODE_ULP>;
> + };
> + ldo6 {
> + regulator-compatible = "LDO6";
> + regulator-name = "mt6360,ldo6";
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <2100000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> + ldo7 {
> + regulator-compatible = "LDO7";
> + regulator-name = "mt6360,ldo7";
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <2100000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> + ldo1 {
> + regulator-compatible = "LDO1";
> + regulator-name = "mt6360,ldo1";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <3600000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> + ldo2 {
> + regulator-compatible = "LDO2";
> + regulator-name = "mt6360,ldo2";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <3600000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> + ldo3 {
> + regulator-compatible = "LDO3";
> + regulator-name = "mt6360,ldo3";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <3600000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };
> + ldo5 {
> + regulator-compatible = "LDO5";
> + regulator-name = "mt6360,ldo5";
> + regulator-min-microvolt = <2700000>;
> + regulator-max-microvolt = <3600000>;
> + regulator-allowed-modes = <MT6360_OPMODE_NORMAL
> + MT6360_OPMODE_LP>;
> + };

Why aren't the LDO cells in order?

> + };
> + };
> diff --git a/include/dt-bindings/mfd/mt6360.h b/include/dt-bindings/mfd/mt6360.h
> new file mode 100644
> index 0000000..6368388
> --- /dev/null
> +++ b/include/dt-bindings/mfd/mt6360.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides macros for MT6360 device bindings.
> + *
> + * Copyright (c) 2020 Mediatek Inc.
> + */
> +
> +#ifndef __DT_BINDINGS_MT6360_H__
> +#define __DT_BINDINGS_MT6360_H__
> +
> +#define MT6360_OPMODE_LP (2)
> +#define MT6360_OPMODE_ULP (3)
> +#define MT6360_OPMODE_NORMAL (0)
> +
> +#endif /* __DT_BINDINGS_MT6360_H__ */
> --
> 2.7.4
>

--
Lee Jones [æçæ]
Senior Technical Lead - Developer Services
Linaro.org â Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog