[PATCH v2 0/5] Devicetree support for Loongson-1 GPIO

[PATCH v2 0/5] Devicetree support for Loongson-1 GPIO

[Keguang Zhang]

Update the driver to add DT support and dt-binding document,
along with other minor changes.

Keguang Zhang (5):
  gpio: loongson1: Convert to SPDX identifier
  gpio: loongson1: Use readl() & writel()
  gpio: loongson1: Introduce ls1x_gpio_chip struct
  gpio: loongson1: Add DT support
  dt-bindings: gpio: Add Loongson-1 GPIO

 .../bindings/gpio/loongson,ls1x-gpio.yaml     | 49 +++++++++++++
 drivers/gpio/gpio-loongson1.c                 | 71 +++++++++++--------
 2 files changed, 92 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/loongson,ls1x-gpio.yaml


base-commit: 4827aae061337251bb91801b316157a78b845ec7
-- 
2.34.1

[PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Keguang Zhang]

Use SPDX-License-Identifier instead of the license text and
update the author information.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V1 -> V2: Keep GPLv2, just convert to SPDX identifier
---
 drivers/gpio/gpio-loongson1.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
index 5d90b3bc5a25..8862c9ea0d41 100644
--- a/drivers/gpio/gpio-loongson1.c
+++ b/drivers/gpio/gpio-loongson1.c
@@ -1,11 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * GPIO Driver for Loongson 1 SoC
  *
- * Copyright (C) 2015-2016 Zhang, Keguang <keguang.zhang@gmail.com>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
+ * Copyright (C) 2015-2023 Keguang Zhang <keguang.zhang@gmail.com>
  */
 
 #include <linux/module.h>
@@ -90,6 +87,6 @@ static struct platform_driver ls1x_gpio_driver = {
 
 module_platform_driver(ls1x_gpio_driver);
 
-MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@gmail.com>");
+MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
 MODULE_DESCRIPTION("Loongson1 GPIO driver");
 MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v2 2/5] gpio: loongson1: Use readl() & writel()

[Keguang Zhang]

This patch replace __raw_readl() & __raw_writel() with readl() & writel().

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V1 -> V2: Split this change to a separate patch
---
 drivers/gpio/gpio-loongson1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
index 8862c9ea0d41..b6c11caa3ade 100644
--- a/drivers/gpio/gpio-loongson1.c
+++ b/drivers/gpio/gpio-loongson1.c
@@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
-	__raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
-		     gpio_reg_base + GPIO_CFG);
+	writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
+	       gpio_reg_base + GPIO_CFG);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
 	return 0;
@@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
-	__raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
-		     gpio_reg_base + GPIO_CFG);
+	writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
+	       gpio_reg_base + GPIO_CFG);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }
 
-- 
2.34.1

[PATCH v2 3/5] gpio: loongson1: Introduce ls1x_gpio_chip struct

[Keguang Zhang]

This patch introduces and allocates ls1x_gpio_chip struct containing
gpio_chip and reg_base to avoid global gpio_reg_base.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V1 -> V2: Split this change to a separate patch
---
 drivers/gpio/gpio-loongson1.c | 45 +++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
index b6c11caa3ade..3ac9e49e7efb 100644
--- a/drivers/gpio/gpio-loongson1.c
+++ b/drivers/gpio/gpio-loongson1.c
@@ -16,15 +16,19 @@
 #define GPIO_DATA		0x20
 #define GPIO_OUTPUT		0x30
 
-static void __iomem *gpio_reg_base;
+struct ls1x_gpio_chip {
+	struct gpio_chip gc;
+	void __iomem *reg_base;
+};
 
 static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
 {
+	struct ls1x_gpio_chip *ls1x_gc = gpiochip_get_data(gc);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
-	writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
-	       gpio_reg_base + GPIO_CFG);
+	writel(readl(ls1x_gc->reg_base + GPIO_CFG) | BIT(offset),
+	       ls1x_gc->reg_base + GPIO_CFG);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
 	return 0;
