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

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

quoted

Some GPIO chips allow to rise an IRQ on GPIO level changes but do not
provide an IRQ status for each separate line: only the current gpio
level can be retrieved.

Add support for these chips, emulating IRQ status by comparing GPIO
levels with the levels during the previous interrupt.

Thanks, this will help to convert more drivers to regmap
(e.g., gpio-pca953x that seems use similar approach).

...

quoted

+static irqreturn_t regmap_irq_thread(int irq, void *d)
+{
+	struct regmap_irq_chip_data *data = d;
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+	int ret, i;

	unsigned int i;
?

I agree, but signed int index variables are used in all functions of
this file. What would be the best approach here? Only fix it on code
parts I modified? On the whole file?

quoted

+	bool handled = false;
+	u32 reg;
+
+	if (chip->handle_pre_irq)
+		chip->handle_pre_irq(chip->irq_drv_data);
+
+	if (chip->runtime_pm) {
+		ret = pm_runtime_get_sync(map->dev);
+		if (ret < 0) {

quoted

+			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
+				ret);

Can be one line.

Yes. Kind of the same question here: should I fix only the code close to
the parts I modified or the whole file?

...

quoted

+		for (i = 0; i < d->chip->num_regs; i++)
+			d->prev_status_buf[i] = d->status_buf[i];

Hmm... Wouldn't memcpy() suffice?
But okay, this seems to be not a hot path and the intention is clear.

Yes... I don't know why I didn't use memcpy. I will fix it.


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