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

  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@loongson.cn>
---
 .../bindings/gpio/loongson,ls-gpio.yaml       | 40 ++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)
  

Hey,

On Mon, Aug 07, 2023 at 03:40:42PM +0800, 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@loongson.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

If you're adding new compatibles that depend on the new offset
properties to function, they could be set up with the existing
"ls2k-gpio" as a fallback, so that further driver changes are not
required when you add ones for the 2k500 etc.

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

How is one supposed to know which of these modes to use?

> +  loongson,gpio-inten-offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      This option indicate this GPIO interrupt enable register offset
> +      address.
> +

tbh, I want to leave the final say on this stuff to Krzysztof or Rob.
I'm not really sure what the best way to do to support your GPIO
controllers is & I don't understand your hardware sufficiently to come
up with an approach that I would use had I been in your shoes.

Thanks,
Conor.

>    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
>    - 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>;
>        gpio-ranges = <&pctrl 0 0 15>,
>                      <&pctrl 16 16 15>,
>                      <&pctrl 32 32 10>,
> -- 
> 2.20.1
>
  

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

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

在 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@loongson.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@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.
  

在 2023/8/8 下午8:05, Conor Dooley 写道:
> Hey,
> 
> On Mon, Aug 07, 2023 at 03:40:42PM +0800, 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@loongson.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
> 
> If you're adding new compatibles that depend on the new offset
> properties to function, they could be set up with the existing
> "ls2k-gpio" as a fallback, so that further driver changes are not
> required when you add ones for the 2k500 etc.


okay, I got it.

> 
>>   
>>     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.
> 
> How is one supposed to know which of these modes to use?


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.

If both modes are supported, it is recommended to prioritize using byte
mode that according to spec.

> 
>> +  loongson,gpio-inten-offset:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      This option indicate this GPIO interrupt enable register offset
>> +      address.
>> +
> 
> tbh, I want to leave the final say on this stuff to Krzysztof or Rob.
> I'm not really sure what the best way to do to support your GPIO
> controllers is & I don't understand your hardware sufficiently to come
> up with an approach that I would use had I been in your shoes.
> 


okay, I got it.

Thanks,
Yinbo

> 
>>     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
>>     - 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>;
>>         gpio-ranges = <&pctrl 0 0 15>,
>>                       <&pctrl 16 16 15>,
>>                       <&pctrl 32 32 10>,
>> -- 
>> 2.20.1
>>
  

On 09/08/2023 09:28, Yinbo Zhu wrote:
>>
>>>     - 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.

Uhu, so there we are. You use this method now to avoid new compatibles.
No, therefore I do not agree.

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

Does not help to convince us, but it is your right. With this and above
explanation, my answer is no - NAK.

Best regards,
Krzysztof
  

On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote:
> 在 2023/8/8 下午8:05, Conor Dooley 写道:
> > On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote:

> > > +  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.
> > 
> > How is one supposed to know which of these modes to use?
> 
> 
> 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.
> 
> If both modes are supported, it is recommended to prioritize using byte
> mode that according to spec.

So, sounds like this property should instead be a boolean that notes
whether the hardware supports the mode or not, rather than the current
enum used to determine software policy.

However, from Krzysztof's comments & my own feeling, it really does seem
like you should drop the do-everything compatible and introduce things
that are soc-specific.
  

在 2023/8/9 下午9:00, Krzysztof Kozlowski 写道:
> On 09/08/2023 09:28, Yinbo Zhu wrote:
>>>
>>>>      - 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.
> 
> Uhu, so there we are. You use this method now to avoid new compatibles.
> No, therefore I do not agree.


I don't seem to got it, if the GPIO controllers of two platforms are
compatible, shouldn't they use the 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.
> 
> Does not help to convince us, but it is your right. With this and above
> explanation, my answer is no - NAK.


The community work for DTS on the 2K platform is still ongoing. Do I
need to add a GPIO DTS node based on the following DTS to request your
review?  so that you can more conveniently review whether my patch is
suitable.

2k1000
https://lore.kernel.org/all/99bdbfc66604b4700e3e22e28c3d27ef7c9c9af7.1686882123.git.zhoubinbin@loongson.cn/

2k500
https://lore.kernel.org/all/c7087046a725e7a2cfde788185112c150e216f1b.1686882123.git.zhoubinbin@loongson.cn/

2k2000
https://lore.kernel.org/all/977009099c38177c384fca5a0ee77ebbe50e3ea2.1686882123.git.zhoubinbin@loongson.cn/


Thanks,
Yinbo
  

On 10/08/2023 05:35, Yinbo Zhu wrote:
> 
> 
> 在 2023/8/9 下午9:00, Krzysztof Kozlowski 写道:
>> On 09/08/2023 09:28, Yinbo Zhu wrote:
>>>>
>>>>>      - 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.
>>
>> Uhu, so there we are. You use this method now to avoid new compatibles.
>> No, therefore I do not agree.
> 
> 
> I don't seem to got it, if the GPIO controllers of two platforms are
> compatible, shouldn't they use the same compatible?

They can use the same fallback compatible, but you should have specific
compatible anyway. However they are not compatible, because programming
model is different.-

