Re: [PATCH 12/18] gpio: add support for the sl28cpld GPIO controller

From: Michael Walle
Date: Sat Mar 28 2020 - 13:20:26 EST

Next message: Andreas Kemnade: "Re: CHKDT error by f95cad74acdb ("dt-bindings: clocks: Convert Allwinner legacy clocks to schemas")"
Previous message: Daniel Borkmann: "Re: [PATCH bpf-next v8 0/8] MAC and Audit policy using eBPF (KRSI)"
In reply to: Michael Walle: "Re: [PATCH 12/18] gpio: add support for the sl28cpld GPIO controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Am 2020-03-18 10:14, schrieb Bartosz Golaszewski:

wt., 17 mar 2020 o 21:50 Michael Walle <michael@xxxxxxxx> napisaÅ(a):

This adds support for the GPIO controller of the sl28 board management
controller. This driver is part of a multi-function device.

Signed-off-by: Michael Walle <michael@xxxxxxxx>

Hi Michael,

thanks for the driver. Please take a look at some comments below.

---
drivers/gpio/Kconfig | 11 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-sl28cpld.c | 332 +++++++++++++++++++++++++++++++++++
3 files changed, 344 insertions(+)
create mode 100644 drivers/gpio/gpio-sl28cpld.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3cbf8882a0dd..516e47017ef5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1211,6 +1211,17 @@ config GPIO_RC5T583
This driver provides the support for driving/reading the gpio pins
of RC5T583 device through standard gpio library.

+config GPIO_SL28CPLD
+ tristate "Kontron sl28 GPIO"
+ depends on MFD_SL28CPLD
+ depends on OF_GPIO
+ select GPIOLIB_IRQCHIP

Please see below - I think both are not needed.

+ help
+ This enables support for the GPIOs found on the Kontron sl28 CPLD.
+
+ This driver can also be built as a module. If so, the module will be
+ called gpio-sl28cpld.
+
config GPIO_STMPE
bool "STMPE GPIOs"
depends on MFD_STMPE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 0b571264ddbc..0ca2d52c78e8 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -127,6 +127,7 @@ obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o
obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o
obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o
+obj-$(CONFIG_GPIO_SL28CPLD) += gpio-sl28cpld.o
obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o
obj-$(CONFIG_GPIO_SPEAR_SPICS) += gpio-spear-spics.o
obj-$(CONFIG_GPIO_SPRD) += gpio-sprd.o
diff --git a/drivers/gpio/gpio-sl28cpld.c b/drivers/gpio/gpio-sl28cpld.c
new file mode 100644
index 000000000000..94f82013882f
--- /dev/null
+++ b/drivers/gpio/gpio-sl28cpld.c
@@ -0,0 +1,332 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * SMARC-sAL28 GPIO driver.
+ *
+ * Copyright 2019 Kontron Europe GmbH
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+#include <linux/gpio/driver.h>
+
+#define GPIO_REG_DIR 0
+#define GPIO_REG_OUT 1
+#define GPIO_REG_IN 2
+#define GPIO_REG_IE 3
+#define GPIO_REG_IP 4

These values would be more clear if they were defined as hex.

+
+#define GPI_REG_IN 0
+
+#define GPO_REG_OUT 0

Please also use a common prefix even for defines.

+
+enum sl28cpld_gpio_type {
+ sl28cpld_gpio,
+ sl28cpld_gpi,
+ sl28cpld_gpo,
+};

Enum values should be all upper-case.

