Re: [PATCH 8/8] dt-bindings: firmware: arm,scmi: Convert to json schema

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, May 27, 2021 at 11:33:23AM +0100, Cristian Marussi wrote:
> Hi Sudeep,
>
> Some feedback down below.
>
> On Wed, May 26, 2021 at 07:28:07PM +0100, Sudeep Holla wrote:

[...]

> > +patternProperties:
> > +  '^protocol@[0-9a-f]+$':
> > +    type: object
> > +    description: |
> > +      Each sub-node represents a protocol supported. If the platform
> > +      supports dedicated communication channel for a particular protocol,
> > +      then corresponding transport properties must be present.
> > +
>
> Not sure if it's needed, but maybe a reference or an example to which
> are the transport properties could be useful in this description.
>

Good point, I will try to add that example.

> > +    properties:
> > +      reg:
> > +        maxItems: 1
> > +
>
> Shouldn't be expressed that reg is required for these protocol
> patternProperties ? (no sure how though...:D)
>

Hmm, right again need to explore on that.

> > +      '#clock-cells':
> > +        const: 1
> > +
> > +      '#reset-cells':
> > +        const: 1
> > +
> > +      '#power-domain-cells':
> > +        const: 1
> > +
> > +      '#thermal-sensor-cells':
> > +        const: 1
> > +
>
> Maybe it does not matter, but all the info present in the old .txt binding
> about references to external std bindings like:
>
> > -This binding for the SCMI power domain providers uses the generic
> > power
> > -domain binding[2].
>
> is no more reported in yaml. Is it fine ?
>

I think we can add it as $ref if there is yaml schema, I really don't
think old .txt based reference adds anything.

> > +required:
> > +  - compatible
> > +  - shmem
>
> Indeed shmem is required by chance all the transports defined in this
> binding, but it is not really something generally required, in fact
> virtio transport won't require it.
>
> But I'm not sure if it's better to move it now down under some kind of
> if: arm,scmi|arm,scmi-smc or just do it later when virtio transport binding
> will be defined/introduced.
>

Yes I was aware of that fact while I wrote this and expect it to be part of
virtio update.

--
Regards,
Sudeep