Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190429224143.192506-1-sjg@chromium.org> <CAL_JsqL3HrX5K4A3bLBrkR7LKNTCSj+--ip7z-xiCrgpmdZN6A@mail.gmail.com>
In-Reply-To: <CAL_JsqL3HrX5K4A3bLBrkR7LKNTCSj+--ip7z-xiCrgpmdZN6A@mail.gmail.com>
From:   Simon Glass <sjg@chromium.org>
Date:   Mon, 20 May 2019 14:51:04 -0600
Message-ID: <CAPnjgZ1YXUK1+hXgLf_GoS=WybnR+qhEpLPys+ikbQ-_epzUWQ@mail.gmail.com>
Subject: Re: [PATCH] RFC: Example schema files written in Python
To:     Rob Herring <robh@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Devicetree Discuss <devicetree@vger.kernel.org>,
        Grant Likely <grant.likely@secretlab.ca>,
        Frank Rowand <frank.rowand@sony.com>,
        David Gibson <david@gibson.dropbear.id.au>,
        Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Rob,

On Wed, 8 May 2019 at 13:21, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Apr 29, 2019 at 5:41 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Most of these are hand-written, but xilinx-xadc.py is auto-generated by
> > binding_to_py.py as an example of the use of that tool.
> >
> > This is part of a proof-of-concept device-tree validator. See the patch
> > on the dtc mailing list for details:
>
> Honestly, we are pretty far down the path of using json-schema to
> consider changing to something else. We've already gone thru plenty of
> concepts over the years with different languages for the schema.

I don't think I saw much of that. I did hear that others has suggested
such options but the yaml/json design is the only one I'm aware of.

Anyway, it sounds like things are pretty set in stone right now. Even
so, I'll reply to this email.

>
> While I think there are some cases where being able to do schema with
> code is useful or necessary, the vast majority of cases can be handled
> just fine with structured data. I'd rather see how we could augment
> the data with code. Maybe that's snippets of code within the schema or
> making the validation code more modular. I would like to see the dtc
> checks infrastructure be extendable without modifying dtc. That could
> include supporting checks written in python.
>
> One example where we need more than just schema data is validating
> properties that depend on a provider #.*-cells property. We can't
> really do that with json-schema. At least the number of cells being
> correct is covered by dtc already. So it would really be how do we
> validate the cell data itself. OTOH, I think that is pretty far down
> the list in priorities of things to validate. There's already
> thousands of warnings generated by dtc and the json-schema which are
> slow to get fixed (though some are really subjective and more what to
> avoid for new users).
>
> >
> >    RFC: Python-based device-tree validation
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
>
> I'll use this one to comment on. Comments are most around goals for
> the binding doc format.
>
> > diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.py b/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.py
> > new file mode 100644
> > index 0000000000000..9f55f48f7cde7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.py
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +
> > +# Xilinx XADC device driver
>
> Having some defined structure at the top-level is beneficial for
> extracting data and automating review checks.

What does this refer to?

