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

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]

Date:   Wed, 9 Aug 2023 15:28:39 +0800
From:   Yinbo Zhu <zhuyinbo@...ngson.cn>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-gpio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Jianmin Lv <lvjianmin@...ngson.cn>, wanghongliang@...ngson.cn,
        loongson-kernel@...ts.loongnix.cn, zhuyinbo@...ngson.cn
Subject: Re: [PATCH v3 1/2] gpio: dt-bindings: add parsing of loongson gpio
 offset



在 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@...ngson.cn>
>> ---
>>   .../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@...00500 {
>> -      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.