Date:   Wed, 23 Jan 2019 13:00:46 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Lina Iyer <ilina@...eaurora.org>
Cc:     evgreen@...omium.org, marc.zyngier@....com,
        linux-kernel@...r.kernel.org, rplsssn@...eaurora.org,
        linux-arm-msm@...r.kernel.org, thierry.reding@...il.com,
        bjorn.andersson@...aro.org
Subject: Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

Quoting Lina Iyer (2019-01-16 15:13:28)
> On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-12-19 14:11:03)
> >> +
> >> +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >> +                                unsigned int nr_irqs, void *arg)
> >> +{
> >> +       int ret;
> >> +       irq_hw_number_t hwirq;
> >> +       struct gpio_chip *gc = domain->host_data;
> >> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> >> +       struct irq_fwspec *fwspec = arg;
> >> +       struct qcom_irq_fwspec parent = { };
> >> +       unsigned int type;
> >> +
> >> +       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> >> +                                           &pctrl->irq_chip, gc);
> >> +       if (ret < 0)
> >> +               return ret;
> >> +
> >> +       if (!domain->parent)
> >> +               return 0;
> >> +
> >> +       parent.fwspec.fwnode      = domain->parent->fwnode;
> >> +       parent.fwspec.param_count = 2;
> >> +       parent.fwspec.param[0]    = hwirq;
> >> +       parent.fwspec.param[1]    = type;
> >> +
> >> +       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       if (parent.mask)
> >> +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +/*
> >> + * TODO: Get rid of this and push it into gpiochip_to_irq()
> >
> >Hmm.. yeah we need to do this still. I think we can have a generic two
> >cell function similar to irq_domain_xlate_twocell() that does the fwspec
> >creation and uses some of the things that we pass to
> >gpiochip_irqchip_add(), like the default level type. This existing
> >function is not good to have, so there's work to do to get rid of this.
> >
> 
> >I was also thinking that maybe we can make the alloc function above take
> >a struct gpio_irq_fwspec structure that tells the alloc function what
> >gpiochip the irq is for. That would mean that we need to change the
> >gpio_to_irq() function below to be generic and stuff the chip inside the
> >fwspec wrapper structure:
> >
> >       struct gpio_irq_fwspec {
> >               struct irq_fwspec fwspec;
> >               struct gpio_chip *chip;
> >               unsigned int offset;
> >       };
> >
> >but I seem to recall that was not working for some reason.
> >
> I was thinking about this. If I understand you correctly, we want to
> generalize the .translate and .alloc functions. We could move the
> .translate to generic however, the alloc would still need to be specific
> for the parent.mask. But we can do this without the gpio_irq_fwspec. I
> presume you suggest this structure so we could pass the hwirq and type
> to the .alloc function. but we have that in the fwspec. What am I
> missing?

I'm trying to flatten the irq number space into a single integer while
letting it cross multiple gpio chips. Similar to how gpiolib flattens
the gpio number space across multiple gpio chips. The goal being to make
gpiochip_to_irq() do this all for us without having to implement it in
each gpio driver, but maybe that isn't needed or wise if the goal is to
move drivers away from taking a gpio number and converting it into an
irq with gpio_to_irq() to begin with.

FWIW, the SPMI PMIC gpio chip conversion patches didn't fix this problem
and Brian just called irq_create_fwspec_mapping() from the to_irq()
hook. So maybe this can be cleaned up later.

Powered by blists - more mailing lists