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:

quoted

On Wed, Feb 12, 2025 at 05:08:56PM +0100, Mathieu Dubois-Briand wrote:

quoted

On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:

quoted

On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:

quoted

On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:

quoted

On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:

...

quoted

quoted

quoted

quoted

quoted

+	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?

quoted

- 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.

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.
- Add a function allowing to retrieve this domain out of the gpio_regmap
  structure.
- 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().
- 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.
- 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 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.


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