Re: [PATCH v3 1/2] gpio: dt-bindings: add parsing of loongson gpio offset

[Krzysztof Kozlowski]

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