On 07/08/2023 09:40, Yinbo Zhu wrote:
> Loongson GPIO controllers come in multiple variants that are compatible
> except for certain register offset values. Add support in yaml file for
> device properties allowing to specify them in DT.
>
> Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
> ---
> .../bindings/gpio/loongson,ls-gpio.yaml | 40 ++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml b/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
> index fb86e8ce6349..fc51cf40fccd 100644
> --- a/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
> +++ b/Documentation/devicetree/bindings/gpio/loongson,ls-gpio.yaml
> @@ -14,6 +14,7 @@ properties:
> enum:
> - loongson,ls2k-gpio
> - loongson,ls7a-gpio
> + - loongson,ls2k1000-gpio
>
> reg:
> maxItems: 1
> @@ -29,6 +30,33 @@ properties:
>
> gpio-ranges: true
>
> + loongson,gpio-conf-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + This option indicate this GPIO configuration register offset address.
> +
> + loongson,gpio-out-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + This option indicate this GPIO output register offset address.
> +
> + loongson,gpio-in-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + This option indicate this GPIO input register offset address.
> +
> + loongson,gpio-ctrl-mode:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + This option indicate this GPIO control mode, where '0' represents
> + bit control mode and '1' represents byte control mode.
I have no clue what does it mean. Is it only 0 or 1? Then it should be
enum or even bool.
> +
> + loongson,gpio-inten-offset:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + This option indicate this GPIO interrupt enable register offset
> + address.
> +
> interrupts:
> minItems: 1
> maxItems: 64
> @@ -39,6 +67,11 @@ required:
> - ngpios
> - "#gpio-cells"
> - gpio-controller
> + - loongson,gpio-conf-offset
> + - loongson,gpio-in-offset
> + - loongson,gpio-out-offset
> + - loongson,gpio-ctrl-mode
> + - loongson,gpio-inten-offset
No, you cannot add them as required to every other device. First, there
is no single need. Second, it breaks the ABI.
> - gpio-ranges
> - interrupts
>
> @@ -49,11 +82,16 @@ examples:
> #include <dt-bindings/interrupt-controller/irq.h>
>
> gpio0: gpio@1fe00500 {
> - compatible = "loongson,ls2k-gpio";
> + compatible = "loongson,ls2k1000-gpio";
> reg = <0x1fe00500 0x38>;
> ngpios = <64>;
> #gpio-cells = <2>;
> gpio-controller;
> + loongson,gpio-conf-offset = <0>;
> + loongson,gpio-in-offset = <0x20>;
> + loongson,gpio-out-offset = <0x10>;
> + loongson,gpio-ctrl-mode = <0>;
> + loongson,gpio-inten-offset = <0x30>;
I still think that you just embed the programming model into properties,
instead of using dedicated compatible for different blocks. It could be
fine, although I would prefer to check it with your DTS.
Where is your DTS?
Best regards,
Krzysztof
