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
