Hi Linus,
On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> Use the new infrastructure for hierarchical irqchips in
> gpiolib.
>
> I have no chance to test or debug this so I need
> help.
>
> Cc: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> Cc: Thierry Reding <treding@xxxxxxxxxx>
> Cc: Brian Masney <masneyb@xxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> drivers/gpio/Kconfig | 1 +
> drivers/gpio/gpio-uniphier.c | 172 +++++++++--------------------------
> 2 files changed, 45 insertions(+), 128 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index ee34716a39aa..806d642498a6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -552,6 +552,7 @@ config GPIO_UNIPHIER
> tristate "UniPhier GPIO support"
> depends on ARCH_UNIPHIER || COMPILE_TEST
> depends on OF_GPIO
> + select GPIOLIB_IRQCHIP
> select IRQ_DOMAIN_HIERARCHY
> help
> Say yes here to support UniPhier GPIOs.
> diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
> index 93cdcc41e9fb..e960836b9c01 100644
> --- a/drivers/gpio/gpio-uniphier.c
> +++ b/drivers/gpio/gpio-uniphier.c
> @@ -6,7 +6,6 @@
> #include <linux/bits.h>
> #include <linux/gpio/driver.h>
> #include <linux/irq.h>
> -#include <linux/irqdomain.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -30,7 +29,6 @@
> struct uniphier_gpio_priv {
> struct gpio_chip chip;
> struct irq_chip irq_chip;
> - struct irq_domain *domain;
> void __iomem *regs;
> spinlock_t lock;
> u32 saved_vals[0];
> @@ -162,49 +160,33 @@ static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
> }
> }
>
> -static int uniphier_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +static void uniphier_gpio_irq_mask(struct irq_data *d)
> {
> - struct irq_fwspec fwspec;
> -
> - if (offset < UNIPHIER_GPIO_IRQ_OFFSET)
> - return -ENXIO;
> -
> - fwspec.fwnode = of_node_to_fwnode(chip->parent->of_node);
> - fwspec.param_count = 2;
> - fwspec.param[0] = offset - UNIPHIER_GPIO_IRQ_OFFSET;
> - /*
> - * IRQ_TYPE_NONE is rejected by the parent irq domain. Set LEVEL_HIGH
> - * temporarily. Anyway, ->irq_set_type() will override it later.
> - */
> - fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
> -
> - return irq_create_fwspec_mapping(&fwspec);
> -}
> -
> -static void uniphier_gpio_irq_mask(struct irq_data *data)
> -{
> - struct uniphier_gpio_priv *priv = data->chip_data;
> - u32 mask = BIT(data->hwirq);
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
> + u32 mask = BIT(d->hwirq);
>
> uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, 0);
>
> - return irq_chip_mask_parent(data);
> + return irq_chip_mask_parent(d);
> }
>
> -static void uniphier_gpio_irq_unmask(struct irq_data *data)
> +static void uniphier_gpio_irq_unmask(struct irq_data *d)
Are you renaming 'data' -> 'd'
just for your personal preference?
> {
> - struct uniphier_gpio_priv *priv = data->chip_data;
> - u32 mask = BIT(data->hwirq);
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
> + u32 mask = BIT(d->hwirq);
>
> uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, mask);
>
> - return irq_chip_unmask_parent(data);
> + return irq_chip_unmask_parent(d);
> }
>
> -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type)
Again, this seems a noise change.
> {
> - struct uniphier_gpio_priv *priv = data->chip_data;
> - u32 mask = BIT(data->hwirq);
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
> + u32 mask = BIT(d->hwirq);
> u32 val = 0;
>
> if (type == IRQ_TYPE_EDGE_BOTH) {
> @@ -216,7 +198,7 @@ static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> /* To enable both edge detection, the noise filter must be enabled. */
> uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_FLT_EN, mask, val);
>
> - return irq_chip_set_type_parent(data, type);
> + return irq_chip_set_type_parent(d, type);
> }
>
> static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv,
> @@ -245,82 +227,6 @@ static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv,
> return -ENOENT;
> }
>
> -static int uniphier_gpio_irq_domain_translate(struct irq_domain *domain,
> - struct irq_fwspec *fwspec,
> - unsigned long *out_hwirq,
> - unsigned int *out_type)
> -{
> - if (WARN_ON(fwspec->param_count < 2))
> - return -EINVAL;
> -
> - *out_hwirq = fwspec->param[0];
> - *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> -
> - return 0;
> -}
> -
> -static int uniphier_gpio_irq_domain_alloc(struct irq_domain *domain,
> - unsigned int virq,
> - unsigned int nr_irqs, void *arg)
> -{
> - struct uniphier_gpio_priv *priv = domain->host_data;
> - struct irq_fwspec parent_fwspec;
> - irq_hw_number_t hwirq;
> - unsigned int type;
> - int ret;
> -
> - if (WARN_ON(nr_irqs != 1))
> - return -EINVAL;
> -
> - ret = uniphier_gpio_irq_domain_translate(domain, arg, &hwirq, &type);
> - if (ret)
> - return ret;
> -
> - ret = uniphier_gpio_irq_get_parent_hwirq(priv, hwirq);
> - if (ret < 0)
> - return ret;
> -
> - /* parent is UniPhier AIDET */
> - parent_fwspec.fwnode = domain->parent->fwnode;
> - parent_fwspec.param_count = 2;
> - parent_fwspec.param[0] = ret;
> - parent_fwspec.param[1] = (type == IRQ_TYPE_EDGE_BOTH) ?
> - IRQ_TYPE_EDGE_FALLING : type;
> -
> - ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> - &priv->irq_chip, priv);
> - if (ret)
> - return ret;
> -
> - return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> -}
> -
> -static int uniphier_gpio_irq_domain_activate(struct irq_domain *domain,
> - struct irq_data *data, bool early)
> -{
> - struct uniphier_gpio_priv *priv = domain->host_data;
> - struct gpio_chip *chip = &priv->chip;
> -
> - return gpiochip_lock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
> -}
> -
> -static void uniphier_gpio_irq_domain_deactivate(struct irq_domain *domain,
> - struct irq_data *data)
> -{
> - struct uniphier_gpio_priv *priv = domain->host_data;
> - struct gpio_chip *chip = &priv->chip;
> -
> - gpiochip_unlock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
> -}
I did not test this patch, but probably it would break my board.
->(de)activate hook has offset UNIPHIER_GPIO_IRQ_OFFSET (=120),
but you are replacing it with generic gpiochip_irq_domain_activate,
which as zero offset.
> ret = devm_gpiochip_add_data(dev, chip, priv);
> if (ret)
> return ret;
>
> - priv->domain = irq_domain_create_hierarchy(
> - parent_domain, 0,
> - UNIPHIER_GPIO_IRQ_MAX_NUM,
You are replacing UNIPHIER_GPIO_IRQ_MAX_NUM with gc->ngpio,
which will much more irqs than needed.
Is it possible to provide more flexibility?
--
Best Regards
Masahiro Yamada
Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
- Subject: Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
- From: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
- Date: Thu, 18 Jul 2019 20:09:33 +0900
- In-reply-to: <20190624132531.6184-4-linus.walleij@linaro.org>
- References: <20190624132531.6184-1-linus.walleij@linaro.org> <20190624132531.6184-4-linus.walleij@linaro.org>
- Follow-Ups:
- Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
- From: Linus Walleij
- Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
- References:
- [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains
- From: Linus Walleij
- [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
- From: Linus Walleij
- [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains
- Prev by Date: [PATCH 2/2] gpio: mpc8xxx: Add ls1028a device specify function.
- Next by Date: Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains
- Previous by thread: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
- Next by thread: Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP
- Index(es):
 |