| Message ID | 20200423174543.17161-7-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
Michael Walle <michael@walle.cc> writes: > This patch adds support for the interrupt controller inside the sl28 git grep 'This patch' Documentation/process/ > CPLD management controller. > > +static int sl28cpld_intc_probe(struct platform_device *pdev) > +{ > + struct sl28cpld_intc *irqchip; > + struct resource *res; > + unsigned int irq; > + int ret; > + > + if (!pdev->dev.parent) > + return -ENODEV; > + > + irqchip = devm_kzalloc(&pdev->dev, sizeof(*irqchip), GFP_KERNEL); > + if (!irqchip) > + return -ENOMEM; > + > + irqchip->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!irqchip->regmap) > + return -ENODEV; > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + res = platform_get_resource(pdev, IORESOURCE_REG, 0); > + if (!res) > + return -EINVAL; > + > + irqchip->chip.name = "sl28cpld-intc"; > + irqchip->chip.irqs = sl28cpld_irqs; > + irqchip->chip.num_irqs = ARRAY_SIZE(sl28cpld_irqs); > + irqchip->chip.num_regs = 1; > + irqchip->chip.status_base = res->start + INTC_IP; > + irqchip->chip.mask_base = res->start + INTC_IE; > + irqchip->chip.mask_invert = true, > + irqchip->chip.ack_base = res->start + INTC_IP; > + > + ret = devm_regmap_add_irq_chip(&pdev->dev, irqchip->regmap, irq, > + IRQF_SHARED | IRQF_ONESHOT, 0, What's the point of IRQF_SHARED | IRQF_ONESHOT here? > + &irqchip->chip, &irqchip->irq_data); Thanks, tglx
Hi Thomas, thanks for the review. Am 2020-04-27 13:40, schrieb Thomas Gleixner: > Michael Walle <michael@walle.cc> writes: > >> This patch adds support for the interrupt controller inside the sl28 > > git grep 'This patch' Documentation/process/ ok. > >> CPLD management controller. >> >> +static int sl28cpld_intc_probe(struct platform_device *pdev) >> +{ >> + struct sl28cpld_intc *irqchip; >> + struct resource *res; >> + unsigned int irq; >> + int ret; >> + >> + if (!pdev->dev.parent) >> + return -ENODEV; >> + >> + irqchip = devm_kzalloc(&pdev->dev, sizeof(*irqchip), GFP_KERNEL); >> + if (!irqchip) >> + return -ENOMEM; >> + >> + irqchip->regmap = dev_get_regmap(pdev->dev.parent, NULL); >> + if (!irqchip->regmap) >> + return -ENODEV; >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; >> + >> + res = platform_get_resource(pdev, IORESOURCE_REG, 0); >> + if (!res) >> + return -EINVAL; >> + >> + irqchip->chip.name = "sl28cpld-intc"; >> + irqchip->chip.irqs = sl28cpld_irqs; >> + irqchip->chip.num_irqs = ARRAY_SIZE(sl28cpld_irqs); >> + irqchip->chip.num_regs = 1; >> + irqchip->chip.status_base = res->start + INTC_IP; >> + irqchip->chip.mask_base = res->start + INTC_IE; >> + irqchip->chip.mask_invert = true, >> + irqchip->chip.ack_base = res->start + INTC_IP; >> + >> + ret = devm_regmap_add_irq_chip(&pdev->dev, irqchip->regmap, irq, >> + IRQF_SHARED | IRQF_ONESHOT, 0, > > What's the point of IRQF_SHARED | IRQF_ONESHOT here? IRQF_SHARED because this interrupt is shared with all the blocks which can generate interrupts, i.e. the GPIO contollers. IRQF_ONESHOT, because its is a threaded interrupt with no primary handler. But I just noticed, that regmap-irq will also set the IRQF_ONESHOT. But that the commit 09cadf6e088b ("regmap-irq: set IRQF_ONESHOT flag to ensure IRQ request") reads like it is just there to be sure. So I don't know if it should also be set here. -michael > >> + &irqchip->chip, &irqchip->irq_data); > > Thanks, > > tglx
On Mon, Apr 27, 2020 at 07:40:11PM +0200, Michael Walle wrote: > IRQF_ONESHOT, because its is a threaded interrupt with no primary > handler. But I just noticed, that regmap-irq will also set the > IRQF_ONESHOT. But that the commit 09cadf6e088b ("regmap-irq: > set IRQF_ONESHOT flag to ensure IRQ request") reads like it is > just there to be sure. So I don't know if it should also be set > here. Looking at the changelog there the "we can't be sure" bit is that coccinelle couldn't follow the flags through from the caller to make sure that IRQF_ONESHOT is set so we're just oring it in unconditionally.
Am 2020-04-27 19:44, schrieb Mark Brown: > On Mon, Apr 27, 2020 at 07:40:11PM +0200, Michael Walle wrote: > >> IRQF_ONESHOT, because its is a threaded interrupt with no primary >> handler. But I just noticed, that regmap-irq will also set the >> IRQF_ONESHOT. But that the commit 09cadf6e088b ("regmap-irq: >> set IRQF_ONESHOT flag to ensure IRQ request") reads like it is >> just there to be sure. So I don't know if it should also be set >> here. > > Looking at the changelog there the "we can't be sure" bit is that > coccinelle couldn't follow the flags through from the caller to make > sure that IRQF_ONESHOT is set so we're just oring it in > unconditionally. So it is correct that IRQF_ONESHOT is also set in the driver which is using regmap_add_irq_chip(), right? -michael
On Mon, Apr 27, 2020 at 08:01:14PM +0200, Michael Walle wrote: > Am 2020-04-27 19:44, schrieb Mark Brown: > > Looking at the changelog there the "we can't be sure" bit is that > > coccinelle couldn't follow the flags through from the caller to make > > sure that IRQF_ONESHOT is set so we're just oring it in unconditionally. > So it is correct that IRQF_ONESHOT is also set in the driver which is > using regmap_add_irq_chip(), right? It shouldn't break anything and my instinct is that it's better form.
Michael, Michael Walle <michael@walle.cc> writes: > Am 2020-04-27 13:40, schrieb Thomas Gleixner: >>> + >>> + ret = devm_regmap_add_irq_chip(&pdev->dev, irqchip->regmap, irq, >>> + IRQF_SHARED | IRQF_ONESHOT, 0, >> >> What's the point of IRQF_SHARED | IRQF_ONESHOT here? > > IRQF_SHARED because this interrupt is shared with all the blocks > which can generate interrupts, i.e. the GPIO contollers. Why are people still designing hardware with shared interrupts? Shared interrupts are broken by design and that's well known for decades. > IRQF_ONESHOT, because its is a threaded interrupt with no primary > handler. But I just noticed, that regmap-irq will also set the > IRQF_ONESHOT. But that the commit 09cadf6e088b ("regmap-irq: > set IRQF_ONESHOT flag to ensure IRQ request") reads like it is > just there to be sure. So I don't know if it should also be set > here. Ok. Wasn't aware of that magic threaded interrupt connection. Thanks, tglx
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index a85aada04a64..234db932e7cf 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -246,6 +246,9 @@ config RENESAS_RZA1_IRQC Enable support for the Renesas RZ/A1 Interrupt Controller, to use up to 8 external interrupts with configurable sense select. +config SL28CPLD_INTC + bool + config ST_IRQCHIP bool select REGMAP diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 37bbe39bf909..e3c6b94f7b0a 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -107,3 +107,4 @@ obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o obj-$(CONFIG_LOONGSON_LIOINTC) += irq-loongson-liointc.o obj-$(CONFIG_LOONGSON_HTPIC) += irq-loongson-htpic.o +obj-$(CONFIG_SL28CPLD_INTC) += irq-sl28cpld.o diff --git a/drivers/irqchip/irq-sl28cpld.c b/drivers/irqchip/irq-sl28cpld.c new file mode 100644 index 000000000000..88de71d32b09 --- /dev/null +++ b/drivers/irqchip/irq-sl28cpld.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * sl28cpld interrupt controller driver. + * + * Copyright 2019 Kontron Europe GmbH + */ + +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define INTC_IE 0x00 +#define INTC_IP 0x01 + +static const struct regmap_irq sl28cpld_irqs[] = { + REGMAP_IRQ_REG_LINE(0, 8), + REGMAP_IRQ_REG_LINE(1, 8), + REGMAP_IRQ_REG_LINE(2, 8), + REGMAP_IRQ_REG_LINE(3, 8), + REGMAP_IRQ_REG_LINE(4, 8), + REGMAP_IRQ_REG_LINE(5, 8), + REGMAP_IRQ_REG_LINE(6, 8), + REGMAP_IRQ_REG_LINE(7, 8), +}; + +struct sl28cpld_intc { + struct regmap *regmap; + struct regmap_irq_chip chip; + struct regmap_irq_chip_data *irq_data; +}; + +static int sl28cpld_intc_probe(struct platform_device *pdev) +{ + struct sl28cpld_intc *irqchip; + struct resource *res; + unsigned int irq; + int ret; + + if (!pdev->dev.parent) + return -ENODEV; + + irqchip = devm_kzalloc(&pdev->dev, sizeof(*irqchip), GFP_KERNEL); + if (!irqchip) + return -ENOMEM; + + irqchip->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!irqchip->regmap) + return -ENODEV; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + res = platform_get_resource(pdev, IORESOURCE_REG, 0); + if (!res) + return -EINVAL; + + irqchip->chip.name = "sl28cpld-intc"; + irqchip->chip.irqs = sl28cpld_irqs; + irqchip->chip.num_irqs = ARRAY_SIZE(sl28cpld_irqs); + irqchip->chip.num_regs = 1; + irqchip->chip.status_base = res->start + INTC_IP; + irqchip->chip.mask_base = res->start + INTC_IE; + irqchip->chip.mask_invert = true, + irqchip->chip.ack_base = res->start + INTC_IP; + + ret = devm_regmap_add_irq_chip(&pdev->dev, irqchip->regmap, irq, + IRQF_SHARED | IRQF_ONESHOT, 0, + &irqchip->chip, &irqchip->irq_data); + if (ret) + return ret; + dev_info(&pdev->dev, "registered IRQ %d\n", irq); + + return 0; +} + +static const struct platform_device_id sl28cpld_intc_id_table[] = { + { "sl28cpld-intc" }, + {} +}; +MODULE_DEVICE_TABLE(platform, sl28cpld_intc_id_table); + +static struct platform_driver sl28cpld_intc_driver = { + .probe = sl28cpld_intc_probe, + .id_table = sl28cpld_intc_id_table, + .driver = { + .name = KBUILD_MODNAME, + } +}; +module_platform_driver(sl28cpld_intc_driver); + +MODULE_DESCRIPTION("sl28cpld Interrupt Controller Driver"); +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index be0c8d93c526..9e848a455172 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -2065,6 +2065,8 @@ config MFD_SL28CPLD depends on I2C=y depends on OF select REGMAP_I2C + select REGMAP_IRQ + select SL28CPLD_INTC select MFD_CORE help This option enables support for the board management controller
This patch adds support for the interrupt controller inside the sl28 CPLD management controller. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/irqchip/Kconfig | 3 ++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-sl28cpld.c | 97 ++++++++++++++++++++++++++++++++++ drivers/mfd/Kconfig | 2 + 4 files changed, 103 insertions(+) create mode 100644 drivers/irqchip/irq-sl28cpld.c