@@ -32,44 +36,45 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
 
 static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
 {
+	struct ls1x_gpio_chip *ls1x_gc = gpiochip_get_data(gc);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
-	writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
-	       gpio_reg_base + GPIO_CFG);
+	writel(readl(ls1x_gc->reg_base + GPIO_CFG) & ~BIT(offset),
+	       ls1x_gc->reg_base + GPIO_CFG);
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }
 
 static int ls1x_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct gpio_chip *gc;
+	struct ls1x_gpio_chip *ls1x_gc;
 	int ret;
 
-	gc = devm_kzalloc(dev, sizeof(*gc), GFP_KERNEL);
-	if (!gc)
+	ls1x_gc = devm_kzalloc(dev, sizeof(*ls1x_gc), GFP_KERNEL);
+	if (!ls1x_gc)
 		return -ENOMEM;
 
-	gpio_reg_base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(gpio_reg_base))
-		return PTR_ERR(gpio_reg_base);
+	ls1x_gc->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ls1x_gc->reg_base))
+		return PTR_ERR(ls1x_gc->reg_base);
 
-	ret = bgpio_init(gc, dev, 4, gpio_reg_base + GPIO_DATA,
-			 gpio_reg_base + GPIO_OUTPUT, NULL,
-			 NULL, gpio_reg_base + GPIO_DIR, 0);
+	ret = bgpio_init(&ls1x_gc->gc, dev, 4, ls1x_gc->reg_base + GPIO_DATA,
+			 ls1x_gc->reg_base + GPIO_OUTPUT, NULL,
+			 NULL, ls1x_gc->reg_base + GPIO_DIR, 0);
 	if (ret)
 		goto err;
 
-	gc->owner = THIS_MODULE;
-	gc->request = ls1x_gpio_request;
-	gc->free = ls1x_gpio_free;
-	gc->base = pdev->id * 32;
+	ls1x_gc->gc.owner = THIS_MODULE;
+	ls1x_gc->gc.request = ls1x_gpio_request;
+	ls1x_gc->gc.free = ls1x_gpio_free;
+	ls1x_gc->gc.base = pdev->id * 32;
 
-	ret = devm_gpiochip_add_data(dev, gc, NULL);
+	ret = devm_gpiochip_add_data(dev, &ls1x_gc->gc, ls1x_gc);
 	if (ret)
 		goto err;
 
-	platform_set_drvdata(pdev, gc);
+	platform_set_drvdata(pdev, ls1x_gc);
 	dev_info(dev, "Loongson1 GPIO driver registered\n");
 
 	return 0;
-- 
2.34.1

[PATCH v2 4/5] gpio: loongson1: Add DT support

[Keguang Zhang]

This patch adds DT support for Loongson-1 GPIO driver.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V1 -> V2: Let gpiolib parse ngpios property
          Remove unnecessary alias id parsing
          Remove superfluous initialization done by bgpio_init()
          Add MODULE_DEVICE_TABLE()
          Other minor fixes
---
 drivers/gpio/gpio-loongson1.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
index 3ac9e49e7efb..94ac0ccb450f 100644
--- a/drivers/gpio/gpio-loongson1.c
+++ b/drivers/gpio/gpio-loongson1.c
@@ -68,25 +68,38 @@ static int ls1x_gpio_probe(struct platform_device *pdev)
 	ls1x_gc->gc.owner = THIS_MODULE;
 	ls1x_gc->gc.request = ls1x_gpio_request;
 	ls1x_gc->gc.free = ls1x_gpio_free;
-	ls1x_gc->gc.base = pdev->id * 32;
+	/*
+	 * Clear ngpio to let gpiolib get the correct number
+	 * by reading ngpios property
+	 */
+	ls1x_gc->gc.ngpio = 0;
 
 	ret = devm_gpiochip_add_data(dev, &ls1x_gc->gc, ls1x_gc);
 	if (ret)
 		goto err;
 
 	platform_set_drvdata(pdev, ls1x_gc);
