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
