[PATCH v3 0/2] gpio: loongson: add firmware offset parse support

[PATCH v3 0/2] gpio: loongson: add firmware offset parse support

[Yinbo Zhu]

Add some GPIO register offset value parse support for device properties
allowing to specify them in ACPI or DT.

Change in v3:
		1. Reword the dt-bindings patch commit log information.
		2. Add "loongson,ls2k1000-gpio" compatible.
Change in v2:
		1. Reword the patch commit log information.
		2. Add some GPIO register offset description in yaml.

Yinbo Zhu (2):
  gpio: dt-bindings: add parsing of loongson gpio offset
  gpio: loongson: add firmware offset parse support

 .../bindings/gpio/loongson,ls-gpio.yaml       | 40 +++++++++-
 drivers/gpio/gpio-loongson-64bit.c            | 74 +++++++++++++++++--
 2 files changed, 106 insertions(+), 8 deletions(-)

-- 
2.20.1

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

[Yinbo Zhu]

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

[PATCH v3 2/2] gpio: loongson: add firmware offset parse support

[Yinbo Zhu]

Loongson GPIO controllers come in multiple variants that are compatible
except for certain register offset values.  Add support for device
properties allowing to specify them in ACPI or DT.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 drivers/gpio/gpio-loongson-64bit.c | 74 +++++++++++++++++++++++++++---
 1 file changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-loongson-64bit.c b/drivers/gpio/gpio-loongson-64bit.c