-	dev_info(dev, "Loongson1 GPIO driver registered\n");
+
+	dev_info(dev, "GPIO controller registered with %d pins\n",
+		 ls1x_gc->gc.ngpio);
 
 	return 0;
 err:
-	dev_err(dev, "failed to register GPIO device\n");
+	dev_err(dev, "failed to register GPIO controller\n");
 	return ret;
 }
 
+static const struct of_device_id ls1x_gpio_dt_ids[] = {
+	{ .compatible = "loongson,ls1x-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ls1x_gpio_dt_ids);
+
 static struct platform_driver ls1x_gpio_driver = {
 	.probe	= ls1x_gpio_probe,
 	.driver	= {
 		.name	= "ls1x-gpio",
+		.of_match_table = ls1x_gpio_dt_ids,
 	},
 };
 
-- 
2.34.1

[PATCH v2 5/5] dt-bindings: gpio: Add Loongson-1 GPIO

[Keguang Zhang]

Add devicetree binding document for Loongson-1 GPIO.

Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
---
V1 -> V2: Use the same consistent quotes
          Delete superfluous examples
---
 .../bindings/gpio/loongson,ls1x-gpio.yaml     | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/loongson,ls1x-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/loongson,ls1x-gpio.yaml b/Documentation/devicetree/bindings/gpio/loongson,ls1x-gpio.yaml
new file mode 100644
index 000000000000..1a472c05697c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/loongson,ls1x-gpio.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/loongson,ls1x-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Loongson-1 GPIO controller
+
+maintainers:
+  - Keguang Zhang <keguang.zhang@gmail.com>
+
+properties:
+  compatible:
+    const: loongson,ls1x-gpio
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  ngpios:
+    minimum: 1
+    maximum: 32
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+  - ngpios
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio0: gpio@1fd010c0 {
+        compatible = "loongson,ls1x-gpio";
+        reg = <0x1fd010c0 0x4>;
+
+        gpio-controller;
+        #gpio-cells = <2>;
+
+        ngpios = <32>;
+    };
+
+...
-- 
2.34.1

Re: [PATCH v2 5/5] dt-bindings: gpio: Add Loongson-1 GPIO

[Krzysztof Kozlowski]

On 02/03/2023 13:52, Keguang Zhang wrote:
> Add devicetree binding document for Loongson-1 GPIO.
> 
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V1 -> V2: Use the same consistent quotes
>           Delete superfluous examples
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Bartosz Golaszewski]

On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
>
> Use SPDX-License-Identifier instead of the license text and
> update the author information.
>
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V1 -> V2: Keep GPLv2, just convert to SPDX identifier
> ---
>  drivers/gpio/gpio-loongson1.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> index 5d90b3bc5a25..8862c9ea0d41 100644
> --- a/drivers/gpio/gpio-loongson1.c
> +++ b/drivers/gpio/gpio-loongson1.c
> @@ -1,11 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>  /*
>   * GPIO Driver for Loongson 1 SoC
>   *
> - * Copyright (C) 2015-2016 Zhang, Keguang <keguang.zhang@gmail.com>
> - *
> - * This file is licensed under the terms of the GNU General Public
> - * License version 2. This program is licensed "as is" without any
> - * warranty of any kind, whether express or implied.
> + * Copyright (C) 2015-2023 Keguang Zhang <keguang.zhang@gmail.com>
>   */
>
>  #include <linux/module.h>
> @@ -90,6 +87,6 @@ static struct platform_driver ls1x_gpio_driver = {
>
>  module_platform_driver(ls1x_gpio_driver);
>
> -MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@gmail.com>");

Why are you removing credits of the old author?

Bart

> +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
>  MODULE_DESCRIPTION("Loongson1 GPIO driver");
>  MODULE_LICENSE("GPL");
> --
> 2.34.1
>

Re: [PATCH v2 2/5] gpio: loongson1: Use readl() & writel()

[Bartosz Golaszewski]

On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
>
> This patch replace __raw_readl() & __raw_writel() with readl() & writel().
>

Please say WHY you're doing this.

Bart

> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V1 -> V2: Split this change to a separate patch
> ---
>  drivers/gpio/gpio-loongson1.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> index 8862c9ea0d41..b6c11caa3ade 100644
> --- a/drivers/gpio/gpio-loongson1.c
> +++ b/drivers/gpio/gpio-loongson1.c
> @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> -                    gpio_reg_base + GPIO_CFG);
> +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> +              gpio_reg_base + GPIO_CFG);
>         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
>
>         return 0;
> @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
>         unsigned long flags;
>
>         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> -                    gpio_reg_base + GPIO_CFG);
> +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> +              gpio_reg_base + GPIO_CFG);
>         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
>  }
>
> --
> 2.34.1
>

