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 <[email protected]>
---
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
+ 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
+
+#define GPI_REG_IN 0
+
+#define GPO_REG_OUT 0
+
+enum sl28cpld_gpio_type {
+ sl28cpld_gpio,
+ sl28cpld_gpi,
+ sl28cpld_gpo,
+};
+
+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, ®);
+ 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);
+
+}
+
+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);
+}
+
+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;
+}
+
+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;
+
+ 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;
+
+ /* 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")) {
+ 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},
+};
+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");
--
2.20.1
wt., 17 mar 2020 o 21:50 Michael Walle <[email protected]> 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 <[email protected]>
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, ®);
> + 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.
> --
> 2.20.1
>
Best regards,
Bartosz Golaszewski
Hi Bartosz,
Am 2020-03-18 10:14, schrieb Bartosz Golaszewski:
> wt., 17 mar 2020 o 21:50 Michael Walle <[email protected]> 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 <[email protected]>
>
> Hi Michael,
>
> thanks for the driver. Please take a look at some comments below.
well, thank you for the very fast review!
>> ---
>> 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.
ok
>
>> +
>> +enum sl28cpld_gpio_type {
>> + sl28cpld_gpio,
>> + sl28cpld_gpi,
>> + sl28cpld_gpo,
>> +};
>
> Enum values should be all upper-case.
ok
>> +
>> +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,
>> ®);
>> + 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.
ok
>
>> +}
>> +
>> +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?
TBH, I took that from gpio-pcf857x.c. But that
(1) don't uses regmap
(2) also uses that lock on other places.
I'll dig deeper into that and try to understand why there is a lock at
all and why this callback is actually called _irq_lock() because that
made me wonder.
> 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?
regmap_irqchip will register the interrupt controller on the device
which owns the regmap, ie. the parent. So (1) the phandle would need to
point to the parent device instead of the GPIO subnode and (2) I'm
already using the regmap_irqchip for the interrupt controller. I don't
know if you can actually have that multiple times.
there was a discussion which might apply partly to (1):
https://lore.kernel.org/patchwork/patch/802608/
>
>> +}
>> +
>> +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.
See above. I'd be happy to reuse the code though.
>
>> +
>> +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?
I'll change that, you're not the first one which notices that. My reason
was to have the check together with the dev_get_regmap() which uses the
parent, expecting that the error case only happen exceptionally.
>
>> +
>> + 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?
Mh are you sure? The blueprint for this were the regulators in
drivers/regulators/, eg the wm831x-ldo.c. IORESOURCE_REG isn't used
that often. But here is what I want to achieve (for which I haven't
found any existing drivers for now):
(1) the individual blocks of the overall sl28cpld may be used
multiple times, eg. this driver only has the offset to a
base address. So if there are two blocks, this mfd core
driver will register two devices for this driver with
different base offsets, which are passed by IORESOURCE_REG
(2) I wanted to avoid having a private mfd include with some
kind of "proprietary" method how to get that offset
(3) the mfd core driver is the one knowing the offset, thus it
is possible to have different flavours of the sl28cpld
>
>> + /* 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.
ok
>
>> + 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?
These are actually three different components. Ie. you could have a
flavour where you have one GPIO (sl28cpld-gpio) and two output-only
ones (sl28cpld-gpo). Is that what you wanted to know?
>
>> +};
>> +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.
ok, I'll look into that.
thanks,
-michael
>
>> --
>> 2.20.1
>>
>
> Best regards,
> Bartosz Golaszewski
śr., 18 mar 2020 o 13:45 Michael Walle <[email protected]> napisał(a):
>
> Hi Bartosz,
>
> Am 2020-03-18 10:14, schrieb Bartosz Golaszewski:
> > wt., 17 mar 2020 o 21:50 Michael Walle <[email protected]> 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 <[email protected]>
> >
> > Hi Michael,
> >
> > thanks for the driver. Please take a look at some comments below.
>
> well, thank you for the very fast review!
>
> >> ---
> >> 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.
>
> ok
>
> >
> >> +
> >> +enum sl28cpld_gpio_type {
> >> + sl28cpld_gpio,
> >> + sl28cpld_gpi,
> >> + sl28cpld_gpo,
> >> +};
> >
> > Enum values should be all upper-case.
>
> ok
>
> >> +
> >> +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,
> >> ®);
> >> + 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.
> ok
>
> >
> >> +}
> >> +
> >> +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?
>
> TBH, I took that from gpio-pcf857x.c. But that
> (1) don't uses regmap
> (2) also uses that lock on other places.
>
> I'll dig deeper into that and try to understand why there is a lock at
> all and why this callback is actually called _irq_lock() because that
> made me wonder.
>
> > 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?
>
> regmap_irqchip will register the interrupt controller on the device
> which owns the regmap, ie. the parent. So (1) the phandle would need to
> point to the parent device instead of the GPIO subnode and (2) I'm
> already using the regmap_irqchip for the interrupt controller. I don't
> know if you can actually have that multiple times.
>
> there was a discussion which might apply partly to (1):
> https://lore.kernel.org/patchwork/patch/802608/
>
In that case maybe you should use the disable_locking option in
regmap_config and provide your own callbacks that you can use in the
irqchip code too?
> >
> >> +}
> >> +
> >> +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.
>
> See above. I'd be happy to reuse the code though.
>
> >
> >> +
> >> +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?
>
> I'll change that, you're not the first one which notices that. My reason
> was to have the check together with the dev_get_regmap() which uses the
> parent, expecting that the error case only happen exceptionally.
>
> >
> >> +
> >> + 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?
>
> Mh are you sure? The blueprint for this were the regulators in
> drivers/regulators/, eg the wm831x-ldo.c. IORESOURCE_REG isn't used
> that often. But here is what I want to achieve (for which I haven't
> found any existing drivers for now):
> (1) the individual blocks of the overall sl28cpld may be used
> multiple times, eg. this driver only has the offset to a
> base address. So if there are two blocks, this mfd core
> driver will register two devices for this driver with
> different base offsets, which are passed by IORESOURCE_REG
> (2) I wanted to avoid having a private mfd include with some
> kind of "proprietary" method how to get that offset
> (3) the mfd core driver is the one knowing the offset, thus it
> is possible to have different flavours of the sl28cpld
>
Ok, now I see it's documented in the bindings. Thanks for the explanation.
>
> >
> >> + /* 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.
>
> ok
>
>
> >
> >> + 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?
>
> These are actually three different components. Ie. you could have a
> flavour where you have one GPIO (sl28cpld-gpio) and two output-only
> ones (sl28cpld-gpo). Is that what you wanted to know?
>
Yes, thanks. This could use some documentation in the bindings though.
Bartosz
> >
> >> +};
> >> +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.
>
> ok, I'll look into that.
>
> thanks,
> -michael
>
> >
> >> --
> >> 2.20.1
> >>
> >
> > Best regards,
> > Bartosz Golaszewski
Hi Bartosz,
Am 2020-03-25 12:50, schrieb Bartosz Golaszewski:
> śr., 18 mar 2020 o 13:45 Michael Walle <[email protected]> napisał(a):
>>
>> Hi Bartosz,
>>
>> Am 2020-03-18 10:14, schrieb Bartosz Golaszewski:
>> > wt., 17 mar 2020 o 21:50 Michael Walle <[email protected]> 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 <[email protected]>
>> >
>> > Hi Michael,
>> >
>> > thanks for the driver. Please take a look at some comments below.
>>
>> well, thank you for the very fast review!
>>
>> >> ---
>> >> 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.
>>
>> ok
>>
>> >
>> >> +
>> >> +enum sl28cpld_gpio_type {
>> >> + sl28cpld_gpio,
>> >> + sl28cpld_gpi,
>> >> + sl28cpld_gpo,
>> >> +};
>> >
>> > Enum values should be all upper-case.
>>
>> ok
>>
>> >> +
>> >> +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,
>> >> ®);
>> >> + 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.
>> ok
>>
>> >
>> >> +}
>> >> +
>> >> +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?
>>
>> TBH, I took that from gpio-pcf857x.c. But that
>> (1) don't uses regmap
>> (2) also uses that lock on other places.
>>
>> I'll dig deeper into that and try to understand why there is a lock at
>> all and why this callback is actually called _irq_lock() because that
>> made me wonder.
>>
>> > 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?
>>
>> regmap_irqchip will register the interrupt controller on the device
>> which owns the regmap, ie. the parent. So (1) the phandle would need
>> to
>> point to the parent device instead of the GPIO subnode and (2) I'm
>> already using the regmap_irqchip for the interrupt controller. I don't
>> know if you can actually have that multiple times.
>>
>> there was a discussion which might apply partly to (1):
>> https://lore.kernel.org/patchwork/patch/802608/
>>
>
> In that case maybe you should use the disable_locking option in
> regmap_config and provide your own callbacks that you can use in the
> irqchip code too?
But how would that solve problem (1). And keep in mind, that the
reqmap_irqchip is actually used for the interrupt controller, which
is not this gpio controller.
Ie. the interrupt controller of the sl28cpld uses the regmap_irqchip
and all interrupt phandles pointing to the interrupt controller will
reference the toplevel node. Any phandles pointing to the gpio
controller will reference the GPIO subnode.
>
>> >
>> >> +}
>> >> +
>> >> +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.
>>
>> See above. I'd be happy to reuse the code though.
>>
>> >
>> >> +
>> >> +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?
>>
>> I'll change that, you're not the first one which notices that. My
>> reason
>> was to have the check together with the dev_get_regmap() which uses
>> the
>> parent, expecting that the error case only happen exceptionally.
>>
>> >
>> >> +
>> >> + 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?
>>
>> Mh are you sure? The blueprint for this were the regulators in
>> drivers/regulators/, eg the wm831x-ldo.c. IORESOURCE_REG isn't used
>> that often. But here is what I want to achieve (for which I haven't
>> found any existing drivers for now):
>> (1) the individual blocks of the overall sl28cpld may be used
>> multiple times, eg. this driver only has the offset to a
>> base address. So if there are two blocks, this mfd core
>> driver will register two devices for this driver with
>> different base offsets, which are passed by IORESOURCE_REG
>> (2) I wanted to avoid having a private mfd include with some
>> kind of "proprietary" method how to get that offset
>> (3) the mfd core driver is the one knowing the offset, thus it
>> is possible to have different flavours of the sl28cpld
>>
>
> Ok, now I see it's documented in the bindings. Thanks for the
> explanation.
>
>>
>> >
>> >> + /* 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.
>>
>> ok
>>
>>
>> >
>> >> + 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?
>>
>> These are actually three different components. Ie. you could have a
>> flavour where you have one GPIO (sl28cpld-gpio) and two output-only
>> ones (sl28cpld-gpo). Is that what you wanted to know?
>>
>
> Yes, thanks. This could use some documentation in the bindings though.
ok. thats easy ;)
-michael
> Bartosz
>
>> >
>> >> +};
>> >> +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.
>>
>> ok, I'll look into that.
>>
>> thanks,
>> -michael
>>
>> >
>> >> --
>> >> 2.20.1
>> >>
>> >
>> > Best regards,
>> > Bartosz Golaszewski
On Thu, Mar 26, 2020 at 9:06 PM Michael Walle <[email protected]> wrote:
> Am 2020-03-25 12:50, schrieb Bartosz Golaszewski:
> > In that case maybe you should use the disable_locking option in
> > regmap_config and provide your own callbacks that you can use in the
> > irqchip code too?
>
> But how would that solve problem (1). And keep in mind, that the
> reqmap_irqchip is actually used for the interrupt controller, which
> is not this gpio controller.
>
> Ie. the interrupt controller of the sl28cpld uses the regmap_irqchip
> and all interrupt phandles pointing to the interrupt controller will
> reference the toplevel node. Any phandles pointing to the gpio
> controller will reference the GPIO subnode.
Ideally we would create something generic that has been on my
mind for some time, like a generic GPIO regmap irqchip now that
there are a few controllers like that.
I don't know how feasible it is or how much work it would be. But
as with GPIO_GENERIC (for MMIO) it would be helpful since we
can then implement things like .set_multiple() and .get_multiple()
for everyone.
Yours,
Linus Walleij
Am 2020-03-27 11:20, schrieb Linus Walleij:
> On Thu, Mar 26, 2020 at 9:06 PM Michael Walle <[email protected]> wrote:
>> Am 2020-03-25 12:50, schrieb Bartosz Golaszewski:
>
>> > In that case maybe you should use the disable_locking option in
>> > regmap_config and provide your own callbacks that you can use in the
>> > irqchip code too?
>>
>> But how would that solve problem (1). And keep in mind, that the
>> reqmap_irqchip is actually used for the interrupt controller, which
>> is not this gpio controller.
>>
>> Ie. the interrupt controller of the sl28cpld uses the regmap_irqchip
>> and all interrupt phandles pointing to the interrupt controller will
>> reference the toplevel node. Any phandles pointing to the gpio
>> controller will reference the GPIO subnode.
>
> Ideally we would create something generic that has been on my
> mind for some time, like a generic GPIO regmap irqchip now that
> there are a few controllers like that.
>
> I don't know how feasible it is or how much work it would be. But
> as with GPIO_GENERIC (for MMIO) it would be helpful since we
> can then implement things like .set_multiple() and .get_multiple()
> for everyone.
For starters, would that be a drivers/gpio/gpio-regmap.c or a
drivers/base/regmap/regmap-gpio.c? I would assume the first,
because the stuff in drivers/base/regmap operates on a given
regmap and we'd just be using one, correct? On the other hand
there is also the reqmap-irq.c. But as pointed out before, it
will add an interrupt controller to the regmap, not a device
so to speak.
-michael
On Fri, Mar 27, 2020 at 4:28 PM Michael Walle <[email protected]> wrote:
> For starters, would that be a drivers/gpio/gpio-regmap.c or a
> drivers/base/regmap/regmap-gpio.c? I would assume the first,
Yeah I would name it like that. gpio-regmap.c.
Yours,
Linus Walleij
Hi Bartosz,
Am 2020-03-18 10:14, schrieb Bartosz Golaszewski:
> wt., 17 mar 2020 o 21:50 Michael Walle <[email protected]> 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 <[email protected]>
>
> Hi Michael,
>
> thanks for the driver. Please take a look at some comments below.
>
>> ---
[..]
>> +#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.
The GPIO_, GPI_ and GPO_ prefixes corresponds to the different
flavours. Do they still need a common prefix? Ie. the GPI_REG_IN
has nothing to do with GPO_REG_OUT, nor has both something
to do with the GPIO_REG_IN and GPIO_REG_OUT. I could prefix them
with SL28CPLD_ though. But I don't know if that is what you had
in mind because then they would be SL28CPLD_GPIO_REG_IN and
SL28CPLD_GPI_REG_IN for example.
-michael
Am 2020-03-18 10:14, schrieb Bartosz Golaszewski:
> wt., 17 mar 2020 o 21:50 Michael Walle <[email protected]> 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 <[email protected]>
>
> 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,
>> ®);
>> + 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
pt., 27 mar 2020 o 16:28 Michael Walle <[email protected]> napisał(a):
>
> Am 2020-03-27 11:20, schrieb Linus Walleij:
> > On Thu, Mar 26, 2020 at 9:06 PM Michael Walle <[email protected]> wrote:
> >> Am 2020-03-25 12:50, schrieb Bartosz Golaszewski:
> >
> >> > In that case maybe you should use the disable_locking option in
> >> > regmap_config and provide your own callbacks that you can use in the
> >> > irqchip code too?
> >>
> >> But how would that solve problem (1). And keep in mind, that the
> >> reqmap_irqchip is actually used for the interrupt controller, which
> >> is not this gpio controller.
> >>
> >> Ie. the interrupt controller of the sl28cpld uses the regmap_irqchip
> >> and all interrupt phandles pointing to the interrupt controller will
> >> reference the toplevel node. Any phandles pointing to the gpio
> >> controller will reference the GPIO subnode.
> >
> > Ideally we would create something generic that has been on my
> > mind for some time, like a generic GPIO regmap irqchip now that
> > there are a few controllers like that.
> >
> > I don't know how feasible it is or how much work it would be. But
> > as with GPIO_GENERIC (for MMIO) it would be helpful since we
> > can then implement things like .set_multiple() and .get_multiple()
> > for everyone.
>
> For starters, would that be a drivers/gpio/gpio-regmap.c or a
> drivers/base/regmap/regmap-gpio.c? I would assume the first,
> because the stuff in drivers/base/regmap operates on a given
> regmap and we'd just be using one, correct? On the other hand
> there is also the reqmap-irq.c. But as pointed out before, it
> will add an interrupt controller to the regmap, not a device
> so to speak.
>
> -michael
This has been on my TODO list for so long, but I've never been able to
find the time... I'd really appreciate any effort in that direction as
I believe it would allow us to slowly port a big part of the GPIO
expander drivers over to it and make large portions of our codebase
generic.
Best regards,
Bartosz Golaszewski
Hi,
Am 2020-03-30 13:21, schrieb Bartosz Golaszewski:
> pt., 27 mar 2020 o 16:28 Michael Walle <[email protected]> napisał(a):
>>
>> Am 2020-03-27 11:20, schrieb Linus Walleij:
>> > On Thu, Mar 26, 2020 at 9:06 PM Michael Walle <[email protected]> wrote:
>> >> Am 2020-03-25 12:50, schrieb Bartosz Golaszewski:
>> >
>> >> > In that case maybe you should use the disable_locking option in
>> >> > regmap_config and provide your own callbacks that you can use in the
>> >> > irqchip code too?
>> >>
>> >> But how would that solve problem (1). And keep in mind, that the
>> >> reqmap_irqchip is actually used for the interrupt controller, which
>> >> is not this gpio controller.
>> >>
>> >> Ie. the interrupt controller of the sl28cpld uses the regmap_irqchip
>> >> and all interrupt phandles pointing to the interrupt controller will
>> >> reference the toplevel node. Any phandles pointing to the gpio
>> >> controller will reference the GPIO subnode.
>> >
>> > Ideally we would create something generic that has been on my
>> > mind for some time, like a generic GPIO regmap irqchip now that
>> > there are a few controllers like that.
>> >
>> > I don't know how feasible it is or how much work it would be. But
>> > as with GPIO_GENERIC (for MMIO) it would be helpful since we
>> > can then implement things like .set_multiple() and .get_multiple()
>> > for everyone.
>>
>> For starters, would that be a drivers/gpio/gpio-regmap.c or a
>> drivers/base/regmap/regmap-gpio.c? I would assume the first,
>> because the stuff in drivers/base/regmap operates on a given
>> regmap and we'd just be using one, correct? On the other hand
>> there is also the reqmap-irq.c. But as pointed out before, it
>> will add an interrupt controller to the regmap, not a device
>> so to speak.
>>
>> -michael
>
> This has been on my TODO list for so long, but I've never been able to
> find the time... I'd really appreciate any effort in that direction as
> I believe it would allow us to slowly port a big part of the GPIO
> expander drivers over to it and make large portions of our codebase
> generic.
I might have at least a first patch this week. So if you and Linus
Walleij might have some time to help review and comment on that, it
would be greatly appreciated.
-michael
>
> Best regards,
> Bartosz Golaszewski