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

[Yinbo Zhu]

在 2023/8/8 下午11:05, 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.


Yes, it only 0 or 1, and I will use bool type.

Byte mode is to access by byte, such as gpio3, the base address of the
gpio controller is offset by 3 bytes as the access address of gpio3.

The bit mode is the normal mode that like other platform gpio and it is
to access by bit.

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


Okay, I will remove it in required paragraph.

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

Okay, I got it,  and if I understand correctly, you seem to agree with
me adding attributes like this.

And, if using this method that programming model into dts properites,
then when adding a new platform's GPIO,  there is no longer a need to
modify the driver because gpio controller is compatible and different
platform can use a same compatible.

> Where is your DTS?


Sorry, the dts containing gpio nodes are only available in the product
code and have not been sent to the community yet.

2k500, 2k2000, and 3a5000's gpio dts node have been listed in v2, which
gpio dts node will be added in upstream dts.


Thanks,
Yinbo.