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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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. 
> 
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl-phytec-phycore-som.dtsi?h=next-20231207#n95
> 
> [2]
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mfd/da9062.txt?h=next-20231207#n38

But does it make sense? Don't just blindly copy things, but actually
investigate whether this is correct DTS.

> 
>>
>>
>>> +    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.

...

>>> +    #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?
> 
> I have used a real example [1] here.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl-phytec-phycore-som.dtsi?h=next-20231207#n95
> 
> Currently I don't see any driver is using this compatible other than MFD.

Open the MFD so you will see it...


Best regards,
Krzysztof