+
+struct sl28cpld_gpio {
+ struct gpio_chip gpio_chip;
+ struct irq_chip irq_chip;
+ struct regmap *regmap;
+ u32 offset;
+ struct mutex lock;
+ u8 ie;
+};
+
+static void sl28cpld_gpio_set_reg(struct gpio_chip *chip, unsigned int reg,
+ unsigned int offset, int value)
+{
+ struct sl28cpld_gpio *gpio = gpiochip_get_data(chip);
+ unsigned int mask = 1 << offset;
+ unsigned int val = value << offset;
+
+ regmap_update_bits(gpio->regmap, gpio->offset + reg, mask, val);
+}
+
+static void sl28cpld_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ sl28cpld_gpio_set_reg(chip, GPIO_REG_OUT, offset, value);
+}
+
+static void sl28cpld_gpo_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ sl28cpld_gpio_set_reg(chip, GPO_REG_OUT, offset, value);
+}
+
+static int sl28cpld_gpio_get_reg(struct gpio_chip *chip, unsigned int reg,
+ unsigned int offset)
+{
+ struct sl28cpld_gpio *gpio = gpiochip_get_data(chip);
+ unsigned int mask = 1 << offset;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(gpio->regmap, gpio->offset + reg, &val);
+ if (ret)
+ return ret;
+
+ return (val & mask) ? 1 : 0;
+}
+
+static int sl28cpld_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ return sl28cpld_gpio_get_reg(chip, GPIO_REG_IN, offset);
+}
+
+static int sl28cpld_gpi_get(struct gpio_chip *chip, unsigned int offset)
+{
+ return sl28cpld_gpio_get_reg(chip, GPI_REG_IN, offset);
+}
+
+static int sl28cpld_gpio_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct sl28cpld_gpio *gpio = gpiochip_get_data(chip);
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(gpio->regmap, gpio->offset + GPIO_REG_DIR, &reg);
+ if (ret)
+ return ret;
+
+ if (reg & (1 << offset))
+ return GPIO_LINE_DIRECTION_OUT;
+ else
+ return GPIO_LINE_DIRECTION_IN;
+}
+
+static int sl28cpld_gpio_set_direction(struct gpio_chip *chip,
+ unsigned int offset,
+ bool output)
+{
+ struct sl28cpld_gpio *gpio = gpiochip_get_data(chip);
+ unsigned int mask = 1 << offset;
+ unsigned int val = (output) ? mask : 0;
+
+ return regmap_update_bits(gpio->regmap, gpio->offset + GPIO_REG_DIR,
+ mask, val);
+

Stray newline.

+}
+
+static int sl28cpld_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ return sl28cpld_gpio_set_direction(chip, offset, false);
+}
+
+static int sl28cpld_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ sl28cpld_gpio_set_reg(chip, GPIO_REG_OUT, offset, value);
+ return sl28cpld_gpio_set_direction(chip, offset, true);
+}
+
+static void sl28cpld_gpio_irq_lock(struct irq_data *data)
+{
+ struct sl28cpld_gpio *gpio =
+ gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+ mutex_lock(&gpio->lock);

How does that actually lock anything? Regmap uses a different lock and
if you want to make sure nobody modifies the GPIO registers than you'd
need to use the same lock. Also: this looks a lot like a task for
regmap_irqchip - maybe you could use it here or in the core mfd
module?

+}
+
+static void sl28cpld_gpio_irq_sync_unlock(struct irq_data *data)
+{
+ struct sl28cpld_gpio *gpio =
+ gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+ regmap_write(gpio->regmap, gpio->offset + GPIO_REG_IE, gpio->ie);
+ mutex_unlock(&gpio->lock);
+}
+
+static void sl28cpld_gpio_irq_disable(struct irq_data *data)
+{
+ struct sl28cpld_gpio *gpio =
+ gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+ if (data->hwirq >= 8)
+ return;
+
+ gpio->ie &= ~(1 << data->hwirq);
+}
+
+static void sl28cpld_gpio_irq_enable(struct irq_data *data)
+{
+ struct sl28cpld_gpio *gpio =
+ gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
+ if (data->hwirq >= 8)
+ return;
+
+ gpio->ie |= (1 << data->hwirq);
+}
+
+static int sl28cpld_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ /* only edge triggered interrupts on both edges are supported */
+ return (type == IRQ_TYPE_EDGE_BOTH) ? 0 : -EINVAL;
+}
+
+static irqreturn_t sl28cpld_gpio_irq_thread(int irq, void *data)
+{
+ struct sl28cpld_gpio *gpio = data;
+ unsigned int ip;
+ unsigned int virq;
+ int pin;
+ int ret;
+
+ ret = regmap_read(gpio->regmap, gpio->offset + GPIO_REG_IP, &ip);
+ if (ret)
+ return IRQ_NONE;
+
+ /* mask other pending interrupts which are not enabled */
+ ip &= gpio->ie;
+
+ /* ack the interrupts */
+ regmap_write(gpio->regmap, gpio->offset + GPIO_REG_IP, ip);
+
+ /* and handle them */
+ while (ip) {
+ pin = __ffs(ip);
+ ip &= ~BIT(pin);
+
+ virq = irq_find_mapping(gpio->gpio_chip.irq.domain, pin);
+ if (virq)
+ handle_nested_irq(virq);
+ }
+
+ return IRQ_HANDLED;
+}

This definitely looks like parts of regmap_irqchip reimplemented.
Please check if you could reuse it - it would save a lot of code.