Re: [PATCH v2 4/5] gpio: loongson1: Add DT support

[Bartosz Golaszewski]

On Thu, Mar 2, 2023 at 1:53 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
>
> This patch adds DT support for Loongson-1 GPIO driver.
>
> Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> ---
> V1 -> V2: Let gpiolib parse ngpios property
>           Remove unnecessary alias id parsing
>           Remove superfluous initialization done by bgpio_init()
>           Add MODULE_DEVICE_TABLE()
>           Other minor fixes
> ---
>  drivers/gpio/gpio-loongson1.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> index 3ac9e49e7efb..94ac0ccb450f 100644
> --- a/drivers/gpio/gpio-loongson1.c
> +++ b/drivers/gpio/gpio-loongson1.c
> @@ -68,25 +68,38 @@ static int ls1x_gpio_probe(struct platform_device *pdev)
>         ls1x_gc->gc.owner = THIS_MODULE;
>         ls1x_gc->gc.request = ls1x_gpio_request;
>         ls1x_gc->gc.free = ls1x_gpio_free;
> -       ls1x_gc->gc.base = pdev->id * 32;
> +       /*
> +        * Clear ngpio to let gpiolib get the correct number
> +        * by reading ngpios property
> +        */
> +       ls1x_gc->gc.ngpio = 0;
>

Who could have set it before and why would this information need to be
unconditionally discarded?

Bart

>         ret = devm_gpiochip_add_data(dev, &ls1x_gc->gc, ls1x_gc);
>         if (ret)
>                 goto err;
>
>         platform_set_drvdata(pdev, ls1x_gc);
> -       dev_info(dev, "Loongson1 GPIO driver registered\n");
> +
> +       dev_info(dev, "GPIO controller registered with %d pins\n",
> +                ls1x_gc->gc.ngpio);
>
>         return 0;
>  err:
> -       dev_err(dev, "failed to register GPIO device\n");
> +       dev_err(dev, "failed to register GPIO controller\n");
>         return ret;
>  }
>
> +static const struct of_device_id ls1x_gpio_dt_ids[] = {
> +       { .compatible = "loongson,ls1x-gpio" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ls1x_gpio_dt_ids);
> +
>  static struct platform_driver ls1x_gpio_driver = {
>         .probe  = ls1x_gpio_probe,
>         .driver = {
>                 .name   = "ls1x-gpio",
> +               .of_match_table = ls1x_gpio_dt_ids,
>         },
>  };
>
> --
> 2.34.1
>

Re: [PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Keguang Zhang]

On Mon, Mar 6, 2023 at 5:29 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> >
> > Use SPDX-License-Identifier instead of the license text and
> > update the author information.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> > V1 -> V2: Keep GPLv2, just convert to SPDX identifier
> > ---
> >  drivers/gpio/gpio-loongson1.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > index 5d90b3bc5a25..8862c9ea0d41 100644
> > --- a/drivers/gpio/gpio-loongson1.c
> > +++ b/drivers/gpio/gpio-loongson1.c
> > @@ -1,11 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> >  /*
> >   * GPIO Driver for Loongson 1 SoC
> >   *
> > - * Copyright (C) 2015-2016 Zhang, Keguang <keguang.zhang@gmail.com>
> > - *
> > - * This file is licensed under the terms of the GNU General Public
> > - * License version 2. This program is licensed "as is" without any
> > - * warranty of any kind, whether express or implied.
> > + * Copyright (C) 2015-2023 Keguang Zhang <keguang.zhang@gmail.com>
> >   */
> >
> >  #include <linux/module.h>
> > @@ -90,6 +87,6 @@ static struct platform_driver ls1x_gpio_driver = {
> >
> >  module_platform_driver(ls1x_gpio_driver);
> >
> > -MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@gmail.com>");
>
> Why are you removing credits of the old author?
Kelvin Cheung and Keguang Zhang are the same person.
This change is to keep pace with the related entry of MAINTAINERS.

>
> Bart
>
> > +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
> >  MODULE_DESCRIPTION("Loongson1 GPIO driver");
> >  MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >



-- 
Best regards,

Kelvin Cheung

Re: [PATCH v2 4/5] gpio: loongson1: Add DT support

[Keguang Zhang]

On Mon, Mar 6, 2023 at 5:39 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Mar 2, 2023 at 1:53 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> >
> > This patch adds DT support for Loongson-1 GPIO driver.
> >
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> > V1 -> V2: Let gpiolib parse ngpios property
> >           Remove unnecessary alias id parsing
> >           Remove superfluous initialization done by bgpio_init()
> >           Add MODULE_DEVICE_TABLE()
> >           Other minor fixes
> > ---
> >  drivers/gpio/gpio-loongson1.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > index 3ac9e49e7efb..94ac0ccb450f 100644
> > --- a/drivers/gpio/gpio-loongson1.c
> > +++ b/drivers/gpio/gpio-loongson1.c
> > @@ -68,25 +68,38 @@ static int ls1x_gpio_probe(struct platform_device *pdev)
> >         ls1x_gc->gc.owner = THIS_MODULE;
> >         ls1x_gc->gc.request = ls1x_gpio_request;
> >         ls1x_gc->gc.free = ls1x_gpio_free;
> > -       ls1x_gc->gc.base = pdev->id * 32;
> > +       /*
> > +        * Clear ngpio to let gpiolib get the correct number
> > +        * by reading ngpios property
> > +        */
> > +       ls1x_gc->gc.ngpio = 0;
> >
>
> Who could have set it before and why would this information need to be
> unconditionally discarded?
>
'ngpio' has been set to 32 by bgpio_init().
But this is incorrect for LS1B who has different number of GPIOs for each group.
To get the right number, I have to discard this default value to let
gpiolib parse 'ngpios' property.


> Bart
>
> >         ret = devm_gpiochip_add_data(dev, &ls1x_gc->gc, ls1x_gc);
> >         if (ret)
> >                 goto err;
> >
> >         platform_set_drvdata(pdev, ls1x_gc);
> > -       dev_info(dev, "Loongson1 GPIO driver registered\n");
> > +
> > +       dev_info(dev, "GPIO controller registered with %d pins\n",
> > +                ls1x_gc->gc.ngpio);
> >
> >         return 0;
> >  err:
> > -       dev_err(dev, "failed to register GPIO device\n");
> > +       dev_err(dev, "failed to register GPIO controller\n");
> >         return ret;
> >  }
> >
> > +static const struct of_device_id ls1x_gpio_dt_ids[] = {
> > +       { .compatible = "loongson,ls1x-gpio" },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ls1x_gpio_dt_ids);
> > +
> >  static struct platform_driver ls1x_gpio_driver = {
> >         .probe  = ls1x_gpio_probe,
> >         .driver = {
> >                 .name   = "ls1x-gpio",
> > +               .of_match_table = ls1x_gpio_dt_ids,
> >         },
> >  };
> >
> > --
> > 2.34.1
> >



--
Best regards,

Kelvin Cheung

Re: [PATCH v2 2/5] gpio: loongson1: Use readl() & writel()

[Keguang Zhang]

On Mon, Mar 6, 2023 at 5:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> >
> > This patch replace __raw_readl() & __raw_writel() with readl() & writel().
> >
>
> Please say WHY you're doing this.
>
readl & writel contain memory barriers which can guarantee access order.

> Bart
>
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > ---
> > V1 -> V2: Split this change to a separate patch
> > ---
> >  drivers/gpio/gpio-loongson1.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > index 8862c9ea0d41..b6c11caa3ade 100644
> > --- a/drivers/gpio/gpio-loongson1.c
> > +++ b/drivers/gpio/gpio-loongson1.c
> > @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> >         unsigned long flags;
> >
> >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > -                    gpio_reg_base + GPIO_CFG);
> > +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > +              gpio_reg_base + GPIO_CFG);
> >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> >
> >         return 0;
> > @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
> >         unsigned long flags;
> >
> >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > -                    gpio_reg_base + GPIO_CFG);
> > +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > +              gpio_reg_base + GPIO_CFG);
> >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> >  }
> >
> > --
> > 2.34.1
> >



--
Best regards,

Kelvin Cheung

Re: [PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Linus Walleij]

On Tue, Mar 7, 2023 at 3:25 AM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> On Mon, Mar 6, 2023 at 5:29 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > >
> > > Use SPDX-License-Identifier instead of the license text and
> > > update the author information.
> > >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>

> > Why are you removing credits of the old author?

> Kelvin Cheung and Keguang Zhang are the same person.
> This change is to keep pace with the related entry of MAINTAINERS.

That's a pretty interesting change!

Is Kelvin Cheung the "westernized" name and Keguang Zhang the
closer to the real name, such as pinyin form? That would make
a lot of sense.

I think some authors even use the native characters these days,
as git and all tools and terminals should support Unicode now.
It might make it hard for us to answer mails (not knowing which
characters to refer to as given name) but I kind of like it when I
see it.

Yours,
Linus Walleij

Re: [PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Bartosz Golaszewski]

On Tue, Mar 7, 2023 at 3:25 AM Keguang Zhang <keguang.zhang@gmail.com> wrote:
>
> On Mon, Mar 6, 2023 at 5:29 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > >
> > > Use SPDX-License-Identifier instead of the license text and
> > > update the author information.
> > >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > ---
> > > V1 -> V2: Keep GPLv2, just convert to SPDX identifier
> > > ---
> > >  drivers/gpio/gpio-loongson1.c | 9 +++------
> > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > > index 5d90b3bc5a25..8862c9ea0d41 100644
> > > --- a/drivers/gpio/gpio-loongson1.c
> > > +++ b/drivers/gpio/gpio-loongson1.c
> > > @@ -1,11 +1,8 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > >  /*
> > >   * GPIO Driver for Loongson 1 SoC
> > >   *
> > > - * Copyright (C) 2015-2016 Zhang, Keguang <keguang.zhang@gmail.com>
> > > - *
> > > - * This file is licensed under the terms of the GNU General Public
> > > - * License version 2. This program is licensed "as is" without any
> > > - * warranty of any kind, whether express or implied.
> > > + * Copyright (C) 2015-2023 Keguang Zhang <keguang.zhang@gmail.com>
> > >   */
> > >
> > >  #include <linux/module.h>
> > > @@ -90,6 +87,6 @@ static struct platform_driver ls1x_gpio_driver = {
> > >
> > >  module_platform_driver(ls1x_gpio_driver);
> > >
> > > -MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@gmail.com>");
> >
> > Why are you removing credits of the old author?
> Kelvin Cheung and Keguang Zhang are the same person.
> This change is to keep pace with the related entry of MAINTAINERS.
>

Even so - how could I have possibly known this? Please put it into the
commit message, it's crucial information for this change.

Bart

Re: [PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Keguang Zhang]

On Tue, Mar 7, 2023 at 9:31 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Mar 7, 2023 at 3:25 AM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > On Mon, Mar 6, 2023 at 5:29 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > > >
> > > > Use SPDX-License-Identifier instead of the license text and
> > > > update the author information.
> > > >
> > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
>
> > > Why are you removing credits of the old author?
>
> > Kelvin Cheung and Keguang Zhang are the same person.
> > This change is to keep pace with the related entry of MAINTAINERS.
>
> That's a pretty interesting change!
>
> Is Kelvin Cheung the "westernized" name and Keguang Zhang the
> closer to the real name, such as pinyin form? That would make
> a lot of sense.
>
Exactly.
Kelvin Cheung is easy to pronounce, but has no direct relationship
with my real name.
Keguang Zhang, the Pinyin form of my real name, is the official name.
That is why I'd like to make this change.

> I think some authors even use the native characters these days,
> as git and all tools and terminals should support Unicode now.
> It might make it hard for us to answer mails (not knowing which
> characters to refer to as given name) but I kind of like it when I
> see it.
>
Yes, I did see the names written in native characters.
But it's even harder for western people to identify.
Maybe the real name with native characters + "westernized" name is better.

> Yours,
> Linus Walleij



-- 
Best regards,

Kelvin Cheung

Re: [PATCH v2 1/5] gpio: loongson1: Convert to SPDX identifier

[Keguang Zhang]

On Wed, Mar 8, 2023 at 12:49 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Mar 7, 2023 at 3:25 AM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> >
> > On Mon, Mar 6, 2023 at 5:29 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > > >
> > > > Use SPDX-License-Identifier instead of the license text and
> > > > update the author information.
> > > >
> > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > > ---
> > > > V1 -> V2: Keep GPLv2, just convert to SPDX identifier
> > > > ---
> > > >  drivers/gpio/gpio-loongson1.c | 9 +++------
> > > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > > > index 5d90b3bc5a25..8862c9ea0d41 100644
> > > > --- a/drivers/gpio/gpio-loongson1.c
> > > > +++ b/drivers/gpio/gpio-loongson1.c
> > > > @@ -1,11 +1,8 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > >  /*
> > > >   * GPIO Driver for Loongson 1 SoC
> > > >   *
> > > > - * Copyright (C) 2015-2016 Zhang, Keguang <keguang.zhang@gmail.com>
> > > > - *
> > > > - * This file is licensed under the terms of the GNU General Public
> > > > - * License version 2. This program is licensed "as is" without any
> > > > - * warranty of any kind, whether express or implied.
> > > > + * Copyright (C) 2015-2023 Keguang Zhang <keguang.zhang@gmail.com>
> > > >   */
> > > >
> > > >  #include <linux/module.h>
> > > > @@ -90,6 +87,6 @@ static struct platform_driver ls1x_gpio_driver = {
> > > >
> > > >  module_platform_driver(ls1x_gpio_driver);
> > > >
> > > > -MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@gmail.com>");
> > >
> > > Why are you removing credits of the old author?
> > Kelvin Cheung and Keguang Zhang are the same person.
> > This change is to keep pace with the related entry of MAINTAINERS.
> >
>
> Even so - how could I have possibly known this? Please put it into the
> commit message, it's crucial information for this change.
>
Sure. I will amend the commit message.
In addition, should I update this patch only? Or the whole patch series?

> Bart



-- 
Best regards,

Kelvin Cheung

RE: [PATCH v2 2/5] gpio: loongson1: Use readl() & writel()

[David Laight]

From: Keguang Zhang
> Sent: 07 March 2023 03:46
> 
> On Mon, Mar 6, 2023 at 5:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > >
> > > This patch replace __raw_readl() & __raw_writel() with readl() & writel().
> > >
> >
> > Please say WHY you're doing this.
> >
> readl & writel contain memory barriers which can guarantee access order.

So what...

There is a data dependency between the read and write.
The read can't be scheduled before the lock is acquired.
The write can't be scheduled after the lock is released.

So any barriers in readl()/writel() aren't needed.

If they are only compile barriers they'll have no real effect.
OTOH if the cpu needs actual synchronising instructions (as some
ppc do) then they will slow things down.

	David

> 
> > Bart
> >
> > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > ---
> > > V1 -> V2: Split this change to a separate patch
> > > ---
> > >  drivers/gpio/gpio-loongson1.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > > index 8862c9ea0d41..b6c11caa3ade 100644
> > > --- a/drivers/gpio/gpio-loongson1.c
> > > +++ b/drivers/gpio/gpio-loongson1.c
> > > @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> > >         unsigned long flags;
> > >
> > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > -                    gpio_reg_base + GPIO_CFG);
> > > +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > +              gpio_reg_base + GPIO_CFG);
> > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > >
> > >         return 0;
> > > @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
> > >         unsigned long flags;
> > >
> > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > -                    gpio_reg_base + GPIO_CFG);
> > > +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > +              gpio_reg_base + GPIO_CFG);
> > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > >  }
> > >
> > > --
> > > 2.34.1
> > >
> 
> 
> 
> --
> Best regards,
> 
> Kelvin Cheung

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 2/5] gpio: loongson1: Use readl() & writel()

