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>
In-Reply-To: <20190429224143.192506-1-sjg@chromium.org>
From:   Rob Herring <robh@kernel.org>
Date:   Wed, 8 May 2019 14:20:46 -0500
Message-ID: <CAL_JsqL3HrX5K4A3bLBrkR7LKNTCSj+--ip7z-xiCrgpmdZN6A@mail.gmail.com>
Subject: Re: [PATCH] RFC: Example schema files written in Python
To:     Simon Glass <sjg@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, 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

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.

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.

> +
> +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?

> +            '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.

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.

> +        PropInterrupts(required=True,

How would you handle a property being conditionally required?

> +            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.

> +                        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?

Rob