Re: [PATCH v4 07/10] gpio: max7360: Add MAX7360 gpio support

On Fri Feb 14, 2025 at 4:59 PM CET, Andy Shevchenko wrote:

On Fri, Feb 14, 2025 at 12:49:57PM +0100, Mathieu Dubois-Briand wrote:

quoted

Add driver for Maxim Integrated MAX7360 GPIO/GPO controller.

...

quoted

+
+	/*
+	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
+	 * timings and gpos/keypad columns repartition. Only the later is
+	 * modified here.
+	 */
+	val = FIELD_PREP(MAX7360_PORTS, ngpios);
+	ret = regmap_write_bits(regmap, MAX7360_REG_DEBOUNCE, MAX7360_PORTS, val);
+	if (ret) {
+		dev_err(dev, "Failed to write max7360 columns/gpos configuration");
+		return ret;
+	}

Shouldn't this be configured via ->set_config() callback?

I believe this comment has been a bit outdated by our discussion on
using GPIO valid mask, but I believe we could not use the ->set_config()
callback here: this callback is made to configure a single pin while the
gpos/keypad columns repartition is global.

...

quoted

+		if (irq < 0)
+			return dev_err_probe(dev, irq, "Failed to get IRQ\n");
+
+		irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
+		if (!irq_chip)
+			return -ENOMEM;
+
+		irq_chip->name = dev_name(dev);
+		irq_chip->status_base = MAX7360_REG_GPIOIN;
+		irq_chip->num_regs = 1;
+		irq_chip->num_irqs = MAX7360_MAX_GPIO;
+		irq_chip->irqs = max7360_regmap_irqs;
+		irq_chip->handle_mask_sync = max7360_handle_mask_sync;
+		irq_chip->status_is_level = true;
+		irq_chip->irq_drv_data = regmap;
+
+		for (unsigned int i = 0; i < MAX7360_MAX_GPIO; i++) {
+			regmap_write_bits(regmap, MAX7360_REG_PWMCFG(i),
+					  MAX7360_PORT_CFG_INTERRUPT_EDGES,
+					  MAX7360_PORT_CFG_INTERRUPT_EDGES);
+		}
+
+		flags = IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED;
+		ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), regmap, irq, flags, 0,
+						      irq_chip, &irq_chip_data);

Right.

What I mean in previous discussion is to update gpio-regmap to call this from inside.
You need to add irq_chip pointer and irq_chip_data pointer to the regmap configuration
and if they are set (or the first one, I dunno if this is supported by IRQ chip core)
call this function and assign domain. This should be called after GPIO chip is
added, but before IRQ domain attachment.

Ok, this is a bit more clear to me now. So I came up with something, it
will be part of the next iteration, probably during the next week.

This required to add a few additional fields to the gpio_regmap_config
structure, specifying the IRQ configuration:

+ * @regmap_irq_chip:   (Optional) Pointer on an regmap_irq_chip structure. If
+ *                     set, a regmap-irq device will be created and the IRQ
+ *                     domain will be set accordingly.
+ * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data
+ *                      structure pointer. If set, it will be populated with a
+ *                      pointer on allocated regmap_irq data.
+ * @regmap_irq_irqno   (Optional) The IRQ the device uses to signal interrupts.
+ * @regmap_irq_flags   (Optional) The IRQF_ flags to use for the interrupt.

...

quoted

+
+		regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
+	}
+
+	/* Add gpio device. */
+	gpio_config.parent = dev;
+	gpio_config.regmap = regmap;

quoted

+	if (gpio_function == MAX7360_GPIO_PORT) {
+		gpio_config.ngpio = MAX7360_MAX_GPIO;

Why this case can't be managed also via ngpios property? Maybe at the end of
the day you rather need to have another property to tell where the split is?

This will help a lot and removes unneeded sharing of ngpios here and there.

What I read from this code is like you are trying to put _two_in_one_ semantics
on the shoulders of "ngpios".

So as I reworked the keypad columns GPIOs, PORT GPIOs and the COL GPIOs
are a bit more similar on this point. So far I now use a constant value
assigned in the driver for both, as I believe there is no way the number
of GPIOs could be a different. Yet I can easily switch back to a value
provided by a device property.

Thanks again for your review.
Mathieu

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