Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732285 > unrolled thread

Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure Linus Walleij <linus.walleij@linaro.org> - 2017-09-14 16:00 +0200
    Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked  infrastructure Thierry Reding <thierry.reding@gmail.com> - 2017-09-15 17:20 +0200
      Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked  infrastructure Tony Lindgren <tony@atomide.com> - 2017-09-15 19:00 +0200
        Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked  infrastructure Grygorii Strashko <grygorii.strashko@ti.com> - 2017-09-16 00:30 +0200
        Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure Linus Walleij <linus.walleij@linaro.org> - 2017-09-21 14:10 +0200

#1732285 — Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

On Fri, Sep 1, 2017 at 8:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote:

> here's the latest series of patches that implement the tighter IRQ chip
> integration as well as the banked GPIO infrastructure that we had
> discussed a couple of weeks/months back.

Yes it has become really tasty now, don't you think :)

I really like the series.

Banks are handled in the core, exactly as I wanted.

I will likely go in and change some things I don't like, like switching
num_pins in the bank to num_lines. I have preferred that terminology
to avoid confusion with pin control. So GPIO chips have lines, not pins.
But it's so minor that I can fix it up if you don't want to.

We also need to go in and patch Documentation/gpio/driver.txt
to represent the current best practice. But that can be later,
separate patch.

> The first couple of patches are mostly preparatory work in order to
> consolidate all IRQ chip related fields in a new structure and create
> the base functionality for adding IRQ chips.
>
> After that, I've added the Tegra186 GPIO support patch that makes use of
> the new tight integration.
>
> To round things off the new banked GPIO infrastructure is added (along
> with some more preparatory work), followed by the conversion of the two
> Tegra GPIO drivers to the new infrastructure.

I have put all on a branch for pushing to the test builders to begin with.

Then I plan to make one branch with all infrastructure patches
(patches 1-10, 12-14) and pull that into devel, then apply patch
11 and 15-16 directly on devel.

That way other subsystems (pinctrl ...) can pull in the infrastructure
for people adding new gpiochips this cycle.

> Any thoughts on this? I'd like to target 4.15 with this,

Me, too.

> unless you'd be
> willing to take this into 4.14, which I doubt at this point. The absence
> of a GPIO driver has been hampering Tegra186 support upstream for a
> while now, so it'd be good to make progress on this.

Sorry about that. Let's move ahead with this now, it is neat and
clean.

What I want (as maintainer) is a bit of fingerpointing at the drivers
that need to be converted to use the new banking infrastructure
so they don't stay with their old crappy design pattern. OMAP is
a clear candidate right? (Added Tony to CC...)

Who else?

Yours,
Linus Walleij

[toc] | [next] | [standalone]

#1732923 — Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

[Multipart message — attachments visible in raw view] — view raw

On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote:
> On Fri, Sep 1, 2017 at 8:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > here's the latest series of patches that implement the tighter IRQ chip
> > integration as well as the banked GPIO infrastructure that we had
> > discussed a couple of weeks/months back.
> 
> Yes it has become really tasty now, don't you think :)
> 
> I really like the series.
> 
> Banks are handled in the core, exactly as I wanted.
> 
> I will likely go in and change some things I don't like, like switching
> num_pins in the bank to num_lines. I have preferred that terminology
> to avoid confusion with pin control. So GPIO chips have lines, not pins.
> But it's so minor that I can fix it up if you don't want to.

I rebased this on today's linux-next and noticed that there was a small
conflict. I can rebase and work in the changes that you requested.

I'm travelling this week and next, so it may take until after -rc2 that
I can send out a new version that's properly build-tested.

> We also need to go in and patch Documentation/gpio/driver.txt
> to represent the current best practice. But that can be later,
> separate patch.
> 
> > The first couple of patches are mostly preparatory work in order to
> > consolidate all IRQ chip related fields in a new structure and create
> > the base functionality for adding IRQ chips.
> >
> > After that, I've added the Tegra186 GPIO support patch that makes use of
> > the new tight integration.
> >
> > To round things off the new banked GPIO infrastructure is added (along
> > with some more preparatory work), followed by the conversion of the two
> > Tegra GPIO drivers to the new infrastructure.
> 
> I have put all on a branch for pushing to the test builders to begin with.
> 
> Then I plan to make one branch with all infrastructure patches
> (patches 1-10, 12-14) and pull that into devel, then apply patch
> 11 and 15-16 directly on devel.
> 
> That way other subsystems (pinctrl ...) can pull in the infrastructure
> for people adding new gpiochips this cycle.

Sounds good.

> > Any thoughts on this? I'd like to target 4.15 with this,
> 
> Me, too.
> 
> > unless you'd be
> > willing to take this into 4.14, which I doubt at this point. The absence
> > of a GPIO driver has been hampering Tegra186 support upstream for a
> > while now, so it'd be good to make progress on this.
> 
> Sorry about that. Let's move ahead with this now, it is neat and
> clean.
> 
> What I want (as maintainer) is a bit of fingerpointing at the drivers
> that need to be converted to use the new banking infrastructure
> so they don't stay with their old crappy design pattern. OMAP is
> a clear candidate right? (Added Tony to CC...)

OMAP should be able to use this infrastructure, but it may not want to
because the semantics would change slightly. Currently OMAP registers a
GPIO chip for each bank, whereas this infrastructure exposes multiple
banks via a single chip.

There might be some userspace that relies on the existence of multiple
chips, but Tony can probably knows that better than I.

