Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Fri, Jan 17, 2025 at 04:22:33PM +0100, Mathieu Dubois-Briand wrote:
> On Tue Jan 14, 2025 at 3:33 PM CET, Linus Walleij wrote:
> > On Mon, Jan 13, 2025 at 1:43 PM Mathieu Dubois-Briand
> > My most generic feedback is if you have looked at using
> > select GPIO_REGMAP for this driver?
> >
> > The regmap utility library is very helpful, look how other driver
> > selecting GPIO_REGMAP gets default implementations
> > from the library just git grep GPIO_REGMAP drivers/gpio/
> 
> I tried to switch to GPIO_REGMAP and I really like it overall, as it
> does simplify a lot the code. However, I identified two features that I
> was not able to port so far: the request()/free() callbacks and the
> interrupts.

You can easily extend the config to provide both if needed.
So, update gpio-regmap.c itself as a prerequisite.

> So for the request()/free() callbacks, I cannot add them anymore, as
> they are set on the gpio_chip structure, and this structure is hidden
> behind the gpio_regmap structure. I could easily modify the
> gpio_regmap_config structure and gpio_regmap_register() to allow to
> provide these callbacks, but is this acceptable? Or should I switch to a
> different way to prevent concurrent use of the same pin? I saw you
> mentioned the possibility of defining pin control.
> 
> On the IRQ side, before switching to GPIO_REGMAP, I was able to define
> the IRQ configuration using the irq member of the gpio_chip structure.
> This does create the IRQ domain for me in a quite straightforward way.
> Again, I will not be able to do that anymore, as the gpio_chip structure
> is hidden. 

Look how it's done in, e.g., drivers/gpio/gpio-104-idi-48.c.

It's pretty straightforward. You create and register an IRQ chip with
devm_regmap_add_irq_chip(). It creates a domain for you.

> I saw I can specify my own irq_domain in gpio_regmap_config, so that
> would be a way, but I was wondering if there is any way to have
> something as easy as previously.
> 
> I had a quick look at existing drivers using GPIO_REGMAP and providing
> IRQ support: I believe they are all using REGMAP_IRQ. And I believe I
> cannot use REGMAP_IRQ here, as if I understood correctly, I would need
> to have a register telling me exactly on which GPIO I have a pending
> interrupt and I don't have such a thing: all I know is there was an
> interrupt related to the GPIOs, and then I have to compare each GPIO
> with the previous known state to know which pin is affected.
> 
> Do you have any thought about this?

I briefly looked at the code of regmap IRQ and I don't see big impediments
for your case. So, you seems to have mask, unmask, and input registers.
What you most likely want to do is to use input as status (regmap IRQ
supports configuration without status registers).

What seems to be needed is the logic that uses previous state to handle a new
one. This is not only this chip that may need this type of handling, so it
will be beneficial for others that may be converted to use gpio-regmap.c.

So, I don't think we need full bypass of the GPIO IRQ chip through
gpio-regmap.c.

-- 
With Best Regards,
Andy Shevchenko