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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!