> Who else?

gpio-intel-mid.c and gpio-merrifield.c look like they could use this new
infrastructure. So do gpio-pca953x.c, gpio-stmpe.c and gpio-tc3589x.c.

gpio-ws16c48.c is another one that uses a similar pattern.

Thierry

[toc] | [prev] | [next] | [standalone]

#1732953 — Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

* Thierry Reding <thierry.reding@gmail.com> [170915 08:10]:
> On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote:
> > Sorry about that. Let's move ahead with this now, it is neat and
> > clean.
> > 
> > What I want (as maintainer) is a bit of fingerpointing at the drivers
> > that need to be converted to use the new banking infrastructure
> > so they don't stay with their old crappy design pattern. OMAP is
> > a clear candidate right? (Added Tony to CC...)
> 
> OMAP should be able to use this infrastructure, but it may not want to
> because the semantics would change slightly. Currently OMAP registers a
> GPIO chip for each bank, whereas this infrastructure exposes multiple
> banks via a single chip.

Oh so you don't have separate interrupts for the instances?
Thanks for clarifying that.

> There might be some userspace that relies on the existence of multiple
> chips, but Tony can probably knows that better than I.

On omaps, each bank is a separate driver instance with it's own
interrupt. Maybe really all we need to do is get rid of the "bank"
naming, I think that's left over from 15 years ago when we did not
have separate driver instances. It seems we should s/bank/ddata/
on the driver to avoid confusion.

Grygorii, any comments?

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1733121 — Re: [PATCH 00/16] gpio: Tight IRQ chip integration and banked infrastructure

On 09/15/2017 11:57 AM, Tony Lindgren wrote:
> * Thierry Reding <thierry.reding@gmail.com> [170915 08:10]:
>> On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote:
>>> Sorry about that. Let's move ahead with this now, it is neat and
>>> clean.
>>>
>>> What I want (as maintainer) is a bit of fingerpointing at the drivers
>>> that need to be converted to use the new banking infrastructure
>>> so they don't stay with their old crappy design pattern. OMAP is
>>> a clear candidate right? (Added Tony to CC...)
>>
>> OMAP should be able to use this infrastructure, but it may not want to
>> because the semantics would change slightly. Currently OMAP registers a
>> GPIO chip for each bank, whereas this infrastructure exposes multiple
>> banks via a single chip.
> 
> Oh so you don't have separate interrupts for the instances?
> Thanks for clarifying that.
> 
>> There might be some userspace that relies on the existence of multiple
>> chips, but Tony can probably knows that better than I.
> 
> On omaps, each bank is a separate driver instance with it's own
> interrupt. Maybe really all we need to do is get rid of the "bank"
> naming, I think that's left over from 15 years ago when we did not
> have separate driver instances. It seems we should s/bank/ddata/
> on the driver to avoid confusion.
> 
> Grygorii, any comments?

Sry, for delayed reply - I've saw this series, but, honestly, it's very big 
change for review :(

So, can it be split? I think, patches which reorganize gpio irqchip specific fields placement 
and move them in gpio_irq_chip can be considered separately if they will not introduce
functional changes. Also, omap changes can be considered separately.
(Pay attention that new fields introduced in patch 1).  

Regarding OMAP GPIO - right now I do not see how it can be applied for OMAP :(
Each OMAP GPIO bank is standalone device which can be enabled/disabled, 
powered on/off in Linux. There are no contiguous MMIO space and each GPIO
bank have separate MMIO space.

I really, need more time to review this idea and I think that it can be done
more easily if series size will be reduced.

Few more notes:
- pay attention on commit dc749a0 "gpiolib: allow gpio irqchip to map irqs dynamically"
- good to see binding and DT examples
- not sure if I've got idea of encoding bank&pin in spec[0] :(

+	bank = (spec[0] >> gc->of_gpio_bank_mask) & gc->of_gpio_bank_shift;
+	pin = (spec[0] >> gc->of_gpio_pin_mask) & gc->of_gpio_pin_shift;

- irq "mapping" inside gpio_irq_chip is not clear :( does it static?
does it require one array item/per pin - sry, this is waste of memory?

irq->map[offset + j] = irq->parents[parent];

Potentially, this feature can be applied to Davinci GPIO driver, which
is GPIO controller divided in multiple logical banks and which also have
set of common registers for all logical banks. But, again, not sure how
effective this implementation is - need more time. As of now, we perfectly 
handle this in Davinci GPIO driver by creating only ONE gpio_chip which hides
HW details in driver and still uses standard irq DT mappings:
 <&gpio0 140 GPIO_ACTIVE_LOW>;

By the way Patch 14 adds 300 lines, while patch changes 200 lines, so
in terms of code lines this feature seems is not very efficient.
(same for Patch 15, 16)

-- 
regards,
-grygorii

[toc] | [prev] | [next] | [standalone]

#1736607

On Fri, Sep 15, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote:

> On omaps, each bank is a separate driver instance with it's own
> interrupt. Maybe really all we need to do is get rid of the "bank"
> naming, I think that's left over from 15 years ago when we did not
> have separate driver instances. It seems we should s/bank/ddata/
> on the driver to avoid confusion.

OK sorry maybe OMAP is not a target for this, I just thought so
since it was one of the platforms that is patches in the patch
series.

But I'm pretty sure we have chips with banking like this: separate
interrupts but a single device. I would have to read through them
all I guess.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web