Hi Brian,
I tried to look into this ssbi-gpio problem...
Paging in Marc Z as well.
On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@xxxxxxxxxxxxx> wrote:
> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
> this little snippet that's different from spmi-gpio:
>
> [ fwspec mapping code ]
>
> /*
> * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
> * line level.
> */
> pin->irq = ret;
>
> Here's the relevant code in pm8xxx_gpio_get():
>
> if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
> ret = pin->output_value;
> } else if (pin->irq >= 0) {
> ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
> ...
> }
It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
the global irq numbers and the internal function using irqdesc
__irq_get_irqchip_state() is not exported.
We should be encouraged to operate on IRQ descriptors rather
than IRQ numbers when doing this kind of deep core work,
right?
> What do you think about using EXPORT_SYMBOL_GPL() for gpiochip_to_irq() so
> that we can call it in pm8xxx_gpio_to_irq()?
I would like to avoid that because people tend to abuse every
API I expose (leasson learnt: APIs shall be narrow and deep).
> Or do you have any other
> suggestions for how we can get rid of that IRQ cache?
So the issue is that moving this to the hierarchical helpers
rids pm8xxx_gpio_to_irq() and we needed that to map this.
I would rather just add a new driver API to wrap the irqchip
API:
ret = gpiochip_get_irq_state(offset, IRQCHIP_STATE_LINE_LEVEL, &state);
Where int gpiochip_get_own_irq_state(int offset, ...) will figure
out the gpio_desc of the offset and then call gpiod_to_irq()
and then use that irq number to call irq_get_irqchip_state()
and the goal is met.
This should work I think, and expose a more precise
API for what this driver wants.
> I don't see any other issues for ssbi-gpio.
That's good news!
Yours,
Linus Walleij
