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. So, stuff like "-" means list is just not obvious, and the error messages make it totally unobvious that's what the problem was. > > Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml: properties:compatible: [{'items': [{'const': 'apple,t8103-smc'}, {'const': 'apple,smc-gpio'}]}] is not of type 'object', 'boolean' from schema $id: http://json-schema.org/draft-07/schema# > > > >>> + - enum: > >>> + - apple,t8103-smc > >>> + - const: apple,smc-gpio > >>> + > >>> + gpio-controller: true > >>> + > >>> + '#gpio-cells': > >>> + const: 2 > >> > >> Missing required properties. Start from new bindings or example-schema. > > > > What's missing? All the other bindings that I see follow this pattern. > > No. All other bindings have list of required properties. Only yours do > not have. Oh, you don't mean there are required properties missing from #gpio-cells, you mean the list of required properties is missing, which is something _entirely_ different. Your comment was utterly ambiguous. > >>> + > >>> +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 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. 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. 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
