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
