| Message ID | 20200317205017.28280-13-michael@walle.cc (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
wt., 17 mar 2020 o 21:50 Michael Walle <michael@walle.cc> 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@walle.cc> 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 <michael@walle.cc> 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@walle.cc> > > 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 <michael@walle.cc> napisał(a): > > Hi Bartosz, > > Am 2020-03-18 10:14, schrieb Bartosz Golaszewski: > > wt., 17 mar 2020 o 21:50 Michael Walle <michael@walle.cc> 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@walle.cc> > > > > 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 <michael@walle.cc> napisał(a): >> >> Hi Bartosz, >> >> Am 2020-03-18 10:14, schrieb Bartosz Golaszewski: >> > wt., 17 mar 2020 o 21:50 Michael Walle <michael@walle.cc> 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@walle.cc> >> > >> > 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 <michael@walle.cc> 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 <michael@walle.cc> 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 <michael@walle.cc> 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 <michael@walle.cc> 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@walle.cc> > > 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 <michael@walle.cc> 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@walle.cc> > > 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 <michael@walle.cc> napisał(a): > > Am 2020-03-27 11:20, schrieb Linus Walleij: > > On Thu, Mar 26, 2020 at 9:06 PM Michael Walle <michael@walle.cc> 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 <michael@walle.cc> napisał(a): >> >> Am 2020-03-27 11:20, schrieb Linus Walleij: >> > On Thu, Mar 26, 2020 at 9:06 PM Michael Walle <michael@walle.cc> 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
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");
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@walle.cc> --- 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