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

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Conor Dooley <conor@kernel.org>
Cc: 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>,
	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 v3.1 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema
Date: Tue, 5 Dec 2023 18:52:49 +0000	[thread overview]
Message-ID: <TYCPR01MB112693ED613DEAF3735282EF38685A@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20231205-negate-matrix-0e55c151017b@spud>

Hi Conor Dooley,

Thanks for the feedback.

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Tuesday, December 5, 2023 5:12 PM
> Subject: Re: [PATCH v3.1 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062
> to json-schema
> 
> On Mon, Dec 04, 2023 at 05:25:10PM +0000, Biju Das wrote:
> 
> > @@ -60,8 +85,65 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupts
> > -  - interrupt-controller
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - dlg,da9063
> > +              - dlg,da9063l
> > +    then:
> > +      properties:
> > +        thermal: false
> > +        gpio: false
> > +        gpio-controller: false
> > +        "#gpio-cells": false
> > +        regulators:
> > +          patternProperties:
> > +            "^buck[1-4]$": false
> > +      required:
> > +        - interrupts
> > +        - interrupt-controller
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - dlg,da9062
> > +    then:
> > +      properties:
> > +        regulators:
> > +          patternProperties:
> > +            "^(ldo([5-9]|10|11)|bcore([1-2]|s-
> merged)|b(pro|mem|io|peri)|bmem-bio-merged)$": false
> > +      required:
> > +        - gpio
> > +        - onkey
> > +        - rtc
> > +        - thermal
> > +        - watchdog
> 
> Why are these required for the 9062 (and another set for the 9061)?
> The original binding does not seem to require them, nor does the yaml
> binding for the 9063.

The core drivers for 9063 and 9062 devices are different. Absence of child node using da9062 core driver
is giving error messages like (eg: da9062-gpio: Failed to locate of_node [id: -1]).

So, Geert suggested to make them as required one[1]

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20231201110840.37408-6-biju.das.jz@bp.renesas.com/

> 
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - dlg,da9061
> > +    then:
> > +      properties:
> > +        gpio: false
> > +        gpio-controller: false
> > +        "#gpio-cells": false
> > +        regulators:
> > +          patternProperties:
> > +            "^(ldo([5-9]|10|11)|bcore([1-2]|s-
> merged)|b(pro|mem|io|peri)|bmem-bio-merged|buck4)$": false
> > +        rtc: false
> > +      required:
> > +        - onkey
> > +        - thermal
> > +        - watchdog
> >
> >  additionalProperties: false
> >
> > @@ -118,4 +200,98 @@ examples:
> >          };
> >        };
> >      };
> > +
> > +  - |
> > +    #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 add it disabled? This should be enabled IMO.

Same as above.

Cheers,
Biju

> 
> > +        rtc {
> > +          compatible = "dlg,da9062-rtc";
> > +          status = "disabled";
> > +        };
> > +
> > +        thermal {
> > +          compatible = "dlg,da9062-thermal";
> > +          status = "disabled";
> > +        };
> 
> Ditto for these.
> 
> Thanks,
> Conor.