| Message ID | E1oo6Hw-00HYp8-Sa@rmk-PC.armlinux.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Add Apple macsmc GPIO support | expand |
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@armlinux.org.uk> > --- > .../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" > @@ -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@marcan.st> > + > +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. > + - 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. > + > +additionalProperties: false Missing example, it's necessary to validate these. Best regards, Krzysztof
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@armlinux.org.uk> > > --- > > .../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. > > @@ -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@marcan.st> > > + > > +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. 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: 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. > > + > > +additionalProperties: false > > Missing example, it's necessary to validate these. Documentation states that examples are optional according to the "writing-schema" documentation. 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!
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@armlinux.org.uk> >>> --- >>> .../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@marcan.st> >>> + >>> +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. > > 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. > >>> + >>> +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. > > 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. I would also say that C stuff is extremely difficult... Impossible to get it right. Wait... Best regards, Krzysztof
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@armlinux.org.uk> > >>> --- > >>> .../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@marcan.st> > >>> + > >>> +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.
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@armlinux.org.uk> >>>>> --- >>>>> .../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@marcan.st> >>>>> + >>>>> +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
On Thu, Oct 27, 2022 at 06:06:21PM -0400, Krzysztof Kozlowski wrote: > On 27/10/2022 17:49, Russell King (Oracle) wrote: > > 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? I wonder which bit of "I don't understand YAML" isn't obvious to you? The "-" means list isn't stated in yaml online tutorials, it seems one is just supposed to know this. I guess if one been in JS and similar landscapes it's obvious, but not everyone has. C is damn easy in comparison! :)
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 @@ -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@marcan.st> + +description: + This describes the binding for the Apple Mac System Management Controller + GPIO block. + +properties: + compatible: + allOf: + - enum: + - apple,t8103-smc + - const: apple,smc-gpio + + gpio-controller: true + + '#gpio-cells': + const: 2 + +additionalProperties: false
Add the DT binding for the Apple Mac System Management Controller GPIOs. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml