[PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Baolin Wang]

This patch adds the device tree bindings for the Spreadtrum
GPIO controller. The gpios will be supported by the GPIO
generic library.

Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Changes since v1:
 - No updates.
---
 .../devicetree/bindings/gpio/gpio-sprd.txt         |   28 ++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sprd.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-sprd.txt b/Documentation/devicetree/bindings/gpio/gpio-sprd.txt
new file mode 100644
index 0000000..eca97d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-sprd.txt
@@ -0,0 +1,28 @@
+Spreadtrum GPIO controller bindings
+
+The controller's registers are organized as sets of sixteen 16-bit
+registers with each set controlling a bank of up to 16 pins. A single
+interrupt is shared for all of the banks handled by the controller.
+
+Required properties:
+- compatible: Should be "sprd,sc9860-gpio".
+- reg: Define the base and range of the I/O address space containing
+the GPIO controller registers.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be <2>. The first cell is the gpio number and
+the second cell is used to specify optional parameters.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Should be <2>. Specifies the number of cells needed
+to encode interrupt source.
+- interrupts: Should be the port interrupt shared by all the gpios.
+
+Example:
+	ap_gpio: gpio@40280000 {
+		compatible = "sprd,sc9860-gpio";
+		reg = <0 0x40280000 0 0x1000>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Baolin Wang]

This patch adds the device tree bindings for the Spreadtrum
GPIO controller. The gpios will be supported by the GPIO
generic library.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v1:
 - No updates.
---
 .../devicetree/bindings/gpio/gpio-sprd.txt         |   28 ++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sprd.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-sprd.txt b/Documentation/devicetree/bindings/gpio/gpio-sprd.txt
new file mode 100644
index 0000000..eca97d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-sprd.txt
@@ -0,0 +1,28 @@
+Spreadtrum GPIO controller bindings
+
+The controller's registers are organized as sets of sixteen 16-bit
+registers with each set controlling a bank of up to 16 pins. A single
+interrupt is shared for all of the banks handled by the controller.
+
+Required properties:
+- compatible: Should be "sprd,sc9860-gpio".
+- reg: Define the base and range of the I/O address space containing
+the GPIO controller registers.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be <2>. The first cell is the gpio number and
+the second cell is used to specify optional parameters.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Should be <2>. Specifies the number of cells needed
+to encode interrupt source.
+- interrupts: Should be the port interrupt shared by all the gpios.
+
+Example:
+	ap_gpio: gpio@40280000 {
+		compatible = "sprd,sc9860-gpio";
+		reg = <0 0x40280000 0 0x1000>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+	};
-- 
1.7.9.5

[PATCH v3 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Baolin Wang]

The Spreadtrum SC9860 platform GPIO controller contains 16 groups and
each group contains 16 GPIOs. Each GPIO can set input/output and has
the interrupt capability.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes since v2:
 - Use devm_ioremap_resource() instead of devm_ioremap_nocache().

Changes since v1:
 - Change 'bool' to 'tristate'.
 - Add reviewed tag from Andy.
---
 drivers/gpio/Kconfig     |    7 ++
 drivers/gpio/Makefile    |    1 +
 drivers/gpio/gpio-sprd.c |  290 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 298 insertions(+)
 create mode 100644 drivers/gpio/gpio-sprd.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d6a8e85..2ed1a88 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -404,6 +404,13 @@ config GPIO_SPEAR_SPICS
 	help
 	  Say yes here to support ST SPEAr SPI Chip Select as GPIO device
 
+config GPIO_SPRD
+	tristate "Spreadtrum GPIO support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support Spreadtrum GPIO device.
+
 config GPIO_STA2X11
 	bool "STA2x11/ConneXt GPIO support"
 	depends on MFD_STA2X11
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4bc24fe..5b633a0 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -108,6 +108,7 @@ obj-$(CONFIG_GPIO_SCH)		+= gpio-sch.o
 obj-$(CONFIG_GPIO_SCH311X)	+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SODAVILLE)	+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)	+= gpio-spear-spics.o
+obj-$(CONFIG_GPIO_SPRD)		+= gpio-sprd.o
 obj-$(CONFIG_GPIO_STA2X11)	+= gpio-sta2x11.o
 obj-$(CONFIG_GPIO_STMPE)	+= gpio-stmpe.o
 obj-$(CONFIG_GPIO_STP_XWAY)	+= gpio-stp-xway.o
diff --git a/drivers/gpio/gpio-sprd.c b/drivers/gpio/gpio-sprd.c
new file mode 100644
index 0000000..7c0f9f1
--- /dev/null
+++ b/drivers/gpio/gpio-sprd.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Spreadtrum Communications Inc.
+ * Copyright (c) 2018 Linaro Ltd.
+ */
+
+#include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/* GPIO registers definition */
+#define SPRD_GPIO_DATA		0x0
+#define SPRD_GPIO_DMSK		0x4
+#define SPRD_GPIO_DIR		0x8
+#define SPRD_GPIO_IS		0xc
+#define SPRD_GPIO_IBE		0x10
+#define SPRD_GPIO_IEV		0x14
+#define SPRD_GPIO_IE		0x18
+#define SPRD_GPIO_RIS		0x1c
+#define SPRD_GPIO_MIS		0x20
+#define SPRD_GPIO_IC		0x24
+#define SPRD_GPIO_INEN		0x28
+
+/* We have 16 groups GPIOs and each group contain 16 GPIOs */
+#define SPRD_GPIO_GROUP_NR	16
+#define SPRD_GPIO_NR		256
+#define SPRD_GPIO_GROUP_SIZE	0x80
+#define SPRD_GPIO_GROUP_MASK	GENMASK(15, 0)
+#define SPRD_GPIO_BIT(x)	((x) & (SPRD_GPIO_GROUP_NR - 1))
+
+struct sprd_gpio {
+	struct gpio_chip chip;
+	void __iomem *base;
+	spinlock_t lock;
+	int irq;
+};
+
+static inline void __iomem *sprd_gpio_group_base(struct sprd_gpio *sprd_gpio,
+						 unsigned int group)
+{
+	return sprd_gpio->base + SPRD_GPIO_GROUP_SIZE * group;
+}
+
+static void sprd_gpio_update(struct gpio_chip *chip, unsigned int offset,
+			     unsigned int reg, unsigned int val)
+{
+	struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
+	void __iomem *base = sprd_gpio_group_base(sprd_gpio,
+						  offset / SPRD_GPIO_GROUP_NR);
+	u32 shift = SPRD_GPIO_BIT(offset);
+	unsigned long flags;
+	u32 orig, tmp;
+
+	spin_lock_irqsave(&sprd_gpio->lock, flags);
+	orig = readl_relaxed(base + reg);
+
+	tmp = (orig & ~BIT(shift)) | (val << shift);
+	writel_relaxed(tmp, base + reg);
+	spin_unlock_irqrestore(&sprd_gpio->lock, flags);
+}
+
+static int sprd_gpio_read(struct gpio_chip *chip, unsigned int offset,
+			  unsigned int reg)
+{
+	struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
+	void __iomem *base = sprd_gpio_group_base(sprd_gpio,
+						  offset / SPRD_GPIO_GROUP_NR);
+	u32 value = readl_relaxed(base + reg) & SPRD_GPIO_GROUP_MASK;
+	u32 shift = SPRD_GPIO_BIT(offset);
+
+	return !!(value & BIT(shift));
+}
+
+static int sprd_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	sprd_gpio_update(chip, offset, SPRD_GPIO_DMSK, 1);
+	return 0;
+}
+
+static void sprd_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	sprd_gpio_update(chip, offset, SPRD_GPIO_DMSK, 0);
+}
+
+static int sprd_gpio_direction_input(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	sprd_gpio_update(chip, offset, SPRD_GPIO_DIR, 0);
+	sprd_gpio_update(chip, offset, SPRD_GPIO_INEN, 1);
+	return 0;
+}
+
+static int sprd_gpio_direction_output(struct gpio_chip *chip,
+				      unsigned int offset, int value)
+{
+	sprd_gpio_update(chip, offset, SPRD_GPIO_DIR, 1);
+	sprd_gpio_update(chip, offset, SPRD_GPIO_INEN, 0);
+	sprd_gpio_update(chip, offset, SPRD_GPIO_DATA, value);
+	return 0;
+}
+
+static int sprd_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return sprd_gpio_read(chip, offset, SPRD_GPIO_DATA);
+}
+
+static void sprd_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			  int value)
+{
+	sprd_gpio_update(chip, offset, SPRD_GPIO_DATA, value);
+}
+
+static void sprd_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	u32 offset = irqd_to_hwirq(data);
+
+	sprd_gpio_update(chip, offset, SPRD_GPIO_IE, 0);
+}
+
+static void sprd_gpio_irq_ack(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	u32 offset = irqd_to_hwirq(data);
+
+	sprd_gpio_update(chip, offset, SPRD_GPIO_IC, 1);
+}
+
+static void sprd_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	u32 offset = irqd_to_hwirq(data);
+
+	sprd_gpio_update(chip, offset, SPRD_GPIO_IE, 1);
+}
+
+static int sprd_gpio_irq_set_type(struct irq_data *data,
+				  unsigned int flow_type)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	u32 offset = irqd_to_hwirq(data);
+
+	switch (flow_type) {
+	case IRQ_TYPE_EDGE_RISING:
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
+		irq_set_handler_locked(data, handle_edge_irq);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
+		irq_set_handler_locked(data, handle_edge_irq);
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 1);
+		irq_set_handler_locked(data, handle_edge_irq);
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 1);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 1);
+		irq_set_handler_locked(data, handle_level_irq);
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IS, 1);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IBE, 0);
+		sprd_gpio_update(chip, offset, SPRD_GPIO_IEV, 0);
+		irq_set_handler_locked(data, handle_level_irq);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void sprd_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *chip = irq_desc_get_handler_data(desc);
+	struct irq_chip *ic = irq_desc_get_chip(desc);
+	struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
+	u32 group, n, girq;
+
+	chained_irq_enter(ic, desc);
+
+	for (group = 0; group * SPRD_GPIO_GROUP_NR < chip->ngpio; group++) {
+		void __iomem *base = sprd_gpio_group_base(sprd_gpio, group);
+		unsigned long reg = readl_relaxed(base + SPRD_GPIO_MIS) &
+			SPRD_GPIO_GROUP_MASK;
+
+		for_each_set_bit(n, &reg, SPRD_GPIO_GROUP_NR) {
+			girq = irq_find_mapping(chip->irq.domain,
+						group * SPRD_GPIO_GROUP_NR + n);
+
+			generic_handle_irq(girq);
+		}
+
+	}
+	chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip sprd_gpio_irqchip = {
+	.name = "sprd-gpio",
+	.irq_ack = sprd_gpio_irq_ack,
+	.irq_mask = sprd_gpio_irq_mask,
+	.irq_unmask = sprd_gpio_irq_unmask,
+	.irq_set_type = sprd_gpio_irq_set_type,
+	.flags = IRQCHIP_SKIP_SET_WAKE,
+};
+
+static int sprd_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_irq_chip *irq;
+	struct sprd_gpio *sprd_gpio;
+	struct resource *res;
+	int ret;
+
+	sprd_gpio = devm_kzalloc(&pdev->dev, sizeof(*sprd_gpio), GFP_KERNEL);
+	if (!sprd_gpio)
+		return -ENOMEM;
+
+	sprd_gpio->irq = platform_get_irq(pdev, 0);
+	if (sprd_gpio->irq < 0) {
+		dev_err(&pdev->dev, "Failed to get GPIO interrupt.\n");
+		return sprd_gpio->irq;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sprd_gpio->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(sprd_gpio->base))
+		return PTR_ERR(sprd_gpio->base);
+
+	spin_lock_init(&sprd_gpio->lock);
+
+	sprd_gpio->chip.label = dev_name(&pdev->dev);
+	sprd_gpio->chip.ngpio = SPRD_GPIO_NR;
+	sprd_gpio->chip.base = -1;
+	sprd_gpio->chip.parent = &pdev->dev;
+	sprd_gpio->chip.of_node = pdev->dev.of_node;
+	sprd_gpio->chip.request = sprd_gpio_request;
+	sprd_gpio->chip.free = sprd_gpio_free;
+	sprd_gpio->chip.get = sprd_gpio_get;
+	sprd_gpio->chip.set = sprd_gpio_set;
+	sprd_gpio->chip.direction_input = sprd_gpio_direction_input;
+	sprd_gpio->chip.direction_output = sprd_gpio_direction_output;
+
+	irq = &sprd_gpio->chip.irq;
+	irq->chip = &sprd_gpio_irqchip;
+	irq->handler = handle_simple_irq;
+	irq->default_type = IRQ_TYPE_NONE;
+	irq->parent_handler = sprd_gpio_irq_handler;
+	irq->parent_handler_data = sprd_gpio;
+	irq->num_parents = 1;
+	irq->parents = &sprd_gpio->irq;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &sprd_gpio->chip, sprd_gpio);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, sprd_gpio);
+	return 0;
+}
+
+static const struct of_device_id sprd_gpio_of_match[] = {
+	{ .compatible = "sprd,sc9860-gpio", },
+	{ /* end of list */ },
+};
+MODULE_DEVICE_TABLE(of, sprd_gpio_of_match);
+
+static struct platform_driver sprd_gpio_driver = {
+	.probe = sprd_gpio_probe,
+	.driver = {
+		.name = "sprd-gpio",
+		.of_match_table	= sprd_gpio_of_match,
+	},
+};
+
+module_platform_driver_probe(sprd_gpio_driver, sprd_gpio_probe);
+
+MODULE_DESCRIPTION("Spreadtrum GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v3 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Linus Walleij]

On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> The Spreadtrum SC9860 platform GPIO controller contains 16 groups and
> each group contains 16 GPIOs. Each GPIO can set input/output and has
> the interrupt capability.
>
> Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> Changes since v2:

Hi Baolin,

sorry for taking so long to review :(

I think you need to add

depends on OF_GPIO to the dependencies. Else the build
will break on compile test on things like Usermode Linux
that doesn't have IOMEM.

> +/* GPIO registers definition */
> +#define SPRD_GPIO_DATA         0x0
> +#define SPRD_GPIO_DMSK         0x4
> +#define SPRD_GPIO_DIR          0x8
> +#define SPRD_GPIO_IS           0xc
> +#define SPRD_GPIO_IBE          0x10
> +#define SPRD_GPIO_IEV          0x14
> +#define SPRD_GPIO_IE           0x18
> +#define SPRD_GPIO_RIS          0x1c
> +#define SPRD_GPIO_MIS          0x20
> +#define SPRD_GPIO_IC           0x24
> +#define SPRD_GPIO_INEN         0x28

So this is very simple. And the only reason we cannot use
GPIO_GENERIC is that we have these groups inside the controller
and a shared interrupt line :/

Hm yeah I cannot think of anything better anyway.

Have you contemplated just putting them into the device tree
like this:

ap_gpio0: gpio@40280000 {
        compatible = "sprd,sc9860-gpio";
        reg = <0 0x40280000 0 0x80>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};

ap_gpio1: gpio@40280080 {
        compatible = "sprd,sc9860-gpio";
        reg = <0 0x40280080 0 0x80>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};

ap_gpio2: gpio@40280100 {
        compatible = "sprd,sc9860-gpio";
        reg = <0 0x40280100 0 0x80>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};

(...)

?

It is fine to have 16 driver instances if you grab the interrupt
with IRQF_SHARED and really just handle the IRQ if it is for
"your" instance. The upside is that you could then use
GPIO_GENERIC and get a very small and simple driver.

I understand that the current approach is also appealing though
and I see why. I'm not gonna say no to it, so if you strongly
prefer this approach we can go for it. Just wanted to point out
alternatives.

> +/* We have 16 groups GPIOs and each group contain 16 GPIOs */
> +#define SPRD_GPIO_GROUP_NR     16
> +#define SPRD_GPIO_NR           256
> +#define SPRD_GPIO_GROUP_SIZE   0x80
> +#define SPRD_GPIO_GROUP_MASK   GENMASK(15, 0)
> +#define SPRD_GPIO_BIT(x)       ((x) & (SPRD_GPIO_GROUP_NR - 1))

Please rename this from "groups" to "banks" because in the GPIO
subsystem everyone talks about "banks" not "groups".

This last thing many drivers do like this:

bit = x % 15;

but it is up to you, either way works (and probably result in the
same assembly).

> +static inline void __iomem *sprd_gpio_group_base(struct sprd_gpio *sprd_gpio,
> +                                                unsigned int group)
> +{
> +       return sprd_gpio->base + SPRD_GPIO_GROUP_SIZE * group;
> +}

Since you're always using this like this:

void __iomem *base = sprd_gpio_group_base(sprd_gpio,
                                            offset / SPRD_GPIO_GROUP_NR);

Why not simply have the offset as parameter to the function
instead of group number and do the division inside this
static inline?

> +static void sprd_gpio_update(struct gpio_chip *chip, unsigned int offset,
> +                            unsigned int reg, unsigned int val)

I would use u16 reg.

> +{
> +       struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
> +       void __iomem *base = sprd_gpio_group_base(sprd_gpio,
> +                                                 offset / SPRD_GPIO_GROUP_NR);
> +       u32 shift = SPRD_GPIO_BIT(offset);
> +       unsigned long flags;
> +       u32 orig, tmp;
> +
> +       spin_lock_irqsave(&sprd_gpio->lock, flags);
> +       orig = readl_relaxed(base + reg);
> +
> +       tmp = (orig & ~BIT(shift)) | (val << shift);
> +       writel_relaxed(tmp, base + reg);
> +       spin_unlock_irqrestore(&sprd_gpio->lock, flags);
> +}

You don't need shift, orig and tmp variables here, I think it
gets hard to read.

I would do it like this:

u32 tmp;

tmp = readl_relaxed(base + reg);
if (val)
    tmp |= BIT(SPRD_GPIO_BIT(offset));
else
    tmp &= ~BIT(SPRD_GPIO_BIT(offset));
writel_relaxed(tmp, base + reg);

I don't know if the macros really help much. Maybe just inline it:

tmp = readl_relaxed(base + reg);
if (val)
    tmp |= BIT(offset % 15);
else
    tmp &= ~BIT(offset % 15);
writel_relaxed(tmp, base + reg);

It depends on taste. Just consider my options.
(I'll go with what you feel is easiest to read.)

> +static int sprd_gpio_read(struct gpio_chip *chip, unsigned int offset,
> +                         unsigned int reg)
> +{
> +       struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
> +       void __iomem *base = sprd_gpio_group_base(sprd_gpio,
> +                                                 offset / SPRD_GPIO_GROUP_NR);
> +       u32 value = readl_relaxed(base + reg) & SPRD_GPIO_GROUP_MASK;
> +       u32 shift = SPRD_GPIO_BIT(offset);
> +
> +       return !!(value & BIT(shift));

I would just

return !!(readl_relaxed(base + reg) & BIT(offset % 15)):

But again it is a matter of taste.

(the rest looks fine!)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Linus Walleij]

On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> The Spreadtrum SC9860 platform GPIO controller contains 16 groups and
> each group contains 16 GPIOs. Each GPIO can set input/output and has
> the interrupt capability.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> Changes since v2:

Hi Baolin,

sorry for taking so long to review :(

I think you need to add

depends on OF_GPIO to the dependencies. Else the build
will break on compile test on things like Usermode Linux
that doesn't have IOMEM.

> +/* GPIO registers definition */
> +#define SPRD_GPIO_DATA         0x0
> +#define SPRD_GPIO_DMSK         0x4
> +#define SPRD_GPIO_DIR          0x8
> +#define SPRD_GPIO_IS           0xc
> +#define SPRD_GPIO_IBE          0x10
> +#define SPRD_GPIO_IEV          0x14
> +#define SPRD_GPIO_IE           0x18
> +#define SPRD_GPIO_RIS          0x1c
> +#define SPRD_GPIO_MIS          0x20
> +#define SPRD_GPIO_IC           0x24
> +#define SPRD_GPIO_INEN         0x28

So this is very simple. And the only reason we cannot use
GPIO_GENERIC is that we have these groups inside the controller
and a shared interrupt line :/

Hm yeah I cannot think of anything better anyway.

Have you contemplated just putting them into the device tree
like this:

ap_gpio0: gpio@40280000 {
        compatible = "sprd,sc9860-gpio";
        reg = <0 0x40280000 0 0x80>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};

ap_gpio1: gpio@40280080 {
        compatible = "sprd,sc9860-gpio";
        reg = <0 0x40280080 0 0x80>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};

ap_gpio2: gpio@40280100 {
        compatible = "sprd,sc9860-gpio";
        reg = <0 0x40280100 0 0x80>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
};

(...)

?

It is fine to have 16 driver instances if you grab the interrupt
with IRQF_SHARED and really just handle the IRQ if it is for
"your" instance. The upside is that you could then use
GPIO_GENERIC and get a very small and simple driver.

I understand that the current approach is also appealing though
and I see why. I'm not gonna say no to it, so if you strongly
prefer this approach we can go for it. Just wanted to point out
alternatives.

> +/* We have 16 groups GPIOs and each group contain 16 GPIOs */
> +#define SPRD_GPIO_GROUP_NR     16
> +#define SPRD_GPIO_NR           256
> +#define SPRD_GPIO_GROUP_SIZE   0x80
> +#define SPRD_GPIO_GROUP_MASK   GENMASK(15, 0)
> +#define SPRD_GPIO_BIT(x)       ((x) & (SPRD_GPIO_GROUP_NR - 1))

Please rename this from "groups" to "banks" because in the GPIO
subsystem everyone talks about "banks" not "groups".

This last thing many drivers do like this:

bit = x % 15;

but it is up to you, either way works (and probably result in the
same assembly).

> +static inline void __iomem *sprd_gpio_group_base(struct sprd_gpio *sprd_gpio,
> +                                                unsigned int group)
> +{
> +       return sprd_gpio->base + SPRD_GPIO_GROUP_SIZE * group;
> +}

Since you're always using this like this:

void __iomem *base = sprd_gpio_group_base(sprd_gpio,
                                            offset / SPRD_GPIO_GROUP_NR);

Why not simply have the offset as parameter to the function
instead of group number and do the division inside this
static inline?

> +static void sprd_gpio_update(struct gpio_chip *chip, unsigned int offset,
> +                            unsigned int reg, unsigned int val)

I would use u16 reg.

> +{
> +       struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
> +       void __iomem *base = sprd_gpio_group_base(sprd_gpio,
> +                                                 offset / SPRD_GPIO_GROUP_NR);
> +       u32 shift = SPRD_GPIO_BIT(offset);
> +       unsigned long flags;
> +       u32 orig, tmp;
> +
> +       spin_lock_irqsave(&sprd_gpio->lock, flags);
> +       orig = readl_relaxed(base + reg);
> +
> +       tmp = (orig & ~BIT(shift)) | (val << shift);
> +       writel_relaxed(tmp, base + reg);
> +       spin_unlock_irqrestore(&sprd_gpio->lock, flags);
> +}

You don't need shift, orig and tmp variables here, I think it
gets hard to read.

I would do it like this:

u32 tmp;

tmp = readl_relaxed(base + reg);
if (val)
    tmp |= BIT(SPRD_GPIO_BIT(offset));
else
    tmp &= ~BIT(SPRD_GPIO_BIT(offset));
writel_relaxed(tmp, base + reg);

I don't know if the macros really help much. Maybe just inline it:

tmp = readl_relaxed(base + reg);
if (val)
    tmp |= BIT(offset % 15);
else
    tmp &= ~BIT(offset % 15);
writel_relaxed(tmp, base + reg);

It depends on taste. Just consider my options.
(I'll go with what you feel is easiest to read.)

> +static int sprd_gpio_read(struct gpio_chip *chip, unsigned int offset,
> +                         unsigned int reg)
> +{
> +       struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
> +       void __iomem *base = sprd_gpio_group_base(sprd_gpio,
> +                                                 offset / SPRD_GPIO_GROUP_NR);
> +       u32 value = readl_relaxed(base + reg) & SPRD_GPIO_GROUP_MASK;
> +       u32 shift = SPRD_GPIO_BIT(offset);
> +
> +       return !!(value & BIT(shift));

I would just

return !!(readl_relaxed(base + reg) & BIT(offset % 15)):

But again it is a matter of taste.

(the rest looks fine!)

Yours,
Linus Walleij

Re: [PATCH v3 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Baolin Wang]

Hi Linus,

On 13 February 2018 at 16:13, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> The Spreadtrum SC9860 platform GPIO controller contains 16 groups and
>> each group contains 16 GPIOs. Each GPIO can set input/output and has
>> the interrupt capability.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> ---
>> Changes since v2:
>
> Hi Baolin,
>
> sorry for taking so long to review :(
>
> I think you need to add
>
> depends on OF_GPIO to the dependencies. Else the build
> will break on compile test on things like Usermode Linux
> that doesn't have IOMEM.

OK.

>
>> +/* GPIO registers definition */
>> +#define SPRD_GPIO_DATA         0x0
>> +#define SPRD_GPIO_DMSK         0x4
>> +#define SPRD_GPIO_DIR          0x8
>> +#define SPRD_GPIO_IS           0xc
>> +#define SPRD_GPIO_IBE          0x10
>> +#define SPRD_GPIO_IEV          0x14
>> +#define SPRD_GPIO_IE           0x18
>> +#define SPRD_GPIO_RIS          0x1c
>> +#define SPRD_GPIO_MIS          0x20
>> +#define SPRD_GPIO_IC           0x24
>> +#define SPRD_GPIO_INEN         0x28
>
> So this is very simple. And the only reason we cannot use
> GPIO_GENERIC is that we have these groups inside the controller
> and a shared interrupt line :/
>
> Hm yeah I cannot think of anything better anyway.
>
> Have you contemplated just putting them into the device tree
> like this:
>
> ap_gpio0: gpio@40280000 {
>         compatible = "sprd,sc9860-gpio";
>         reg = <0 0x40280000 0 0x80>;
>         gpio-controller;
>         #gpio-cells = <2>;
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> ap_gpio1: gpio@40280080 {
>         compatible = "sprd,sc9860-gpio";
>         reg = <0 0x40280080 0 0x80>;
>         gpio-controller;
>         #gpio-cells = <2>;
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> ap_gpio2: gpio@40280100 {
>         compatible = "sprd,sc9860-gpio";
>         reg = <0 0x40280100 0 0x80>;
>         gpio-controller;
>         #gpio-cells = <2>;
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> (...)
>
> ?
>
> It is fine to have 16 driver instances if you grab the interrupt
> with IRQF_SHARED and really just handle the IRQ if it is for
> "your" instance. The upside is that you could then use
> GPIO_GENERIC and get a very small and simple driver.
>
> I understand that the current approach is also appealing though
> and I see why. I'm not gonna say no to it, so if you strongly
> prefer this approach we can go for it. Just wanted to point out
> alternatives.

Thanks for pointing out another approach. But we only have one
controller with multiple banks, so I think it is not easy to maintain
if we split one GPIO controller into multiple ones. Moreover if we
have more banks in future, there will be more device node to maintain.
So I still would like to use one device node to describe the only one
GPIO device.

>
>> +/* We have 16 groups GPIOs and each group contain 16 GPIOs */
>> +#define SPRD_GPIO_GROUP_NR     16
>> +#define SPRD_GPIO_NR           256
>> +#define SPRD_GPIO_GROUP_SIZE   0x80
>> +#define SPRD_GPIO_GROUP_MASK   GENMASK(15, 0)
>> +#define SPRD_GPIO_BIT(x)       ((x) & (SPRD_GPIO_GROUP_NR - 1))
>
> Please rename this from "groups" to "banks" because in the GPIO
> subsystem everyone talks about "banks" not "groups".

Sure.

>
> This last thing many drivers do like this:
>
> bit = x % 15;
>
> but it is up to you, either way works (and probably result in the
> same assembly).

OK.

>
>> +static inline void __iomem *sprd_gpio_group_base(struct sprd_gpio *sprd_gpio,
>> +                                                unsigned int group)
>> +{
>> +       return sprd_gpio->base + SPRD_GPIO_GROUP_SIZE * group;
>> +}
>
> Since you're always using this like this:
>
> void __iomem *base = sprd_gpio_group_base(sprd_gpio,
>                                             offset / SPRD_GPIO_GROUP_NR);
>
> Why not simply have the offset as parameter to the function
> instead of group number and do the division inside this
> static inline?

In sprd_gpio_irq_handler() function, we should pass the group number
as parameter, so to consistent with them, I use group number as
parameter.

>
>> +static void sprd_gpio_update(struct gpio_chip *chip, unsigned int offset,
>> +                            unsigned int reg, unsigned int val)
>
> I would use u16 reg.

Sure.

>
>> +{
>> +       struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
>> +       void __iomem *base = sprd_gpio_group_base(sprd_gpio,
>> +                                                 offset / SPRD_GPIO_GROUP_NR);
>> +       u32 shift = SPRD_GPIO_BIT(offset);
>> +       unsigned long flags;
>> +       u32 orig, tmp;
>> +
>> +       spin_lock_irqsave(&sprd_gpio->lock, flags);
>> +       orig = readl_relaxed(base + reg);
>> +
>> +       tmp = (orig & ~BIT(shift)) | (val << shift);
>> +       writel_relaxed(tmp, base + reg);
>> +       spin_unlock_irqrestore(&sprd_gpio->lock, flags);
>> +}
>
> You don't need shift, orig and tmp variables here, I think it
> gets hard to read.
>
> I would do it like this:
>
> u32 tmp;
>
> tmp = readl_relaxed(base + reg);
> if (val)
>     tmp |= BIT(SPRD_GPIO_BIT(offset));
> else
>     tmp &= ~BIT(SPRD_GPIO_BIT(offset));
> writel_relaxed(tmp, base + reg);

OK. This seems more simpler and clearer.

>
> I don't know if the macros really help much. Maybe just inline it:
>
> tmp = readl_relaxed(base + reg);
> if (val)
>     tmp |= BIT(offset % 15);
> else
>     tmp &= ~BIT(offset % 15);
> writel_relaxed(tmp, base + reg);
>
> It depends on taste. Just consider my options.
> (I'll go with what you feel is easiest to read.)

OK.

>
>> +static int sprd_gpio_read(struct gpio_chip *chip, unsigned int offset,
>> +                         unsigned int reg)
>> +{
>> +       struct sprd_gpio *sprd_gpio = gpiochip_get_data(chip);
>> +       void __iomem *base = sprd_gpio_group_base(sprd_gpio,
>> +                                                 offset / SPRD_GPIO_GROUP_NR);
>> +       u32 value = readl_relaxed(base + reg) & SPRD_GPIO_GROUP_MASK;
>> +       u32 shift = SPRD_GPIO_BIT(offset);
>> +
>> +       return !!(value & BIT(shift));
>
> I would just
>
> return !!(readl_relaxed(base + reg) & BIT(offset % 15)):
>
> But again it is a matter of taste.

OK. Thanks for your useful comments.

-- 
Baolin.wang
Best Regards

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Rob Herring]

On Mon, Feb 05, 2018 at 09:55:10AM +0800, Baolin Wang wrote:
> This patch adds the device tree bindings for the Spreadtrum
> GPIO controller. The gpios will be supported by the GPIO
> generic library.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes since v1:
>  - No updates.
> ---
>  .../devicetree/bindings/gpio/gpio-sprd.txt         |   28 ++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sprd.txt

Please add acks/reviews when posting new versions.

Rob

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Baolin Wang]

Hi Rob,

On 8 February 2018 at 05:19, Rob Herring <robh@kernel.org> wrote:
> On Mon, Feb 05, 2018 at 09:55:10AM +0800, Baolin Wang wrote:
>> This patch adds the device tree bindings for the Spreadtrum
>> GPIO controller. The gpios will be supported by the GPIO
>> generic library.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes since v1:
>>  - No updates.
>> ---
>>  .../devicetree/bindings/gpio/gpio-sprd.txt         |   28 ++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-sprd.txt
>
> Please add acks/reviews when posting new versions.

Yes, I always do that. But you acked my V2 after I already send out
the V3. Anyway thanks for your reviewed-tag.

-- 
Baolin.wang
Best Regards

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Linus Walleij]

On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> This patch adds the device tree bindings for the Spreadtrum
> GPIO controller. The gpios will be supported by the GPIO
> generic library.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Patch applied with Rob's ACK from v2.

Yours,
Linus Walleij

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Linus Walleij]

On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> This patch adds the device tree bindings for the Spreadtrum
> GPIO controller. The gpios will be supported by the GPIO
> generic library.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

I took this patch out again for now, as it turns out
that not all of the variants are really GPIO controllers,
but rather irqchips.

If they can't read nor write any GPIO lines, they should
not have the property gpio-controller at all. Then they are
just irqchips.

We need some extended discussion about this.

Yours,
Linus Walleij

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Baolin Wang]

Hi Linus,

On 13 February 2018 at 17:20, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> This patch adds the device tree bindings for the Spreadtrum
>> GPIO controller. The gpios will be supported by the GPIO
>> generic library.
>>
>> Signed-off-by: Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> I took this patch out again for now, as it turns out
> that not all of the variants are really GPIO controllers,
> but rather irqchips.
>
> If they can't read nor write any GPIO lines, they should
> not have the property gpio-controller at all. Then they are
> just irqchips.

I did not get you here. All GPIOs can be read or write for this GPIO controller.

-- 
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Baolin Wang]

Hi Linus,

On 13 February 2018 at 17:20, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> This patch adds the device tree bindings for the Spreadtrum
>> GPIO controller. The gpios will be supported by the GPIO
>> generic library.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>
> I took this patch out again for now, as it turns out
> that not all of the variants are really GPIO controllers,
> but rather irqchips.
>
> If they can't read nor write any GPIO lines, they should
> not have the property gpio-controller at all. Then they are
> just irqchips.

I did not get you here. All GPIOs can be read or write for this GPIO controller.

-- 
Baolin.wang
Best Regards

Re: [PATCH v3 1/2] dt-bindings: gpio: Add Spreadtrum GPIO controller documentation

[Linus Walleij]

On Wed, Feb 14, 2018 at 3:51 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 13 February 2018 at 17:20, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Feb 5, 2018 at 2:55 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>
>>> This patch adds the device tree bindings for the Spreadtrum
>>> GPIO controller. The gpios will be supported by the GPIO
>>> generic library.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>
>> I took this patch out again for now, as it turns out
>> that not all of the variants are really GPIO controllers,
>> but rather irqchips.
>>
>> If they can't read nor write any GPIO lines, they should
>> not have the property gpio-controller at all. Then they are
>> just irqchips.
>
> I did not get you here. All GPIOs can be read or write for this GPIO controller.

Sorry I confused one of the drivers for the other.

I'm so clumsy sometimes :(

Yours,
Linus Walleij