> 
>>
>>>
>>>>
>>>> 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.
>>
>> Does not help to convince us, but it is your right. With this and above
>> explanation, my answer is no - NAK.
> 
> 
> The community work for DTS on the 2K platform is still ongoing. Do I
> need to add a GPIO DTS node based on the following DTS to request your
> review?  so that you can more conveniently review whether my patch is
> suitable.
> 
> 2k1000
> https://lore.kernel.org/all/99bdbfc66604b4700e3e22e28c3d27ef7c9c9af7.1686882123.git.zhoubinbin@loongson.cn/
> 
> 2k500
> https://lore.kernel.org/all/c7087046a725e7a2cfde788185112c150e216f1b.1686882123.git.zhoubinbin@loongson.cn/
> 
> 2k2000
> https://lore.kernel.org/all/977009099c38177c384fca5a0ee77ebbe50e3ea2.1686882123.git.zhoubinbin@loongson.cn/

If you want to convince us that your properties makes sense, adding GPIO
nodes there would be helpful.

Best regards,
Krzysztof
  

在 2023/8/9 下午11:39, Conor Dooley 写道:
> On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote:
>> 在 2023/8/8 下午8:05, Conor Dooley 写道:
>>> On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote:
> 
>>>> +  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.
>>>
>>> How is one supposed to know which of these modes to use?
>>
>>
>> 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.
>>
>> If both modes are supported, it is recommended to prioritize using byte
>> mode that according to spec.
> 
> So, sounds like this property should instead be a boolean that notes
> whether the hardware supports the mode or not, rather than the current
> enum used to determine software policy.


okay, I got it, I will use boolean,

Thanks,
Yinbo.
  

On Thu, Aug 10, 2023 at 8:19 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>
>
>
> 在 2023/8/9 下午11:39, Conor Dooley 写道:
> > On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote:
> >> 在 2023/8/8 下午8:05, Conor Dooley 写道:
> >>> On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote:
> >
> >>>> +  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.
> >>>
> >>> How is one supposed to know which of these modes to use?
> >>
> >>
> >> 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.
> >>
> >> If both modes are supported, it is recommended to prioritize using byte
> >> mode that according to spec.
> >
> > So, sounds like this property should instead be a boolean that notes
> > whether the hardware supports the mode or not, rather than the current
> > enum used to determine software policy.
>
>
> okay, I got it, I will use boolean,
>

Why do you want to put it into device-tree so badly? This is not the
first driver that would have of_match_data for different variants
where you can have a structure that would keep offsets for different
models. It's not like you will have hundreds of "compatible" chips
anyway, most likely just a few?

Bart
  

在 2023/8/11 下午10:25, Bartosz Golaszewski 写道:
> On Thu, Aug 10, 2023 at 8:19 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>>
>>
>>
>> 在 2023/8/9 下午11:39, Conor Dooley 写道:
>>> On Wed, Aug 09, 2023 at 03:47:55PM +0800, Yinbo Zhu wrote:
>>>> 在 2023/8/8 下午8:05, Conor Dooley 写道:
>>>>> On Mon, Aug 07, 2023 at 03:40:42PM +0800, Yinbo Zhu wrote:
>>>
>>>>>> +  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.
>>>>>
>>>>> How is one supposed to know which of these modes to use?
>>>>
>>>>
>>>> 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.
>>>>
>>>> If both modes are supported, it is recommended to prioritize using byte
>>>> mode that according to spec.
>>>
>>> So, sounds like this property should instead be a boolean that notes
>>> whether the hardware supports the mode or not, rather than the current
>>> enum used to determine software policy.
>>
>>
>> okay, I got it, I will use boolean,
>>
> 
> Why do you want to put it into device-tree so badly? This is not the
> first driver that would have of_match_data for different variants
> where you can have a structure that would keep offsets for different
> models. It's not like you will have hundreds of "compatible" chips
> anyway, most likely just a few?


Using this ways that put offset property into device-tree that can be
compatible with future GPIO chips without the need to modify drivers,
such as more 2K chips in the future, but use of_match_data and data
field of_device_id, which every time a new SoC is released, the GPIO
driver needs to be modified once, which is not friendly to us.

Thanks,
Yinbo
  

在 2023/8/15 下午4:59, Linus Walleij 写道:
> On Mon, Aug 14, 2023 at 5:39 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
>>> Why do you want to put it into device-tree so badly? This is not the
>>> first driver that would have of_match_data for different variants
>>> where you can have a structure that would keep offsets for different
>>> models. It's not like you will have hundreds of "compatible" chips
>>> anyway, most likely just a few?
>>
>> Using this ways that put offset property into device-tree that can be
>> compatible with future GPIO chips without the need to modify drivers,
>> such as more 2K chips in the future, but use of_match_data and data
>> field of_device_id, which every time a new SoC is released, the GPIO
>> driver needs to be modified once, which is not friendly to us.
> 
> The purpose of device tree is to describe the hardware and
> to configure it for a target system.
> 
> The purpose of device tree is not to make driver writing easy
> or convenient. It often does, but that is not the purpose.
> 
> These offsets are not relevant to the people that need to
> author and maintain device trees to support and tailor their
> system. They are only relevant to driver authors and SoC
> manufacturers.
> 
> What about just writing these offsets into the driver and use
> the compatible match to look them up.
> 


okay, I got it, I had following your advice and send v4 to upstream,
please you help review it.

Thanks,
Yinbo