Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 27/10/2022 17:49, Russell King (Oracle) wrote:
> On Thu, Oct 27, 2022 at 05:31:49PM -0400, Krzysztof Kozlowski wrote:
>> On 27/10/2022 17:00, Russell King (Oracle) wrote:
>>> On Thu, Oct 27, 2022 at 03:53:25PM -0400, Krzysztof Kozlowski wrote:
>>>> On 27/10/2022 13:03, Russell King (Oracle) wrote:
>>>>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
>>>>>
>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
>>>>> ---
>>>>>  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
>>>>>  1 file changed, 28 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..a3883d62292d
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>>
>>>> Filename based on compatible, so "apple,smc-gpio.yaml"
>>>
>>> Many of the other yaml files in gpio/ are named as such.
>>
>> Poor patterns, inconsistencies or even bugs like to copy themselves and
>> it is never an argument.
>>
>> The convention for all bindings is to use vendor,device.yaml, matching
>> the compatible when applicable.
>>
>>>
>>>>> @@ -0,0 +1,28 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Apple Mac System Management Controller GPIO
>>>>> +
>>>>> +maintainers:
>>>>> +  - Hector Martin <marcan@xxxxxxxxx>
>>>>> +
>>>>> +description:
>>>>> +  This describes the binding for the Apple Mac System Management Controller
>>>>
>>>> Drop "This describes the binding for"
>>>>
>>>>> +  GPIO block.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    allOf:
>>>>
>>>> That's not proper syntax. Look at other examples (e.g. Apple bindings)
>>>> doing it. Probably you wanted items here.
>>>
>>> Really? You're joking. 
>>
>> No. If you look at example-schema then answer should be obvious, so why
>> do you think I am joking?
>>
>>> I had sent an email to Rob to ask how this should
>>> be done because my first guess spat out unhelpful error messages from
>>> dt_bindings_check, and this is the best I could come up with based on
>>> other "examples".
>>>
>>> I tried "- items:" but that made no difference - dt_bindings_check spat
>>> errors, so that's clearly incorrect. Specifically, I tried:
>>>
>>> properties:
>>>   compatible:
>>>     - items:
>>>         - enum:
>>> 	    - apple,t8103-smc
>>> 	- const: apple,smc-gpio
>>>
>>> That doesn't work:
>>
>> Of course, because "-" means list, so "- items" is not correct.
>>
>> Where do you see such pattern? Anywhere following compatible? No. There
>> is no. You just invented something instead of using many, many existing
>> examples.
> 
> No, I did not "invent" something here. I tried to copy it from other
> examples, but I couldn't find something that matched exactly.
> 
> In any case, relying on examples rather than a proper description of
> how this should be done is utterly rediculous. There should be a formal
> definition of the language used to describe this - but there doesn't
> seem to be.

There is...

> 
> So, stuff like "-" means list is just not obvious, and the error

"-" is defined by YAML, so I do not understand what is here not obvious?

> messages make it totally unobvious that's what the problem was.

The error messages could indeed be improved, I agree.

> 
>>>>> +
>>>>> +additionalProperties: false
>>>>
>>>> Missing example, it's necessary to validate these.
>>>
>>> Documentation states that examples are optional according to the
>>> "writing-schema" documentation.
>>
>> Yes, but without it we cannot validate the bindings.
> 
> Please update the writing-schema documentation to state that it's now
> required to validate bindings, so that the documentation is no longer
> stating something that's different from the required process.
> 
>>> Honestly, I find this YAML stuff extremely difficult, especially given
>>> the lack of documentation on how to write it and the cryptic error
>>> messages from the tooling. It's impossible to get it right before
>>> submitting it - and I suspect from what I see above, it's impossible
>>> for reviewers to know what is correct either, since some of what you've
>>> said above appears to be wrong!
>>
>> I would say it is doable - copy example-schema or recent device specific
>> schema and customize it... But you started adding some weird stuff which
>> was never, never in other bindings.
> 
> "weird stuff"? What weird stuff? What wasn't in other bindings? You

The "allOf" in compatible is the weird stuff.

There is basically just one case with it, a special case. There are no
other bindings using such pattern.

> make no sense when you make this accusations, because they are totally
> untrue.
> 
> I started with:
> 
> properties:
>   compatible:
>     - enum:
>        - ...
>     - const: ...
> 
> and dt_bindings_check thew it out. So I looked again at
> Documentation/bindings/gpio/*.yaml. I decided maybe the - enum
> containing one entry could be confusing matters, so I tried converting
> that to a - const. Still failed.

Fourth YAML binding (counting alphabetically) in gpios has it:
Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml

> 
> So I had another look at other *.yaml files, and I then tried adding
> - items: and indenting the following. Failed.
> 
> So then I tried allOf: which passed the checks. That's the evolution
> there - trial and error.

I understand your process. I still think that easier is to start from
example-schema as it has this case exactly.

> 
> Cryptic error messages, nothing else in gpio/ that follows the pattern
> I wanted and trial and error led me to what I had in this patch. This
> is *no* way to develop bindings.
> 
> There has to be a formal definition of this schema language - and
> something better than pointing people at other bindings that may or
> may not be correct.
> 
> So, I repeat myself: writing yaml stuff is utterly horrid and a total
> hit and miss affair whether one gets it correct or not.
> 
> It seems to me that the problem of validating .dts files hasn't been
> solved - the problem has merely been moved to a whole set of different
> problems trying to write .yaml files that allow .dts files to be
> validated, some of which could be solved by a better understanding of
> the syntax, if only it were documented properly.

I repeat also myself, writing C is also difficult and horrid :)

Anyway, your feedback is of course appreciated. Happy to help if you
have some more questions.

Best regards,
Krzysztof