From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
Date: Fri, 23 Apr 2021 18:37:41 +0300 [thread overview]
Message-ID: <CAHp75Ve6PEr5TFGRgALPCbi-T5Y5yNPV+-fJHC7C2mU+ms30uw@mail.gmail.com> (raw)
In-Reply-To: <20210422152055.85544-1-tsbogend@alpha.franken.de>
On Thu, Apr 22, 2021 at 6:21 PM 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.
as an interrupt
Thanks for an update, my comments below (minus that you already figured out).
...
> +config GPIO_IDT3243X
> + tristate "IDT 79RC3243X GPIO support"
> + depends on MIKROTIK_RB532 || COMPILE_TEST
Right.
But if MikroTik is dependent on this you may return default in a form of
default MIKROTIK_RB532
Up to you. (What I meant previously is the unnecessary ' y if' part).
> + select GPIO_GENERIC
> + select GPIOLIB_IRQCHIP
> + help
> + Select this option to enable GPIO driver for
> + IDT 79RC3243X SoC devices.
Seems like you may elaborate a bit more here, what kind of the
devices, list one or couple of examples, etc.
> + To compile this driver as a module, choose M here: the module will
> + be called gpio-idt3243x.
...
> +/*
> + * Driver for IDT/Renesas 79RC3243x Interrupt Controller.
> + */
One line?
...
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Why is this?
...
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
Aren't those guaranteed to be included by irq.h?
> +#include <linux/irqdomain.h>
Missed mod_devicetable.h
module.h
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
Do you use them anyhow? (See below as well)
Missed types.h
...
> +static void idt_gpio_dispatch(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> + struct irq_chip *host_chip = irq_desc_get_chip(desc);
> + unsigned int bit, virq;
> + unsigned long pending;
> +
> + chained_irq_enter(host_chip, desc);
> +
> + pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND);
> + pending &= ~ctrl->mask_cache;
> + for_each_set_bit(bit, &pending, gc->ngpio) {
> + virq = irq_linear_revmap(gc->irq.domain, bit);
Is it guaranteed to be linear always?
> + if (virq)
> + generic_handle_irq(virq);
> + }
> +
> + chained_irq_exit(host_chip, desc);
> +}
...
> + if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
There is a _BOTH variant.
> + 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;
Is it a double check of the above?
...
> + ctrl->gc.parent = dev;
Wondering if it's already done by GPIO library.
...
> + ctrl->gc.ngpio = ngpios;
Shouldn't you do this before calling for bgpio_init()?
...
> + parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
platform_get_irq() ?..
> + if (!parent_irq) {
> + dev_err(&pdev->dev, "Failed to map parent IRQ!\n");
...and drop this, since it will be taken care of.
> + return -EINVAL;
> + }
...
> + /* Mask interrupts. */
> + ctrl->mask_cache = 0xffffffff;
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
What about using ->init_hw() call back?
...
> + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
1 -> girq->num_parents
> + GFP_KERNEL);
> + if (!girq->parents) {
> + ret = -ENOMEM;
> + goto out_unmap_irq;
> + }
...
> + girq->handler = handle_level_irq;
handle_bad_irq()
...
> + ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
> + if (ret)
> + goto out_unmap_irq;
> +
> + return 0;
return devm_...;
...
> +out_unmap_irq:
> + irq_dispose_mapping(parent_irq);
> + return ret;
> +}
No need.
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2021-04-23 15:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-22 15:20 [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
2021-04-23 17:53 ` Rob Herring
2021-04-23 15:37 ` Andy Shevchenko [this message]
2021-04-24 10:35 ` [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x " Thomas Bogendoerfer
2021-04-24 11:35 ` Andy Shevchenko
2021-04-23 15:56 ` Andy Shevchenko
2021-04-26 9:33 ` Thomas Bogendoerfer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHp75Ve6PEr5TFGRgALPCbi-T5Y5yNPV+-fJHC7C2mU+ms30uw@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=bgolaszewski@baylibre.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tsbogend@alpha.franken.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.