| Message ID | 20210420123932.24634-1-tsbogend@alpha.franken.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: Add support for IDT 79RC3243x GPIO controller | expand |
On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote: > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de> > wrote: > > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up > > to 32 gpios. All gpios could be used as interrupt source. > > > > I would recommend looking for latest new drivers in GPIO subsystem to see > how you may improve yours. Could give me a better pointer to it ? I looked at a lot of gpio driver and took what fitted best. > Here just one question, why it can not be a module that's probably doable... > why arch_initcall() is used without that interrupts weren't avaiable early enough. > and why you put a dead code into it (see the first part of the > question)? hmm, pointer please ? > I will perform a deep review later on. thank you. Thomas.
On Wed, Apr 21, 2021 at 11:32 AM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote: > > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > wrote: > > > > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up > > > to 32 gpios. All gpios could be used as interrupt source. > > > > > > > > I would recommend looking for latest new drivers in GPIO subsystem to see > > how you may improve yours. > > Could give me a better pointer to it ? I looked at a lot of gpio driver > and took what fitted best. > > > Here just one question, why it can not be a module > > that's probably doable... > > > why arch_initcall() is used > > without that interrupts weren't avaiable early enough. > > > and why you put a dead code into it (see the first part of the > > question)? > > hmm, pointer please ? It's already in the question above, do your homework :-)
On Wed, Apr 21, 2021 at 11:48:59AM +0300, Andy Shevchenko wrote: > On Wed, Apr 21, 2021 at 11:32 AM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote: > > > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > > wrote: > > > > > > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up > > > > to 32 gpios. All gpios could be used as interrupt source. > > > > > > > > > > > > I would recommend looking for latest new drivers in GPIO subsystem to see > > > how you may improve yours. > > > > Could give me a better pointer to it ? I looked at a lot of gpio driver > > and took what fitted best. > > > > > Here just one question, why it can not be a module > > > > that's probably doable... > > > > > why arch_initcall() is used > > > > without that interrupts weren't avaiable early enough. > > > > > and why you put a dead code into it (see the first part of the > > > question)? > > > > hmm, pointer please ? > > It's already in the question above, do your homework :-) is this some sort of joke I'm not getting ? git log --oneline drivers/gpio/Makefile 2ad74f40dacc gpio: visconti: Add Toshiba Visconti GPIO support that's the latest driver added in v5.12-rc8. Is that a good one ? Thomas.
On Wed, Apr 21, 2021 at 12:19 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > On Wed, Apr 21, 2021 at 11:48:59AM +0300, Andy Shevchenko wrote: > > On Wed, Apr 21, 2021 at 11:32 AM Thomas Bogendoerfer > > <tsbogend@alpha.franken.de> wrote: > > > On Wed, Apr 21, 2021 at 11:09:51AM +0300, Andy Shevchenko wrote: > > > > On Tuesday, April 20, 2021, Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > > > wrote: > > > > > > > > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up > > > > > to 32 gpios. All gpios could be used as interrupt source. > > > > > > > > > > > > > > > > I would recommend looking for latest new drivers in GPIO subsystem to see > > > > how you may improve yours. > > > > > > Could give me a better pointer to it ? I looked at a lot of gpio driver > > > and took what fitted best. > > > > > > > Here just one question, why it can not be a module > > > > > > that's probably doable... > > > > > > > why arch_initcall() is used > > > > > > without that interrupts weren't avaiable early enough. > > > > > > > and why you put a dead code into it (see the first part of the > > > > question)? > > > > > > hmm, pointer please ? > > > > It's already in the question above, do your homework :-) > > is this some sort of joke I'm not getting ? Maybe, but no, it's not a joke. You asked where the dead code is. I answered that you as the author of the code should have a better understanding of what you are doing. I can admit that it's hard to cover all aspects of the kernel programming in one go, but at least this part is a low hanging fruit. As I promised you, I will do a deep review later on, I'm giving you time to find issues yourself. That's how you may actually learn the things. It's solely your choice to follow or not, my promise will be kept and you will get an answer anyway. > git log --oneline drivers/gpio/Makefile > > 2ad74f40dacc gpio: visconti: Add Toshiba Visconti GPIO support > > that's the latest driver added in v5.12-rc8. Is that a good one ? Briefly looking, I haven't found any problems with that code. It looks neat.
On Wed, Apr 21, 2021 at 12:54:53PM +0300, Andy Shevchenko wrote: > As I promised you, I will do a deep review later on, I'm giving you > time to find issues yourself. That's how you may actually learn the > things. It's solely your choice to follow or not, my promise will be > kept and you will get an answer anyway. so let's make it a challenge ;-) I see I could use gpiochip_get_data() in few place. Is there more you see ? Thomas.
On Wed, Apr 21, 2021 at 1:37 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > > On Wed, Apr 21, 2021 at 12:54:53PM +0300, Andy Shevchenko wrote: > > As I promised you, I will do a deep review later on, I'm giving you > > time to find issues yourself. That's how you may actually learn the > > things. It's solely your choice to follow or not, my promise will be > > kept and you will get an answer anyway. > > so let's make it a challenge ;-) > > I see I could use gpiochip_get_data() in few place. > > Is there more you see ? Good. For now: - dead code due to driver not being compiled as module - too verbose Kconfig machinery (it's not about the "help" part!) - open coded stuff in IRQ handler - whatever you found.
On Wed, Apr 21, 2021 at 01:46:43PM +0300, Andy Shevchenko wrote: > On Wed, Apr 21, 2021 at 1:37 PM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > > > On Wed, Apr 21, 2021 at 12:54:53PM +0300, Andy Shevchenko wrote: > > > As I promised you, I will do a deep review later on, I'm giving you > > > time to find issues yourself. That's how you may actually learn the > > > things. It's solely your choice to follow or not, my promise will be > > > kept and you will get an answer anyway. > > > > so let's make it a challenge ;-) > > > > I see I could use gpiochip_get_data() in few place. > > > > Is there more you see ? > > Good. > > For now: > - dead code due to driver not being compiled as module Can you explain, why it's dead code, if it's not compilable as module ? > - too verbose Kconfig machinery (it's not about the "help" part!) the default y part ? Well I'm converting the MIPS rb532 platform to device tree. So I'm trying to make the whole process as bisectable as possible. And this would help, but I've no problem dropping that. > - open coded stuff in IRQ handler done. Thomas.
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index e3607ec4c2e8..6847a06ffcfe 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -770,6 +770,16 @@ config GPIO_MSC313 Say Y here to support the main GPIO block on MStar/SigmaStar ARMv7 based SoCs. +config GPIO_IDT3243X + bool "IDT 79RC3243X GPIO support" + default y if MIKROTIK_RB532 + depends on MIKROTIK_RB532 || COMPILE_TEST + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + help + Select this option to enable GPIO driver for + IDT 79RC3243X SoC devices. + endmenu menu "Port-mapped I/O GPIO drivers" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c58a90a3c3b1..75dd9c5665c5 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o obj-$(CONFIG_HTC_EGPIO) += gpio-htc-egpio.o obj-$(CONFIG_GPIO_ICH) += gpio-ich.o +obj-$(CONFIG_GPIO_IDT3243X) += gpio-idt3243x.o obj-$(CONFIG_GPIO_IOP) += gpio-iop.o obj-$(CONFIG_GPIO_IT87) += gpio-it87.o obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c new file mode 100644 index 000000000000..eaee46480268 --- /dev/null +++ b/drivers/gpio/gpio-idt3243x.c @@ -0,0 +1,210 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Driver for IDT/Renesas 79RC3243x Interrupt Controller. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/gpio/driver.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqchip.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/irqdomain.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> + +#define IDT_GPIO_NR_IRQS 32 + +#define IDT_PIC_IRQ_PEND 0x00 +#define IDT_PIC_IRQ_MASK 0x08 + +#define IDT_GPIO_DIR 0x04 +#define IDT_GPIO_DATA 0x08 +#define IDT_GPIO_ILEVEL 0x0C +#define IDT_GPIO_ISTAT 0x10 + +struct idt_gpio_ctrl { + struct gpio_chip gc; + void __iomem *pic; + void __iomem *gpio; + u32 mask_cache; +}; + +static struct idt_gpio_ctrl *irq_data_to_idt_gpio(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + + return container_of(gc, struct idt_gpio_ctrl, gc); +} + +static void idt_gpio_dispatch(struct irq_desc *desc) +{ + struct gpio_chip *gc = irq_desc_get_handler_data(desc); + struct idt_gpio_ctrl *ctrl = container_of(gc, struct idt_gpio_ctrl, gc); + struct irq_chip *host_chip = irq_desc_get_chip(desc); + u32 pending, hwirq, virq; + + chained_irq_enter(host_chip, desc); + + pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND); + pending &= ~ctrl->mask_cache; + while (pending) { + hwirq = __fls(pending); + virq = irq_linear_revmap(gc->irq.domain, hwirq); + if (virq) + generic_handle_irq(virq); + pending &= ~(1 << hwirq); + } + + chained_irq_exit(host_chip, desc); +} + +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type) +{ + struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d); + unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK; + u32 ilevel; + + if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) + return -EINVAL; + + ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL); + if (sense & IRQ_TYPE_LEVEL_HIGH) + ilevel |= BIT(d->hwirq); + else if (sense & IRQ_TYPE_LEVEL_LOW) + ilevel &= ~BIT(d->hwirq); + else + return -EINVAL; + + writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL); + return 0; +} + +static void idt_gpio_ack(struct irq_data *d) +{ + struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d); + + writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT); +} + +static void idt_gpio_mask(struct irq_data *d) +{ + struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d); + + ctrl->mask_cache |= BIT(d->hwirq); + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); +} + +static void idt_gpio_unmask(struct irq_data *d) +{ + struct idt_gpio_ctrl *ctrl = irq_data_to_idt_gpio(d); + + ctrl->mask_cache &= ~BIT(d->hwirq); + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); +} + +static struct irq_chip idt_gpio_irqchip = { + .name = "IDTGPIO", + .irq_mask = idt_gpio_mask, + .irq_ack = idt_gpio_ack, + .irq_unmask = idt_gpio_unmask, + .irq_set_type = idt_gpio_irq_set_type +}; + +static int idt_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct gpio_irq_chip *girq; + struct idt_gpio_ctrl *ctrl; + unsigned int parent_irq; + int ngpios; + int ret; + + ret = device_property_read_u32(dev, "ngpios", &ngpios); + if (ret) { + dev_err(dev, "ngpios property is not valid\n"); + return ret; + } + + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL); + if (!ctrl) + return -ENOMEM; + + ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio"); + if (!ctrl->gpio) + return -ENOMEM; + + ctrl->gc.parent = dev; + + ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA, + NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0); + if (ret) { + dev_err(dev, "bgpio_init failed\n"); + return ret; + } + ctrl->gc.ngpio = ngpios; + + ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic"); + if (!ctrl->pic) + return -ENOMEM; + + parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0); + if (!parent_irq) { + dev_err(&pdev->dev, "Failed to map parent IRQ!\n"); + return -EINVAL; + } + + /* Mask interrupts. */ + ctrl->mask_cache = 0xffffffff; + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); + + girq = &ctrl->gc.irq; + girq->chip = &idt_gpio_irqchip; + girq->parent_handler = idt_gpio_dispatch; + girq->num_parents = 1; + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents), + GFP_KERNEL); + if (!girq->parents) { + ret = -ENOMEM; + goto out_unmap_irq; + } + girq->parents[0] = parent_irq; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_level_irq; + + ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, NULL); + if (ret) + goto out_unmap_irq; + + return 0; + +out_unmap_irq: + irq_dispose_mapping(parent_irq); + return ret; +} + +static const struct of_device_id idt_gpio_of_match[] = { + { .compatible = "idt,3243x-gpio" }, + { } +}; +MODULE_DEVICE_TABLE(of, idt_gpio_of_match); + +static struct platform_driver idt_gpio_driver = { + .probe = idt_gpio_probe, + .driver = { + .name = "idt3243x-gpio", + .of_match_table = idt_gpio_of_match, + }, +}; + +static int __init idt_gpio_init(void) +{ + return platform_driver_register(&idt_gpio_driver); +} + +arch_initcall(idt_gpio_init); + +MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver"); +MODULE_AUTHOR("Thomas Bogendoerfer <tsbogend@alpha.franken.de>"); +MODULE_LICENSE("GPL");
IDT 79RC3243x SoCs integrated a gpio controller, which handles up to 32 gpios. All gpios could be used as interrupt source. Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> --- drivers/gpio/Kconfig | 10 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-idt3243x.c | 210 +++++++++++++++++++++++++++++++++++ 3 files changed, 221 insertions(+) create mode 100644 drivers/gpio/gpio-idt3243x.c