>
> > +
> > +from kschema import NodeDesc, PropBool, PropClocks, PropInt, PropIntList, PropInterrupts, PropReg, PropStringList
> > +
> > +schema = [
> > +    NodeDesc('xilinx-xadc', ['xlnx,zynq-xadc-1.00.a', 'xlnx,axi-xadc-1.00.a'], False, desc=
>
> If one desires to generate a list of all possible compatible strings
> (to find undocumented ones), how would you do that?

Read in all the schema files and then walk through the entire schema
looking for compatible strings.

>
> > +            'This binding document describes the bindings for both of them since the'
> > +            'bindings are very similar. The Xilinx XADC is a ADC that can be found in the'
> > +            'series 7 FPGAs from Xilinx. The XADC has a DRP interface for communication.'
> > +            'Currently two different frontends for the DRP interface exist. One that is only'
> > +            'available on the ZYNQ family as a hardmacro in the SoC portion of the ZYNQ. The'
> > +            'other one is available on all series 7 platforms and is a softmacro with a AXI'
> > +            'interface. This binding document describes the bindings for both of them since'
> > +            'the bindings are very similar.', elements=[
>
> One goal with the schema (at least core ones) is to generate
> documentation from it. That would need to be a format such as rST so
> we can have formatting. And we'd want to be able to parse the
> properties and generate tables from them.

The docs above are taken verbatim from the binding, so there is no
formatting really, except for blank lines.1

>
> If someone really gets an itch, we'll rewrite sections of the DT spec
> in schema.
>
> > +        PropReg(required=True,
> > +            desc='Address and length of the register set for the device'),
>
> For any standard property, we'd have to create the class before
> bindings can use it.

There is a 'generic' property (PropDesc) which is the base class for
all properties. So most properties would not have their own class.

>
> > +        PropInterrupts(required=True,
>
> How would you handle a property being conditionally required?

The cond_props dictionary is attached to each property. See
ElementPresent for the implementation.

>
> > +            desc='Interrupt for the XADC control interface.'),
> > +        PropClocks(required=True,
> > +            desc='When using the ZYNQ this must be the ZYNQ PCAP clock,'
> > +            'when using the AXI-XADC pcore this must be the clock that provides the'
> > +            'clock to the AXI bus interface of the core.'),
> > +        PropStringList('xlnx,external-mux', str_pattern='none|single|dual',
> > +            desc=''),
> > +        PropIntList('xlnx,external-mux-channel', valid_list='0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|16|1|2|3|4|5|6|8',
> > +            desc='Configures which pair of pins is used to'
> > +            'sample data in external mux mode.'
> > +            'Valid values for single external multiplexer mode are:'
> > +            'Valid values for dual external multiplexer mode are:'
> > +            ''
> > +            'This property needs to be present if the device is configured for'
> > +            'external multiplexer mode (either single or dual). If the device is'
> > +            'not using external multiplexer mode the property is ignored.'),
> > +        NodeDesc('xlnx,channels', None, False, desc=
> > +                'List of external channels that are connected to the ADC', elements=[
> > +            PropInt('#address-cells', required=True,
> > +                desc='Should be 1.'),
> > +            PropInt('#size-cells', required=True,
> > +                desc='Should be 0.'),
> > +            NodeDesc('None', None, False, desc=
> > +                    'The child nodes of this node represent the external channels which are'
> > +                    'connected to the ADC. If the property is no present no external'
> > +                    'channels will be assumed to be connected.', elements=[
> > +                NodeDesc('None', None, False, desc=
> > +                        'Each child node represents one channel and has the following'
> > +                        'properties:', elements=[
> > +                    PropIntList('reg', required=True, valid_list='0|1|2|3|4|5|6|7|8|9|10|11|12|13|14|16',
>
> We need a different method or arg for every possible way we need to
> express constraints? For example, say the value must be a power of 2.

That's one of the nice things about Python is that it is easy to code
up a custom validator. See the Validate() method in each class.

At some point it might worth putting this sort of thing into its own
validator class. How is this done with yaml?

>
> > +                        desc='Pair of pins the channel is connected to.'
> > +                        'Note each channel number should only be used at most'
> > +                        'once.'),
> > +                    PropBool('xlnx,bipolar',
> > +                        desc='If set the channel is used in bipolar'
> > +                        'mode.'),
> > +                    ]),
> > +                ]),
> > +            ]),
> > +        ]),
> > +    ]
> > diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt b/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt
> > index e0e0755cabd8a..24def33e6d6b8 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt
> > +++ b/Documentation/devicetree/bindings/iio/adc/xilinx-xadc.txt
> > @@ -32,24 +32,26 @@ Optional properties:
> >         - xlnx,external-mux-channel: Configures which pair of pins is used to
> >           sample data in external mux mode.
> >           Valid values for single external multiplexer mode are:
> > -               0: VP/VN
> > -               1: VAUXP[0]/VAUXN[0]
> > -               2: VAUXP[1]/VAUXN[1]
> > +               * 0: VP/VN
> > +               * 1: VAUXP[0]/VAUXN[0]
> > +               * 2: VAUXP[1]/VAUXN[1]
>
> Not really automatic conversion if you have to tweak the source. Is
> your thought we'd make the txt files more structured to do automatic
> conversions or we'd commit the python files?

I was hoping to fix up the binding files a little, such that automatic
conversion is good enough, make sure that the Python file can emit the
original binding (in whatever format is chosen) then commit the Python
files as the source of truth.

Regards,
Simon