+
+static int sl28_cpld_gpio_irq_init(struct platform_device *pdev, int irq)
+{
+ struct sl28cpld_gpio *gpio = platform_get_drvdata(pdev);
+ struct irq_chip *irq_chip = &gpio->irq_chip;
+ int ret;
+
+ irq_chip->name = "sl28cpld-gpio-irq",
+ irq_chip->irq_bus_lock = sl28cpld_gpio_irq_lock,
+ irq_chip->irq_bus_sync_unlock = sl28cpld_gpio_irq_sync_unlock,
+ irq_chip->irq_disable = sl28cpld_gpio_irq_disable,
+ irq_chip->irq_enable = sl28cpld_gpio_irq_enable,
+ irq_chip->irq_set_type = sl28cpld_gpio_irq_set_type,
+ irq_chip->flags = IRQCHIP_SKIP_SET_WAKE,
+
+ ret = gpiochip_irqchip_add_nested(&gpio->gpio_chip, irq_chip, 0,
+ handle_simple_irq, IRQ_TYPE_NONE);
+ if (ret)
+ return ret;
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ sl28cpld_gpio_irq_thread,
+ IRQF_SHARED | IRQF_ONESHOT,
+ pdev->name, gpio);
+ if (ret)
+ return ret;
+
+ gpiochip_set_nested_irqchip(&gpio->gpio_chip, irq_chip, irq);
+
+ return 0;
+}
+
+static int sl28cpld_gpio_probe(struct platform_device *pdev)
+{
+ enum sl28cpld_gpio_type type =
+ platform_get_device_id(pdev)->driver_data;
+ struct device_node *np = pdev->dev.of_node;
+ struct sl28cpld_gpio *gpio;
+ struct gpio_chip *chip;
+ struct resource *res;
+ bool irq_support = false;
+ int ret;
+ int irq;
+
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ if (!pdev->dev.parent)
+ return -ENODEV;

Why not check this before allocating any memory?

+
+ gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!gpio->regmap)
+ return -ENODEV;
+
+ res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+ if (!res)
+ return -EINVAL;
+ gpio->offset = res->start;
+

This isn't how IO resources are used. What are you trying to achieve here?

+ /* initialize struct gpio_chip */
+ mutex_init(&gpio->lock);
+ chip = &gpio->gpio_chip;
+ chip->parent = &pdev->dev;
+ chip->label = dev_name(&pdev->dev);
+ chip->owner = THIS_MODULE;
+ chip->can_sleep = true;
+ chip->base = -1;
+ chip->ngpio = 8;
+
+ switch (type) {
+ case sl28cpld_gpio:
+ chip->get_direction = sl28cpld_gpio_get_direction;
+ chip->direction_input = sl28cpld_gpio_direction_input;
+ chip->direction_output = sl28cpld_gpio_direction_output;
+ chip->get = sl28cpld_gpio_get;
+ chip->set = sl28cpld_gpio_set;
+ irq_support = true;
+ break;
+ case sl28cpld_gpo:
+ chip->set = sl28cpld_gpo_set;
+ chip->get = sl28cpld_gpi_get;
+ break;
+ case sl28cpld_gpi:
+ chip->get = sl28cpld_gpi_get;
+ break;
+ }
+
+ ret = devm_gpiochip_add_data(&pdev->dev, chip, gpio);
+ if (ret < 0)
+ return ret;
+
+ platform_set_drvdata(pdev, gpio);
+
+ if (irq_support && of_property_read_bool(np, "interrupt-controller")) {

You're depending on OF_GPIO for this one function. Please switch to
device_property_read_bool() instead.

+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return ret;
+
+ ret = sl28_cpld_gpio_irq_init(pdev, irq);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id sl28cpld_gpio_id_table[] = {
+ {"sl28cpld-gpio", sl28cpld_gpio},
+ {"sl28cpld-gpi", sl28cpld_gpi},
+ {"sl28cpld-gpo", sl28cpld_gpo},

Could you explain this a bit more? Is this the same component with
input/output-only lines or three different components?

+};
+MODULE_DEVICE_TABLE(platform, sl28cpld_gpio_id_table);
+
+static struct platform_driver sl28cpld_gpio_driver = {
+ .probe = sl28cpld_gpio_probe,
+ .id_table = sl28cpld_gpio_id_table,
+ .driver = {
+ .name = "sl28cpld-gpio",
+ },
+};
+module_platform_driver(sl28cpld_gpio_driver);
+
+MODULE_DESCRIPTION("sl28cpld GPIO Driver");
+MODULE_LICENSE("GPL");

I think you could use a MODULE_ALIAS() here if you want this module to
be loaded automatically by udev.

Turns out MODULE_ALIAS("platform:..") isn't working with mfd and
OF_MFD_CELL(), because the match is done on "of:.." module aliases.
So I guess I'll need a of_device_id array after all, although the
matching for the mfd is via the platform driver name.

-michael

Next message: Andreas Kemnade: "Re: CHKDT error by f95cad74acdb ("dt-bindings: clocks: Convert Allwinner legacy clocks to schemas")"
Previous message: Daniel Borkmann: "Re: [PATCH bpf-next v8 0/8] MAC and Audit policy using eBPF (KRSI)"
In reply to: Michael Walle: "Re: [PATCH 12/18] gpio: add support for the sl28cpld GPIO controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]