linux-kernel - Re: [PATCH 1/6] dt-bindings: mfd: add bindings for QIXIS CPLD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <miuxxgv73fmsl5plcoso73dk6bnuwgmlydzupnb7fcz6ub72ra@dro4cqbn67jt>
Date: Tue, 6 May 2025 17:16:13 +0300
From: Ioana Ciornei <ioana.ciornei@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Shawn Guo <shawnguo@...nel.org>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/6] dt-bindings: mfd: add bindings for QIXIS CPLD

On Tue, May 06, 2025 at 04:05:46PM +0200, Krzysztof Kozlowski wrote:
> On 06/05/2025 15:57, Ioana Ciornei wrote:
> > On Fri, May 02, 2025 at 09:01:59AM +0200, Krzysztof Kozlowski wrote:
> >> On Wed, Apr 30, 2025 at 06:36:29PM GMT, Ioana Ciornei wrote:
> >>> This adds device tree bindings for the board management controller -
> >>> QIXIS CPLD - found on some Layerscape based boards such as LX2160A-RDB,
> >>> LX2160AQDS, LS1028AQDS etc.
> >>>
> >>> Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> >>> ---
> >>>  .../bindings/mfd/fsl,qixis-i2c.yaml           | 65 +++++++++++++++++++
> >>>  1 file changed, 65 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mfd/fsl,qixis-i2c.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mfd/fsl,qixis-i2c.yaml b/Documentation/devicetree/bindings/mfd/fsl,qixis-i2c.yaml
> >>> new file mode 100644
> >>> index 000000000000..562878050916
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/fsl,qixis-i2c.yaml
> >>
> >> Filename matching compatible.
> > 
> > How to choose one if there are multiple compatible strings?
> 
> The fallback or the oldest or the lowest number or whichever you prefer
> as a base.
> 
> > 
> >>
> >>> @@ -0,0 +1,65 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mfd/fsl,qixis-i2c.yaml
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml
> >>> +
> >>> +title: NXP's QIXIS CPLD board management controller
> >>> +
> >>> +maintainers:
> >>> +  - Ioana Ciornei <ioana.ciornei@....com>
> >>> +
> >>> +description: |
> >>> +  The board management controller found on some Layerscape boards contains
> >>> +  different IP blocks like GPIO controllers, interrupt controllers, reg-muxes
> >>> +  etc.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - fsl,lx2160a-qds-qixis-i2c
> >>> +      - fsl,lx2162a-qds-qixis-i2c
> >>> +      - fsl,ls1028a-qds-qixis-i2c
> >>
> >> Keep alphabetical order.
> >>
> >> What is actual device name? I2C? Is this an I2C controller or device?
> 
> I assume you will then drop the redundant part.

Ok, I will drop the i2c part. Are you ok with the below compatible
strings?

	- fsl,lx2160a-qds-qixis-cpld
	- fsl,lx2162a-qds-qixis-cpld
	- fsl,ls1028a-qds-qixis-cpld

> 
> >>
> >>> +
> >>> +  reg:
> >>> +    description:
> >>> +      I2C device address.
> >>
> >> This says device, so i2c in compatible is wrong.
> >>
> >> Anyway drop description, redundant.
> > 
> > Ok, will drop.
> > 
> >>
> >>
> >>> +    maxItems: 1
> >>> +
> >>> +  "#address-cells":
> >>> +    const: 1
> >>
> >> Why?
> >>
> >>> +
> >>> +  "#size-cells":
> >>> +    const: 0
> >>
> >> Why? Drop cells.
> >>
> > 
> > See below.
> > 
> >>> +
> >>> +  mux-controller:
> >>> +    $ref: /schemas/mux/reg-mux.yaml#
> >>> +
> >>> +required:
> >>> +  - "#address-cells"
> >>> +  - "#size-cells"
> >>> +  - compatible
> >>> +  - reg
> >>
> >> Keep same order as in properties
> > 
> > Ok.
> > 
> >>
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    i2c {
> >>> +        #address-cells = <1>;
> >>> +        #size-cells = <0>;
> >>> +
> >>> +        qixis@66 {
> >>
> >> Node names should be generic. See also an explanation and list of
> >> examples (not exhaustive) in DT specification:
> >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> > 
> > In this case, an accepted node name is 'cpld'?
> 
> If this is CPLD then yes.

Yes, it is. Thanks for the confirmation.

> 
> > 
> >>
> >>> +            compatible = "fsl,lx2160a-qds-qixis-i2c";
> >>> +            reg = <0x66>;
> >>> +            #address-cells = <1>;
> >>> +            #size-cells = <0>;
> >>
> >> So were do you use address/size cells?
> >>
> > 
> > For example, fsl-ls1028a-qds.dts looks like this:
> > 
> > 	fpga@66 {
> > 		compatible = "fsl,ls1028a-qds-qixis-i2c";
> > 		reg = <0x66>;
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		mux: mux-controller@54 {
> > 			compatible = "reg-mux";
> > 			reg = <0x54>;
> > 			#mux-control-cells = <1>;
> > 			mux-reg-masks = <0x54 0xf0>; /* 0: reg 0x54, bits 7:4 */
> > 		};
> > 	};
> > 
> > Also, some boards have in their qixis CPLD gpio controllers and I am
> > planning to add them as the next step.
> 
> And if you tested that DTS you would see that binding does not work
> well... so my arguments stay valid - these properties in current binding
> make no sense. However binding is just wrong, so maybe these properties
> make sense after fixing the binding but then in both cases: current
> stage is not correct.
> 

Yes, I agree that the current binding file is wrong. I even said this in
one of my ealier replies to the bot which found the new DTB warnings.
I will fix it in v2.

Ioana