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

[PATCH 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>
---
 .../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 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/gpio/Kconfig     |    7 ++
 drivers/gpio/Makefile    |    1 +
 drivers/gpio/gpio-sprd.c |  301 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/gpio/gpio-sprd.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d6a8e85..3bece19 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
+	bool "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..af59b9f
--- /dev/null
+++ b/drivers/gpio/gpio-sprd.c
@@ -0,0 +1,301 @@
+// 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_nocache(&pdev->dev, res->start,
+					       resource_size(res));
+	if (!sprd_gpio->base)
+		return -ENOMEM;
+
+	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,
+	},
+};
+
+static int __init sprd_gpio_init(void)
+{
+	return platform_driver_register(&sprd_gpio_driver);
+}
+subsys_initcall(sprd_gpio_init);
+
+static void __exit sprd_gpio_exit(void)
+{
+	platform_driver_unregister(&sprd_gpio_driver);
+}
+module_exit(sprd_gpio_exit);
+
+MODULE_DESCRIPTION("Spreadtrum GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
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

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

[Andy Shevchenko]

On Tue, Jan 30, 2018 at 2:07 PM, 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.

> +config GPIO_SPRD

> +       bool "Spreadtrum GPIO support"

Either you have to put tristate here, or remove all redundant
module_*() and MODULE_*() macros.

Other than that looks fine to me, FWIW:
Reviewed-by: Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
With Best Regards,
Andy Shevchenko
--
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 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Baolin Wang]

Hi Andy,

On 31 January 2018 at 00:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 30, 2018 at 2:07 PM, 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.
>
>> +config GPIO_SPRD
>
>> +       bool "Spreadtrum GPIO support"
>
> Either you have to put tristate here, or remove all redundant
> module_*() and MODULE_*() macros.

I will remove module_*() and MODULE_*() macros in next version. Thanks
for your comments.

>
> Other than that looks fine to me, FWIW:
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>

-- 
Baolin.wang
Best Regards

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

[Andy Shevchenko]

On Wed, Jan 31, 2018 at 4:01 AM, Baolin Wang <baolin.wang-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 31 January 2018 at 00:48, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Jan 30, 2018 at 2:07 PM, 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.
>>
>>> +config GPIO_SPRD
>>
>>> +       bool "Spreadtrum GPIO support"
>>
>> Either you have to put tristate here, or remove all redundant
>> module_*() and MODULE_*() macros.
>
> I will remove module_*() and MODULE_*() macros in next version. Thanks
> for your comments.

In that case you need to explain why driver can't be module. (And
don't forget to replace module.h with init.h).

-- 
With Best Regards,
Andy Shevchenko
--
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 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Baolin Wang]

On 31 January 2018 at 22:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Jan 31, 2018 at 4:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 31 January 2018 at 00:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Tue, Jan 30, 2018 at 2:07 PM, 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.
>>>
>>>> +config GPIO_SPRD
>>>
>>>> +       bool "Spreadtrum GPIO support"
>>>
>>> Either you have to put tristate here, or remove all redundant
>>> module_*() and MODULE_*() macros.
>>
>> I will remove module_*() and MODULE_*() macros in next version. Thanks
>> for your comments.
>
> In that case you need to explain why driver can't be module. (And
> don't forget to replace module.h with init.h).

After more investigation, I found most GPIO dependencies can be
deferred to probe. So I will change the GPIO driver to module level
and change bool to tristate in next version.

-- 
Baolin.wang
Best Regards

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

[Marcus Folkesson]

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

On Thu, Feb 01, 2018 at 11:08:46AM +0800, Baolin Wang wrote:
> On 31 January 2018 at 22:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jan 31, 2018 at 4:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> >> On 31 January 2018 at 00:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >>> On Tue, Jan 30, 2018 at 2:07 PM, 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.
> >>>
> >>>> +config GPIO_SPRD
> >>>
> >>>> +       bool "Spreadtrum GPIO support"
> >>>
> >>> Either you have to put tristate here, or remove all redundant
> >>> module_*() and MODULE_*() macros.
> >>
> >> I will remove module_*() and MODULE_*() macros in next version. Thanks
> >> for your comments.
> >
> > In that case you need to explain why driver can't be module. (And
> > don't forget to replace module.h with init.h).
> 
> After more investigation, I found most GPIO dependencies can be
> deferred to probe. So I will change the GPIO driver to module level
> and change bool to tristate in next version.

Then you could replace

+static int __init sprd_gpio_init(void)
+{
+       return platform_driver_register(&sprd_gpio_driver);
+}
+subsys_initcall(sprd_gpio_init);
+
+static void __exit sprd_gpio_exit(void)
+{
+       platform_driver_unregister(&sprd_gpio_driver);
+}
+module_exit(sprd_gpio_exit);

with

module_platform_driver(sprd_gpio_driver);

> 
> -- 
> Baolin.wang
> Best Regards


Best regards
Marcus Folkesson

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

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

[Baolin Wang]

On 1 February 2018 at 20:22, Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
> On Thu, Feb 01, 2018 at 11:08:46AM +0800, Baolin Wang wrote:
>> On 31 January 2018 at 22:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> > On Wed, Jan 31, 2018 at 4:01 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> >> On 31 January 2018 at 00:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> >>> On Tue, Jan 30, 2018 at 2:07 PM, 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.
>> >>>
>> >>>> +config GPIO_SPRD
>> >>>
>> >>>> +       bool "Spreadtrum GPIO support"
>> >>>
>> >>> Either you have to put tristate here, or remove all redundant
>> >>> module_*() and MODULE_*() macros.
>> >>
>> >> I will remove module_*() and MODULE_*() macros in next version. Thanks
>> >> for your comments.
>> >
>> > In that case you need to explain why driver can't be module. (And
>> > don't forget to replace module.h with init.h).
>>
>> After more investigation, I found most GPIO dependencies can be
>> deferred to probe. So I will change the GPIO driver to module level
>> and change bool to tristate in next version.
>
> Then you could replace
>
> +static int __init sprd_gpio_init(void)
> +{
> +       return platform_driver_register(&sprd_gpio_driver);
> +}
> +subsys_initcall(sprd_gpio_init);
> +
> +static void __exit sprd_gpio_exit(void)
> +{
> +       platform_driver_unregister(&sprd_gpio_driver);
> +}
> +module_exit(sprd_gpio_exit);
>
> with
>
> module_platform_driver(sprd_gpio_driver);

Yes, I already did this in V2 patch set, you can refer to the V2 patch
set. Thanks.

-- 
Baolin.wang
Best Regards

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

[Andy Shevchenko]

On Thu, Feb 1, 2018 at 2:22 PM, Marcus Folkesson
<marcus.folkesson-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Feb 01, 2018 at 11:08:46AM +0800, Baolin Wang wrote:

> Then you could replace

> +subsys_initcall(sprd_gpio_init);

> module_platform_driver(sprd_gpio_driver);

It's not an equivalent when built-in


-- 
With Best Regards,
Andy Shevchenko
--
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 2/2] gpio: Add GPIO driver for Spreadtrum SC9860 platform

[Marcus Folkesson]

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

Hi Baolin,

On Tue, Jan 30, 2018 at 08:07:43PM +0800, Baolin Wang 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>
> ---
> diff --git a/drivers/gpio/gpio-sprd.c b/drivers/gpio/gpio-sprd.c
> new file mode 100644
> index 0000000..af59b9f
> --- /dev/null
> +++ b/drivers/gpio/gpio-sprd.c
> @@ -0,0 +1,301 @@
> +// 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;
> +}

Better to change the function to void since the return value is not
valueable.

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

Same here


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

and here.


Thanks,
Marcus Folkesson




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

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

[Baolin Wang]

Hi Markus,

On 1 February 2018 at 20:17, Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
> Hi Baolin,
>
> On Tue, Jan 30, 2018 at 08:07:43PM +0800, Baolin Wang 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>
>> ---
>> diff --git a/drivers/gpio/gpio-sprd.c b/drivers/gpio/gpio-sprd.c
>> new file mode 100644
>> index 0000000..af59b9f
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-sprd.c
>> @@ -0,0 +1,301 @@
>> +// 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;
>> +}
>
> Better to change the function to void since the return value is not
> valueable.

The function prototype of gpio_chip structure need one return value,
so we need give one returned value though it is no meaningless.

>
>> +
>> +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;
>> +}
>
> Same here
>
>
>> +
>> +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;
>> +}
>
> and here.
>
>
> Thanks,
> Marcus Folkesson
>
>
>



-- 
Baolin.wang
Best Regards