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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof Kozlowski,

Thanks for the feedback.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> 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






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux