| Message ID | 20250205143109.2955321-2-quic_msavaliy@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Qualcomm i3c master controller driver support | expand |
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. 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. > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > .../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. > @@ -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@quicinc.com> > + > +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. > + > + reg: > + minItems: 1 Drop > + maxItems: 2 Drop and instead list and describe items > + > + clocks: > + minItems: 1 Look at other bindings. There is never code like this. > + > + clock-names: > + items: > + - const: se-clk Drop clock-names > + > + interrupts-extended: > + minItems: 1 > + maxItems: 3 As well - there is never an interrupts-extended property. Just interrupts. Also drop constraints and list items. Best regards, Krzysztof
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@quicinc.com> >> --- >> .../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@quicinc.com> >> + >> +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
Hi Krzysztof, On 2/6/2025 7:13 PM, Mukesh Kumar Savaliya wrote: > 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@quicinc.com> >>> --- >>> .../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@quicinc.com> >>> + >>> +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. >> Wanted to check if below way is fine ? Because some of the ask to drop constraints are already present in other i3c yaml files. if not, could you please help share example removing constraints and listing item ? == Sample == properties: compatible: enum: - qcom,i3c-controller reg: type: array minItems: 1 maxItems: 1 items: type: integer description: Base address and size of the I3C controller registers. interrupts: type: array minItems: 1 maxItems: 1 items: type: integer description: Interrupt number for the I3C controller. clocks: type: array minItems: 1 maxItems: 1 items: type: object description: Clock specifier for the I3C controller. === >> >> Best regards, >> Krzysztof > >
On 06/02/2025 14:43, Mukesh Kumar Savaliya wrote: > 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@quicinc.com> >>> --- >>> .../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@quicinc.com> >>> + >>> +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. No, it does not matter. Keep const. > > SoC name is not required, as this compatible is generic to all the SOCs. That's the statement you make. I accept it. I will bookmark this thread and use it whenever you try to add any future property here (to be clear: you agree you will not add new properties to fulfill *FUTURE* SoC differences). >>> + >>> + 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 I already wrote what you should do: "Just interrupts." > "interrupts" property here ? > e.g timer/riscv,timer.yaml +41 lists it in yaml also. Please do not take one file and create coding style out of it, but instead look what ~2300 other YAML files do. Best regards, Krzysztof
On 07/02/2025 13:03, Mukesh Kumar Savaliya wrote: >> I will remove minItems and MaxItems, will list Items with description. >>> > Wanted to check if below way is fine ? Because some of the ask to drop > constraints are already present in other i3c yaml files. > > if not, could you please help share example removing constraints and > listing item ? > > == Sample == > properties: > compatible: > enum: > - qcom,i3c-controller > > reg: > type: array > minItems: 1 > maxItems: 1 > items: > type: integer > description: Base address and size of the I3C controller registers. I really do not understand this sample. That's not a DT schema code. Please open existing Qcom bindings and look how it is done. Best regards, Krzysztof
On 2/9/2025 5:16 PM, Krzysztof Kozlowski wrote: > On 07/02/2025 13:03, Mukesh Kumar Savaliya wrote: >>> I will remove minItems and MaxItems, will list Items with description. >>>> >> Wanted to check if below way is fine ? Because some of the ask to drop >> constraints are already present in other i3c yaml files. >> >> if not, could you please help share example removing constraints and >> listing item ? >> >> == Sample == >> properties: >> compatible: >> enum: >> - qcom,i3c-controller >> >> reg: >> type: array >> minItems: 1 >> maxItems: 1 >> items: >> type: integer >> description: Base address and size of the I3C controller registers. > > > I really do not understand this sample. That's not a DT schema code. > Please open existing Qcom bindings and look how it is done. > Added changes looking at existing i2c and i3c dt-bindings. Please help review latest. > Best regards, > Krzysztof
Thanks Krzysztof ! On 2/9/2025 5:15 PM, Krzysztof Kozlowski wrote: > On 06/02/2025 14:43, Mukesh Kumar Savaliya wrote: >> 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@quicinc.com> >>>> --- >>>> .../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@quicinc.com> >>>> + >>>> +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. > > No, it does not matter. Keep const. > Sure. I reviewed other files and seems i should write as below. Please help confirm. compatible: items: - enum: - qcom,sm8550-i3c-master - const: qcom,i3c-master >> >> SoC name is not required, as this compatible is generic to all the SOCs. > > That's the statement you make. I accept it. I will bookmark this thread > and use it whenever you try to add any future property here (to be > clear: you agree you will not add new properties to fulfill *FUTURE* SoC > differences). > Sorry, i am not saying there won't be any other compatible but i was saying base driver will use "qcom,i3c-master". After checking other files i realized there can be const compatible but other SOC specific can be added as enum. Hope above given way is fine. >>>> + >>>> + 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. >>> To confirm, i should drop complete clock-names and items too right ? >>>> + >>>> + 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 > > I already wrote what you should do: > > "Just interrupts." > Sure, Changed interrupts-extended to interrupts. Done. >> "interrupts" property here ? >> e.g timer/riscv,timer.yaml +41 lists it in yaml also. > > > Please do not take one file and create coding style out of it, but > instead look what ~2300 other YAML files do. > > I tried to look other many files including i2c and added. Please review latest or correct me before v2. > Best regards, > Krzysztof
On Mon, Feb 10, 2025 at 09:42:03PM +0530, Mukesh Kumar Savaliya wrote: > Thanks Krzysztof ! > > On 2/9/2025 5:15 PM, Krzysztof Kozlowski wrote: > > On 06/02/2025 14:43, Mukesh Kumar Savaliya wrote: > > > 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@quicinc.com> > > > > > --- > > > > > .../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@quicinc.com> > > > > > + > > > > > +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. > > > > No, it does not matter. Keep const. > > > Sure. I reviewed other files and seems i should write as below. Please help > confirm. > > compatible: > items: > - enum: > - qcom,sm8550-i3c-master > - const: qcom,i3c-master No, that's even worse. I doubt there is some universal, never changing QCom I3C master. > > > > > > SoC name is not required, as this compatible is generic to all the SOCs. > > > > That's the statement you make. I accept it. I will bookmark this thread > > and use it whenever you try to add any future property here (to be > > clear: you agree you will not add new properties to fulfill *FUTURE* SoC > > differences). > > > Sorry, i am not saying there won't be any other compatible but i was saying > base driver will use "qcom,i3c-master". > After checking other files i realized there can be const compatible but > other SOC specific can be added as enum. Hope above given way is fine. AIUI, "geni" is some firmware based multi-protocol serial i/o controller and we already have other "geni" bindings. So really, it's probably more coupled to firmware versions than SoC versions. If we haven't had problems with per SoC quirks with the other geni bindings, then I think using the same "geni" here is fine. But we won't be happy if we start seeing per SoC quirk properties. Rob
On 11/02/2025 22:39, Rob Herring wrote: > On Mon, Feb 10, 2025 at 09:42:03PM +0530, Mukesh Kumar Savaliya wrote: >> Thanks Krzysztof ! >> >>> >> Sure. I reviewed other files and seems i should write as below. Please help >> confirm. >> >> compatible: >> items: >> - enum: >> - qcom,sm8550-i3c-master >> - const: qcom,i3c-master > > No, that's even worse. I doubt there is some universal, never changing > QCom I3C master. > >>>> >>>> SoC name is not required, as this compatible is generic to all the SOCs. >>> >>> That's the statement you make. I accept it. I will bookmark this thread >>> and use it whenever you try to add any future property here (to be >>> clear: you agree you will not add new properties to fulfill *FUTURE* SoC >>> differences). >>> >> Sorry, i am not saying there won't be any other compatible but i was saying >> base driver will use "qcom,i3c-master". >> After checking other files i realized there can be const compatible but >> other SOC specific can be added as enum. Hope above given way is fine. > > AIUI, "geni" is some firmware based multi-protocol serial i/o controller > and we already have other "geni" bindings. So really, it's probably more > coupled to firmware versions than SoC versions. If we haven't had > problems with per SoC quirks with the other geni bindings, then I think > using the same "geni" here is fine. But we won't be happy if we start > seeing per SoC quirk properties. Qualcomm IP blocks are heavily versioned. Sometimes it is version per SoC (about which you commented enough) but mostly multiple SoCs use same IP block. Therefore maybe this should be simply versioned according to firmware/IP block? Best regards, Krzysztof
Thanks Rob ! On 2/12/2025 3:09 AM, Rob Herring wrote: > On Mon, Feb 10, 2025 at 09:42:03PM +0530, Mukesh Kumar Savaliya wrote: >> Thanks Krzysztof ! >> >> On 2/9/2025 5:15 PM, Krzysztof Kozlowski wrote: >>> On 06/02/2025 14:43, Mukesh Kumar Savaliya wrote: >>>> 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@quicinc.com> >>>>>> --- >>>>>> .../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@quicinc.com> >>>>>> + >>>>>> +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. >>> >>> No, it does not matter. Keep const. >>> >> Sure. I reviewed other files and seems i should write as below. Please help >> confirm. >> >> compatible: >> items: >> - enum: >> - qcom,sm8550-i3c-master >> - const: qcom,i3c-master > > No, that's even worse. I doubt there is some universal, never changing > QCom I3C master. > Agree, it may change. So can i go with "qcom, i3c-master" now instead of going SOC specific name ? The current i2c and spi qcom drivers are also using "qcom, geno-proto". Since other i3c drivers using "vendor, i3c-master", i followed same. Should i use "qcom, geni-i3c" or "qcom, i3c-master". Accordingly i can make next patch. >>>> >>>> SoC name is not required, as this compatible is generic to all the SOCs. >>> >>> That's the statement you make. I accept it. I will bookmark this thread >>> and use it whenever you try to add any future property here (to be >>> clear: you agree you will not add new properties to fulfill *FUTURE* SoC >>> differences). >>> >> Sorry, i am not saying there won't be any other compatible but i was saying >> base driver will use "qcom,i3c-master". >> After checking other files i realized there can be const compatible but >> other SOC specific can be added as enum. Hope above given way is fine. > > AIUI, "geni" is some firmware based multi-protocol serial i/o controller > and we already have other "geni" bindings. So really, it's probably more > coupled to firmware versions than SoC versions. If we haven't had > problems with per SoC quirks with the other geni bindings, then I think > using the same "geni" here is fine. But we won't be happy if we start > seeing per SoC quirk properties. > Yes, was trying to follow same like geni based i2c, spi. SOC specific can come, but that would be kind of variant which may depend on QUP GENI HW version or FW version. > Rob
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 @@ -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@quicinc.com> + +allOf: + - $ref: i3c.yaml# + +properties: + compatible: + const: qcom,geni-i3c + + reg: + minItems: 1 + maxItems: 2 + + clocks: + minItems: 1 + + clock-names: + items: + - const: se-clk + + interrupts-extended: + minItems: 1 + maxItems: 3 + +required: + - compatible + - reg + - clocks + - clock-names + - interrupts-extended + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/qcom,sm8550-gcc.h> + + i3c@884000 { + compatible = "qcom,geni-i3c"; + reg = <0x00884000 0x4000>; + clocks = <&gcc GCC_QUPV3_WRAP2_S1_CLK>; + clock-names = "se-clk"; + interrupts-extended = <&intc GIC_SPI 583 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <3>; + #size-cells = <0>; + }; +...
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. Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> --- .../bindings/i3c/qcom,i3c-master.yaml | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/i3c/qcom,i3c-master.yaml