Re: [PATCH v1 1/3] dt-bindings: i3c: Add Qualcomm I3C master controller bindings

[Mukesh Kumar Savaliya]

Hi Krzysztof,  Thanks !

On 2/5/2025 8:12 PM, Krzysztof Kozlowski wrote:
> On 05/02/2025 15:31, Mukesh Kumar Savaliya wrote:
> > Add device tree bindings for the Qualcomm I3C master controller. This
> > includes the necessary documentation and properties required to describe
> > the hardware in the device tree.
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
Sure
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Use modern terminology, which means:
> s/master/whatever else or even nothing/
> See other recent bindings and discussions.
Sure
> > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > ---
> >   .../bindings/i3c/qcom,i3c-master.yaml         | 57 +++++++++++++++++++
> >   1 file changed, 57 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/i3c/qcom,i3c-master.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/i3c/qcom,i3c-master.yaml b/Documentation/devicetree/bindings/i3c/qcom,i3c-master.yaml
> > new file mode 100644
> > index 000000000000..ad63ea779fd6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i3c/qcom,i3c-master.yaml
>
> Filename matching compatible.
Changed compatible to "qcom,i3c-master"
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/i3c/qcom,i3c-master.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm I3C master controller
> > +
> > +maintainers:
> > +  - Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
> > +
> > +allOf:
> > +  - $ref: i3c.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,geni-i3c
>
> No SoC? So to be sure: you claim all future SoCs will be using exactly
> the same interface. No new compatibles, no new properties will be added.
I think i should remove const. kept it for now as no other compatible to 
be added as of now.

let me remove const.

SoC name is not required, as this compatible is generic to all the SOCs.
> > +
> > +  reg:
> > +    minItems: 1
>
> Drop
Not required ? I see other bindings are using it, so please confirm if i 
can remove this.
> > +    maxItems: 2
>
> Drop and instead list and describe items
Okay, i can remove maxItems if not mandatory. Taken cdns,i3c-master.yaml 
and added these.

> > +
> > +  clocks:
> > +    minItems: 1
>
> Look at other bindings. There is never code like this.
cdns,i3c-master.yaml taken as reference.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: se-clk
>
> Drop clock-names
Sure, took reference from cdns,i3c-master.yaml.
> > +
> > +  interrupts-extended:
> > +    minItems: 1
> > +    maxItems: 3
>
> As well - there is never an interrupts-extended property. Just interrupts.
No, i see this property many places. Do you mean to say 
interrupts-extended  can be there in examples but not only add 
"interrupts" property here ?
e.g timer/riscv,timer.yaml +41 lists it in yaml also.
> Also drop constraints and list items.
I will remove minItems and MaxItems, will list Items with description.
> Best regards,
> Krzysztof