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

["Mathieu Dubois-Briand" <mathieu.dubois-briand@xxxxxxxxxxx>]

On Thu Feb 13, 2025 at 8:47 PM CET, Andy Shevchenko wrote:
> On Thu, Feb 13, 2025 at 02:45:31PM +0100, Mathieu Dubois-Briand wrote:
> > On Thu Feb 13, 2025 at 11:59 AM CET, Mathieu Dubois-Briand wrote:
> > > On Wed Feb 12, 2025 at 5:17 PM CET, Andy Shevchenko wrote:
> > > > On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:
> > > > > On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> > > > > > On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > > > > > > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > > > > > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
>
> ...
>
> > > > > > > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > > > > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > > > > > > +		return -ENODEV;
> > > > > > > > > +	}
> > > > > > > >
> > > > > > > > This is not needed, it is already done in GPIOLIB core.
> > > > > > > 
> > > > > > > I believe this is still needed:
> > > > > > > - For gpos, we need the gpio count to correctly set the partition
> > > > > > >   between gpo and keypad columns in max7360_set_gpos_count().
> > > > > >
> > > > > > Shouldn't be that done somewhere in the GPIO valid mask initialisation?
> > > > > >
> > > > > > > - For gpios, we need the gpio count to setup the IRQs.
> > > > > >
> > > > > > Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> > > > > > and other init callbacks?
> > > > > 
> > > > > No, I believe I have to register the IRQ before registering the GPIO, so
> > > > > I can get the IRQ domain.
> > > > > 
> > > > > Right now I have something like:
> > > > > 
> > > > > irq_chip->num_irqs = ngpios;
> > > > > devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap,
> > > > > irq, flags, 0, irq_chip, &irq_chip_data);
> > > > > gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
> > > > > devm_gpio_regmap_register(dev, &gpio_config);
> > > > > 
> > > > > Also, gpiolib will store ngpios in the gpio_chip structure, but while
> > > > > using gpio-regmap, this structure is masked behind the opaque
> > > > > gpio_regmap structure. So I believe there is no easy way to retrieve its
> > > > > value.
>
> Would it be needed in your driver ->probe() after all? (See also below)
>

No necessarily in the probe with the changes previously described, but I
will need it in other functions. So either I get it in the probe and
store it, or I will need to retrieve it by other means.

> > > > > This part of the code changed a lot, maybe it would be easier if I push
> > > > > a new version of the series and we continue the discussion there?
> > > >
> > > > So, what seems need to be added is some flag to GPIO regmap configuration
> > > > data structure and a code that is called after gpiochip_add_data() in
> > > > gpio_regmap_register() to create a domain. This will solve the above issue
> > > > and helps other drivers to get rid of potential duplication of
> > > > devm_regmap_add_irq_chip_fwnode() calls.
> > > >
> > > > Have you researched this path?
> > >
> > > OK, so looking at the code, I believe it would need to:
> > > - Add some flag in gpio_regmap_config structure, so
> > >   gpio_regmap_register() creates a new IRQ domain.
>
> Easy.
>
> > > - Add a function allowing to retrieve this domain out of the gpio_regmap
> > >   structure.
>
> Easy, as there is an API available for regmaps, so it looks like one-liner.
>
> > > - Allow to pass a domain in the regmap_irq_chip structure, so
> > >   regmap_add_irq_chip_fwnode() use this domain instead of calling
> > >   regmap_irq_create_domain().
>
> You need this because of...? (Please, remind me what the obstacle is there
> that requires this to be done)
>

OK, maybe I misunderstood you idea. Or maybe I misunderstood something
about IRQ domains.

So what I understood is, in the probe, we first call
gpio_regmap_register(), that will create a new IRQ domain, and we call
regmap_add_irq_chip_fwnode() in second. But this second call will again
create an IRQ domain, so we would end-up with a second one. We have to
prevent this second creation and make it use the one we created first.

Did I miss something?

> > > - Make sure this domain is still populated with the IRQ data: number of
> > >   IRQs, IRQ base but also a pointer on the regmap_irq_chip_data
> > >   structure in .host_data. I believe this will be a bit tricky.
>
> Hmm... But wouldn't gpio-regmap internals have all this information available?
>

I don't think so. It will not know the IRQ base nor the
regmap_irq_chip_data as it is not created at this point.

> > > - Add a function allowing to retrieve ngpio out of the
> > >   gpio_regmap.gpio_chip structure, so it can be used for IRQ setup and
> > >   other places of the driver.
>
> I'm not sure where it can be needed.
>

Let's discuss this on the next version, but yes, I do need to know ngpio
in the driver.

> > > I'm sorry, but I feel like this is a lot of changes to solve this point.
> > > I've been thinking about it, and I can suggest a different solution.
> > >
> > > For gpios, I will remove the ngpios property of the device tree and use
> > > a fixed value:
> > > - For the today version of the chip, this is always 8.
> > > - I a chip variant or a similar chip ever arise later with a different
> > >   number of gpios, the fixed value can be set according to the
> > >   "compatible" value.
> > > - This removes any issue with the IRQ setup.
> > >
> > > For gpos, we have to keep ngpios, as it depends of the implementation on
> > > the board. That means ngpios will be used:
> > > - For the gpio chip configuration: we let gpiolib retrieve it from the
> > >   device tree.
> > > - In gpio-regmap reg_mask_xlate callback: I can add a function allowing
> > >   to retrieve it from gpio_regmap.gpio_chip, as suggested above.
> > > - In max7360_set_gpos_count() to validate the coherency between
> > >   requested gpios and keypad columns and set the correct configuration
> > >   on the chip:
> > >   - I can also retrieve the value from gpio_regmap.gpio_chip, but that
> > >     means the check is made after the call to
> > >     devm_gpio_regmap_register().
> > >   - Or I will still need to retrieve it using device_property_read_u32()
> > >     here.
> > >
> > > How do you feel about this solution?
> > 
> > Actually there is an additional issue: today, relying on gpiolib to
> > parse the "ngpios" property does not work with gpio-regmap.
> > 
> > The gpiochip_get_ngpios() function in gpiolib is called in
> > gpiochip_add_data_with_key(), but when using gpio_regmap_register(),
> > we first ensure ngpio is set correctly before calling anything.
> > 
> > Yet I believe this check can safely be removed, allowing the magic in
> > gpiolib happen as expected.
>
> Not really. I'm about to send a series to address this issue.
> Please, test.

I will test it today.

>
> ...
>
> P.S.
> Maybe it's time to send a new version based on this discussion even
> if not finished / working, so we can see the exact issues we still have
> and target them.

Sure, I will send a new version, probably today.

Thanks again for your help.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com