RE: [PATCH v4 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Lee Jones <lee@kernel.org>
Cc: Support Opensource <support.opensource@diasemi.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Zhang Rui <rui.zhang@intel.com>,
	Lukasz Luba <lukasz.luba@arm.com>,
	Steve Twiss <stwiss.opensource@diasemi.com>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	biju.das.au <biju.das.au@gmail.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v4 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema
Date: Sat, 9 Dec 2023 15:41:51 +0000	[thread overview]
Message-ID: <TYCPR01MB11269F887AE07FDA09F19112D8689A@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <11ec89a8-5fdd-456c-b070-988bbf8f312d@linaro.org>

Hi Krzysztof Kozlowski,

Thanks for the feedback.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, December 8, 2023 7:56 AM
> Subject: Re: [PATCH v4 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062
> to json-schema
> 
> On 07/12/2023 18:03, Biju Das wrote:
> 
> Trim the quote from irrelevant context, especially if your email client
> malforms replies... (because it does)
> 
> >>> @@ -35,6 +42,19 @@ properties:
> >>>    "#interrupt-cells":
> >>>      const: 2
> >>>
> >>> +  gpio:
> >>
> >> Old binding did not have such node and nothing in commit msg
> >> explained changes from pure conversion.
> >
> > OK will update the commit message. Check patch complained about
> undocumented compatible.
> >
> >>
> >>> +    type: object
> >>> +    $ref: /schemas/gpio/gpio.yaml#
> >>
> >> ?!? Why? First: It's always selected. Second, so you have two gpio
> >> controllers? And original binding had 0? Why this is not explained at
> all?
> >> Open the binding and look at its properties.
> >
> >
> > I have referred[1] and [2] to add gpio controller property.
> >
> 
> But does it make sense? Don't just blindly copy things, but actually
> investigate whether this is correct DTS.

It is indeed incorrect. 

I have tested GPIO on my board. The gpio controller must be defined in parent node.
Otherwise gpio probe will fail.

the dt example is as below

da9062: pmic@58 {
		compatible = "dlg,da9062";
		reg = <0x58>;
		gpio-controller;
 		#gpio-cells = <2>;

		sd0-pwr-sel {
			gpio-hog;
			gpios = <1 0>;
			input;
			line-name = "SD0_PWR_SEL";
		};

		sd1-pwr-sel {
			gpio-hog;
			gpios = <2 0>;
			input;
			line-name = "SD1_PWR_SEL";
		};

		sw-et0-en {
			gpio-hog;
			gpios = <3 0>;
			input;
			line-name = "SW_ET0_EN#";
		};

		pmic-good {
			gpio-hog;
			gpios = <4 0>;
			output-high;
			line-name = "PMIC_PGOOD";
		};

		da9062_rtc: rtc {
			compatible = "dlg,da9062-rtc";
		};

		da9062_onkey: onkey {
			compatible = "dlg,da9062-onkey";
			status = "disabled";
		};

		watchdog {
			compatible = "dlg,da9062-watchdog";
			status = "disabled";
		};

		thermal {
			compatible = "dlg,da9062-thermal";
			status = "disabled";
		};

		gpio {
			compatible = "dlg,da9062-gpio";
		};
	};

> 
> >
> >>
> >>
> >>> +    unevaluatedProperties: false
> >>> +    properties:
> >>> +      compatible:
> >>> +        const: dlg,da9062-gpio
> >>> +
> >>> +  gpio-controller: true
> >>
> >> And here is the second gpio-controller...
> >
> > So you mean it is redundant as $ref: /schemas/gpio/gpio.yaml has
> > already defined gpio-controller for this object??
> 
> I meant this would mean you have two GPIO controllers. Why one device
> would have two GPIO controllers? Please answer to this in commit msg, so
> there will be no questions/concerns. You have entire commit msg to explain
> all weird and unexpected things with this binding.

This is correct. gpio-controller should be defined in the parent node.
Otherwise gpio probe will fail.

> 
> ...
> 
> >>> +    #include <dt-bindings/interrupt-controller/irq.h>
> >>> +    #include <dt-bindings/regulator/dlg,da9063-regulator.h>
> >>> +    i2c {
> >>> +      #address-cells = <1>;
> >>> +      #size-cells = <0>;
> >>> +      pmic@58 {
> >>> +        compatible = "dlg,da9062";
> >>> +        reg = <0x58>;
> >>> +        #interrupt-cells = <2>;
> >>> +        interrupt-parent = <&gpio1>;
> >>> +        interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> >>> +        interrupt-controller;
> >>> +
> >>> +        gpio {
> >>> +          compatible = "dlg,da9062-gpio";
> >>> +          status = "disabled";
> >>
> >> Why?
> 
> Why disabling? Drop all statuses from all your binding examples.
> 
> >> Where are gpio-controller and cells? For this node and for parent?
> >> Why this example is incomplete?
> >
> > Currently I don't see any driver is using this compatible other than
> MFD.
> 
> Open the MFD so you will see it...

Actually, found the driver and tested GPIOs, 
For input gpio, I can see the sd1_pwr_sel values are 
toggled during card insert/removal. 
For outout gpio,
System is entering into reset mode, if I set output-low in DT. So set
Init state as output-high to avoid reset.

drivers/pinctrl/pinctrl-da9062.c

Cheers,
Biju