[Keguang Zhang]

On Wed, Mar 8, 2023 at 5:42 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Keguang Zhang
> > Sent: 07 March 2023 03:46
> >
> > On Mon, Mar 6, 2023 at 5:30 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Thu, Mar 2, 2023 at 1:52 PM Keguang Zhang <keguang.zhang@gmail.com> wrote:
> > > >
> > > > This patch replace __raw_readl() & __raw_writel() with readl() & writel().
> > > >
> > >
> > > Please say WHY you're doing this.
> > >
> > readl & writel contain memory barriers which can guarantee access order.
>
> So what...
>
> There is a data dependency between the read and write.
> The read can't be scheduled before the lock is acquired.
> The write can't be scheduled after the lock is released.
>
> So any barriers in readl()/writel() aren't needed.
>
> If they are only compile barriers they'll have no real effect.
> OTOH if the cpu needs actual synchronising instructions (as some
> ppc do) then they will slow things down.
>
Thanks for the explanation.
The intention of this change is to prevent possible order issues.
At present, __raw_readl() & __raw_writel() do work fine.
I will drop this patch in the next version.


>         David
>
> >
> > > Bart
> > >
> > > > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com>
> > > > ---
> > > > V1 -> V2: Split this change to a separate patch
> > > > ---
> > > >  drivers/gpio/gpio-loongson1.c | 8 ++++----
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-loongson1.c b/drivers/gpio/gpio-loongson1.c
> > > > index 8862c9ea0d41..b6c11caa3ade 100644
> > > > --- a/drivers/gpio/gpio-loongson1.c
> > > > +++ b/drivers/gpio/gpio-loongson1.c
> > > > @@ -23,8 +23,8 @@ static int ls1x_gpio_request(struct gpio_chip *gc, unsigned int offset)
> > > >         unsigned long flags;
> > > >
> > > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > > -                    gpio_reg_base + GPIO_CFG);
> > > > +       writel(readl(gpio_reg_base + GPIO_CFG) | BIT(offset),
> > > > +              gpio_reg_base + GPIO_CFG);
> > > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > > >
> > > >         return 0;
> > > > @@ -35,8 +35,8 @@ static void ls1x_gpio_free(struct gpio_chip *gc, unsigned int offset)
> > > >         unsigned long flags;
> > > >
> > > >         raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> > > > -       __raw_writel(__raw_readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > > -                    gpio_reg_base + GPIO_CFG);
> > > > +       writel(readl(gpio_reg_base + GPIO_CFG) & ~BIT(offset),
> > > > +              gpio_reg_base + GPIO_CFG);
> > > >         raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> > > >  }
> > > >
> > > > --
> > > > 2.34.1
> > > >
> >
> >
> >
> > --
> > Best regards,
> >
> > Kelvin Cheung
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)



--
Best regards,

Kelvin Cheung