index 06213bbfabdd..51bcd6e8660f 100644
--- a/drivers/gpio/gpio-loongson-64bit.c
+++ b/drivers/gpio/gpio-loongson-64bit.c
@@ -26,6 +26,7 @@ struct loongson_gpio_chip_data {
 	unsigned int		conf_offset;
 	unsigned int		out_offset;
 	unsigned int		in_offset;
+	unsigned int		inten_offset;
 };
 
 struct loongson_gpio_chip {
@@ -117,7 +118,17 @@ static void loongson_gpio_set(struct gpio_chip *chip, unsigned int pin, int valu
 
 static int loongson_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
 {
+	unsigned int u;
 	struct platform_device *pdev = to_platform_device(chip->parent);
+	struct loongson_gpio_chip *lgpio = to_loongson_gpio_chip(chip);
+
+	if (lgpio->chip_data->mode == BIT_CTRL_MODE) {
+		u = readl(lgpio->reg_base + lgpio->chip_data->inten_offset + offset / 32 * 4);
+		u |= BIT(offset % 32);
+		writel(u, lgpio->reg_base + lgpio->chip_data->inten_offset + offset / 32 * 4);
+	} else {
+		writeb(1, lgpio->reg_base + lgpio->chip_data->inten_offset + offset);
+	}
 
 	return platform_get_irq(pdev, offset);
 }
@@ -127,11 +138,30 @@ static int loongson_gpio_init(struct device *dev, struct loongson_gpio_chip *lgp
 {
 	int ret;
 	u32 ngpios;
+	unsigned int io_width;
 
 	lgpio->reg_base = reg_base;
+	if (device_property_read_u32(dev, "ngpios", &ngpios) || !ngpios)
+		return -EINVAL;
+
+	ret = DIV_ROUND_UP(ngpios, 8);
+	switch (ret) {
+	case 1 ... 2:
+		io_width = ret;
+		break;
+	case 3 ... 4:
+		io_width = 0x4;
+		break;
+	case 5 ... 8:
+		io_width = 0x8;
+		break;
+	default:
+		dev_err(dev, "unsupported io width\n");
+		return -EINVAL;
+	}
 
 	if (lgpio->chip_data->mode == BIT_CTRL_MODE) {
-		ret = bgpio_init(&lgpio->chip, dev, 8,
+		ret = bgpio_init(&lgpio->chip, dev, io_width,
 				lgpio->reg_base + lgpio->chip_data->in_offset,
 				lgpio->reg_base + lgpio->chip_data->out_offset,
 				NULL, NULL,
@@ -151,16 +181,35 @@ static int loongson_gpio_init(struct device *dev, struct loongson_gpio_chip *lgp
 		spin_lock_init(&lgpio->lock);
 	}
 
-	device_property_read_u32(dev, "ngpios", &ngpios);
-
-	lgpio->chip.can_sleep = 0;
 	lgpio->chip.ngpio = ngpios;
-	lgpio->chip.label = lgpio->chip_data->label;
-	lgpio->chip.to_irq = loongson_gpio_to_irq;
+	lgpio->chip.can_sleep = 0;
+	if (lgpio->chip_data->label)
+		lgpio->chip.label = lgpio->chip_data->label;
+	else
+		lgpio->chip.label = kstrdup(to_platform_device(dev)->name, GFP_KERNEL);
+
+	if (lgpio->chip_data->inten_offset)
+		lgpio->chip.to_irq = loongson_gpio_to_irq;
 
 	return devm_gpiochip_add_data(dev, &lgpio->chip, lgpio);
 }
 
+static int loongson_gpio_get_props(struct device *dev,
+				    struct loongson_gpio_chip *lgpio)
+{
+	const struct loongson_gpio_chip_data *d = lgpio->chip_data;
+
+	if (device_property_read_u32(dev, "loongson,gpio-conf-offset", (u32 *)&d->conf_offset)
+	    || device_property_read_u32(dev, "loongson,gpio-in-offset", (u32 *)&d->in_offset)
+	    || device_property_read_u32(dev, "loongson,gpio-out-offset", (u32 *)&d->out_offset)
+	    || device_property_read_u32(dev, "loongson,gpio-ctrl-mode", (u32 *)&d->mode))
+		return -EINVAL;
+
+	device_property_read_u32(dev, "loongson,gpio-inten-offset", (u32 *)&d->inten_offset);
+
+	return 0;
+}
+
 static int loongson_gpio_probe(struct platform_device *pdev)
 {
 	void __iomem *reg_base;
@@ -172,7 +221,12 @@ static int loongson_gpio_probe(struct platform_device *pdev)
 	if (!lgpio)
 		return -ENOMEM;
 
-	lgpio->chip_data = device_get_match_data(dev);
+	lgpio->chip_data = devm_kzalloc(dev, sizeof(*lgpio->chip_data), GFP_KERNEL);
+	if (!lgpio->chip_data)
+		return -ENOMEM;
+
+	if (loongson_gpio_get_props(dev, lgpio))
+		lgpio->chip_data = device_get_match_data(dev);
 
 	reg_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(reg_base))
@@ -206,6 +260,9 @@ static const struct of_device_id loongson_gpio_of_match[] = {
 		.compatible = "loongson,ls7a-gpio",
 		.data = &loongson_gpio_ls7a_data,
 	},
+	{
+		.compatible = "loongson,ls2k1000-gpio",
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, loongson_gpio_of_match);
@@ -215,6 +272,9 @@ static const struct acpi_device_id loongson_gpio_acpi_match[] = {
 		.id = "LOON0002",
 		.driver_data = (kernel_ulong_t)&loongson_gpio_ls7a_data,
 	},
+	{
+		.id = "LOON0007",
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, loongson_gpio_acpi_match);
-- 
2.20.1

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3721 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

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

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

[Yinbo Zhu]

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

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

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

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

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Yinbo Zhu]

在 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

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

[Krzysztof Kozlowski]

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

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

[Yinbo Zhu]

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

Re: [PATCH v3 2/2] gpio: loongson: add firmware offset parse support

[Linus Walleij]

Hi Yinbo,

thanks for your patch!

On Mon, Aug 7, 2023 at 9:41 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:

> Loongson GPIO controllers come in multiple variants that are compatible
> except for certain register offset values.  Add support for device
> properties allowing to specify them in ACPI or DT.
>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>

(...)
> @@ -26,6 +26,7 @@ struct loongson_gpio_chip_data {
>         unsigned int            conf_offset;
>         unsigned int            out_offset;
>         unsigned int            in_offset;
> +       unsigned int            inten_offset;

Consider just changing all of these from unsigned int to u32.

(...)
> +       if (device_property_read_u32(dev, "loongson,gpio-conf-offset", (u32 *)&d->conf_offset)
> +           || device_property_read_u32(dev, "loongson,gpio-in-offset", (u32 *)&d->in_offset)
> +           || device_property_read_u32(dev, "loongson,gpio-out-offset", (u32 *)&d->out_offset)
> +           || device_property_read_u32(dev, "loongson,gpio-ctrl-mode", (u32 *)&d->mode))

Because then you can get rid of this annoying forest of cast.

I'm fine with doing this change in this patch without a need for a separate
refactoring, as it's just a contained driver and clearly just about typing.

Yours,
Linus Walleij

Re: [PATCH v3 2/2] gpio: loongson: add firmware offset parse support

[Yinbo Zhu]

在 2023/8/10 下午4:27, Linus Walleij 写道:
> Hi Yinbo,
> 
> thanks for your patch!
> 
> On Mon, Aug 7, 2023 at 9:41 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
>> Loongson GPIO controllers come in multiple variants that are compatible
>> except for certain register offset values.  Add support for device
>> properties allowing to specify them in ACPI or DT.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> 
> (...)
>> @@ -26,6 +26,7 @@ struct loongson_gpio_chip_data {
>>          unsigned int            conf_offset;
>>          unsigned int            out_offset;
>>          unsigned int            in_offset;
>> +       unsigned int            inten_offset;
> 
> Consider just changing all of these from unsigned int to u32.


okay, I got it.

> 
> (...)
>> +       if (device_property_read_u32(dev, "loongson,gpio-conf-offset", (u32 *)&d->conf_offset)
>> +           || device_property_read_u32(dev, "loongson,gpio-in-offset", (u32 *)&d->in_offset)
>> +           || device_property_read_u32(dev, "loongson,gpio-out-offset", (u32 *)&d->out_offset)
>> +           || device_property_read_u32(dev, "loongson,gpio-ctrl-mode", (u32 *)&d->mode))
> 
> Because then you can get rid of this annoying forest of cast.


Change offset to u32 and here still need use a (u32 *) cast, because the
chip_data is const type so &chip_data->offset will be (const u32 *) type
and need a (u32 *) cast.

> 
> I'm fine with doing this change in this patch without a need for a separate
> refactoring, as it's just a contained driver and clearly just about typing.


okay, I got it.

Thanks,
Yinbo.

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

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

Bart

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

[Yinbo Zhu]

在 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

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

[Krzysztof Kozlowski]

On 14/08/2023 05:39, Yinbo Zhu wrote:
> 
> 
> 在 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,

That's not an argument for putting into DT.

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

Sorry, "friendly" is again hardly an argument what should or should not
be in DT.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] gpio: loongson: add firmware offset parse support

[andy.shevchenko]

Mon, Aug 07, 2023 at 03:40:43PM +0800, Yinbo Zhu kirjoitti:
> Loongson GPIO controllers come in multiple variants that are compatible
> except for certain register offset values.  Add support for device
> properties allowing to specify them in ACPI or DT.

> +	if (device_property_read_u32(dev, "ngpios", &ngpios) || !ngpios)
> +		return -EINVAL;
> +
> +	ret = DIV_ROUND_UP(ngpios, 8);
> +	switch (ret) {
> +	case 1 ... 2:
> +		io_width = ret;
> +		break;
> +	case 3 ... 4:
> +		io_width = 0x4;
> +		break;
> +	case 5 ... 8:
> +		io_width = 0x8;
> +		break;
> +	default:
> +		dev_err(dev, "unsupported io width\n");
> +		return -EINVAL;
> +	}

Why? We have bgpio_init() handle this.
https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=55b2395e4e92adc492c6b30ac109eb78250dcd9d

...

> +	lgpio->chip.can_sleep = 0;

It's boolean, use boolean initializer.

...

> +	if (lgpio->chip_data->label)
> +		lgpio->chip.label = lgpio->chip_data->label;
> +	else
> +		lgpio->chip.label = kstrdup(to_platform_device(dev)->name, GFP_KERNEL);

No error check? Not a devm_*() variant, so leaking memory?

...

> +	{
> +		.id = "LOON0007",
> +	},

How does DSDT excerpt for this device look like?

-- 
With Best Regards,
Andy Shevchenko

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

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

Yours,
Linus Walleij

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

[Yinbo Zhu]

在 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

Re: [PATCH v3 2/2] gpio: loongson: add firmware offset parse support

[Yinbo Zhu]

Hi andy,

Sorry, I lost this email due to issues with my company's email server. I
will adopt your suggestion in v5 version.

在 2023/8/15 上午5:48, andy.shevchenko@gmail.com 写道:
> Mon, Aug 07, 2023 at 03:40:43PM +0800, Yinbo Zhu kirjoitti:
>> Loongson GPIO controllers come in multiple variants that are compatible
>> except for certain register offset values.  Add support for device
>> properties allowing to specify them in ACPI or DT.
> 
>> +	if (device_property_read_u32(dev, "ngpios", &ngpios) || !ngpios)
>> +		return -EINVAL;
>> +
>> +	ret = DIV_ROUND_UP(ngpios, 8);
>> +	switch (ret) {
>> +	case 1 ... 2:
>> +		io_width = ret;
>> +		break;
>> +	case 3 ... 4:
>> +		io_width = 0x4;
>> +		break;
>> +	case 5 ... 8:
>> +		io_width = 0x8;
>> +		break;
>> +	default:
>> +		dev_err(dev, "unsupported io width\n");
>> +		return -EINVAL;
>> +	}
> 
> Why? We have bgpio_init() handle this.
> https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=55b2395e4e92adc492c6b30ac109eb78250dcd9d


okay, I got it.

> 
> ...
> 
>> +	lgpio->chip.can_sleep = 0;
> 
> It's boolean, use boolean initializer.


okay, I got it.

> 
> ...
> 
>> +	if (lgpio->chip_data->label)
>> +		lgpio->chip.label = lgpio->chip_data->label;
>> +	else
>> +		lgpio->chip.label = kstrdup(to_platform_device(dev)->name, GFP_KERNEL);
> 
> No error check? Not a devm_*() variant, so leaking memory?


This code had been removed in v4.

> 
> ...
> 
>> +	{
>> +		.id = "LOON0007",
>> +	},
> 
> How does DSDT excerpt for this device look like?


LOON0007 and LOON000A are similar, LOON000A is for 2k2000 gpio0.

      Device (GPO0)
         {
             Name (_HID, "LOON000A")  // _HID: Hardware ID
             Name (_ADR, Zero)  // _ADR: Address
             Name (_UID, One)  // _UID: Unique ID
             Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource 
Settings
             {
                 QWordMemory (ResourceConsumer, PosDecode, MinFixed, 
MaxFixed, NonCacheable, ReadWrite,
                     0x0000000000000000, // Granularity
                     0x000000001FE00500, // Range Minimum
                     0x000000001FE00520, // Range Maximum
                     0x0000000000000000, // Translation Offset
                     0x0000000000000021, // Length
                     ,, , AddressRangeMemory, TypeStatic)
             })
             Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
             {
                 ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* 
Device Properties for _DSD */,
                 Package (0x01)
                 {
                     Package (0x02)
                     {
                         "ngpios",
                         0x20
                     }
                 }
             })
         }


Thanks,
Yinbo