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

Krzysztof Kozlowski
2025-05-06T17:48:59Z

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@nxp.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@nxp.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.

>>
>>> +
>>> + 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.

>
>>
>>> + 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.

>
> Ioana

Best regards,
Krzysztof