Re: [PATCH v3] dt-bindings: fpga: Convert fpga-region binding to yaml

[Rob Herring]

On Thu, Jan 18, 2024 at 01:34:16PM +0100, Michal Simek wrote:
> 
> 
> On 1/17/24 22:47, Rob Herring wrote:
> > On Wed, Jan 17, 2024 at 12:30:58PM +0100, Michal Simek wrote:
> > > Convert the generic fpga region DT binding to json-schema.
> > > There are some differences compare to txt version.
> > > 1. DT overlay can't be described in example that's why directly include
> > > information from overlay to node which was referenced. It is visible in
> > > example with /* DT Overlay contains: &... */
> > > 
> > > 2. All example have been rewritten to be simpler and describe only full
> > > reconfiguration and partial reconfiguration with one bridge.
> > > Completely drop the case where fpga region can inside partial
> > > reconfiguration region which is already described in description
> > > 
> > > 3. Fixed some typos in descriptions compare to txt version but most of it
> > > is just c&p from txt file.
> > > 
> > > Signed-off-by: Michal Simek <michal.simek@xxxxxxx>

[...]

> > > +additionalProperties: true
> > 
> > Why? This should only be used if another schema is going to include this
> > one. That's not the case here.
> 
> In v2 we discussed this with Krzysztof. I used pattern properties from
> simple-bus.yaml in v2.
> 
> https://lore.kernel.org/all/b2dd8bcd-1e23-4b68-b7b7-c01b034fc1fe@xxxxxxxxxx/

You didn't answer his question. You just picked up 
'additionalProperties: true' which is easy because it avoids 'problems'.

His question was is this a common schema referenced by other schemas? If 
so, then use 'additionalProperties: true'. But it is not. You've 
defined exactly what 'compatible' must be and that means it can't be a 
common schema.

> 
> > 
> > 'type: object' would be acceptable here as that says only nodes can be
> > added.
> 
> What do you think should be patternProperties in this case?

You are the one with FPGAs, what do you need?


> I played with it a little bit but all nodes with @ are likely object type.

'@' is only allowed in node names, so it's more than just likely.

> But what to do with others?
> There are nodes as you see in examples like fixed-factor-clock nodes which
> are also object type.

Then the node names can be anything and you should use what I suggested.

> Standard property like firmware-name or encrypted-fpga-config are coming via
> overlay for sure. Others are not permitted. That's why others then listed
> properties likely must be type object. Is there an elegant way to encode it?

Sorry, I don't follow. You should list every possible property, then the 
only thing left are nodes and my suggestion covers them. If there's a 
pattern to the node names, then you can use patternProperties.

> > > +
> > > +examples:
> > > +  - |
> > > +    /*
> > > +     * Full Reconfiguration without Bridges with DT overlay
> > > +     */
> > > +    fpga_region0: fpga-region {
> > 
> > Drop unused labels.
> 
> Actually it is kind of used. I kept it to be able to reference something in
> comment below.

Okay. Kind of outside the scope of examples and schema as I mentioned.


> > > +      compatible = "fpga-region";
> > > +      #address-cells = <1>;
> > > +      #size-cells = <1>;
> > > +      fpga-mgr = <&fpga_mgr0>;
> > > +
> > > +      /* DT Overlay contains: &fpga_region0 */
> > > +      firmware-name = "zynq-gpio.bin";
> > > +      gpio@40000000 {
> > > +        compatible = "xlnx,xps-gpio-1.00.a";
> > > +        reg = <0x40000000 0x10000>;
> > 
> > This example implies this is a translatable address, but the lack of
> > 'ranges' in the parent prevents that. In turn, that means unit-addresses
> > should be accepted in the parent node name as well.
> 
> v1 contains ranges property and it was removed based on Krzysztof comment
> that fpga-region has no unit address that's why ranges shouldn't be used.
> 
> https://lore.kernel.org/all/c3c92468-a1db-498b-b4a2-7926b84cb5ea@xxxxxxxxxx/

Plain "ranges;" does not have a unit-address. But really, you should 
allow a non-empty ranges too and therefore allow for a unit-address.

Rob