Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 22 Jun 2020 11:24:20 +0100
From:   Lee Jones <lee.jones@linaro.org>
To:     Gene Chen <gene.chen.richtek@gmail.com>
Cc:     jic23@kernel.org, jacek.anaszewski@gmail.com, pavel@ucw.cz,
        matthias.bgg@gmail.com, knaack.h@gmx.de, lars@metafoo.de,
        pmeerw@pmeerw.net, dmurphy@ti.com, lgirdwood@gmail.com,
        broonie@kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        gene_chen@richtek.com, shufan_lee@richtek.com,
        cy_huang@richtek.com, benjamin.chao@mediatek.com
Subject: Re: [PATCH v2 1/4] dt-bindings: mfd: Add bindings for the Mediatek
 MT6360 PMIC
Message-ID: <20200622102420.GR954398@dell>
References: <1592567631-20363-1-git-send-email-gene.chen.richtek@gmail.com>
 <1592567631-20363-2-git-send-email-gene.chen.richtek@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1592567631-20363-2-git-send-email-gene.chen.richtek@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 19 Jun 2020, Gene Chen wrote:

> From: Gene Chen <gene_chen@richtek.com>
> 
> Add devicetree binding document support Mediatek MT6360 PMIC
> 
> Signed-off-by: Gene Chen <gene_chen@richtek.com>
> ---
>  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