diff mbox series

[v4,06/10] regmap: irq: Add support for chips without separate IRQ status

Message ID 20250214-mdb-max7360-support-v4-6-8a35c6dbb966@bootlin.com (mailing list archive)
State New
Headers show
Series Add support for MAX7360 | expand

Commit Message

Mathieu Dubois-Briand Feb. 14, 2025, 11:49 a.m. UTC
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.

Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
 drivers/base/regmap/regmap-irq.c | 83 ++++++++++++++++++++++++++++++----------
 include/linux/regmap.h           |  3 ++
 2 files changed, 66 insertions(+), 20 deletions(-)

Comments

Andy Shevchenko Feb. 14, 2025, 3:18 p.m. UTC | #1
On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote:
> 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).

...

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

> +	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) {

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

Can be one line.

> +			goto exit;
> +		}
> +	}
> +
> +	ret = read_irq_data(data);
> +	if (ret < 0)
> +		goto exit;
> +
> +	if (chip->status_is_level) {
> +		for (i = 0; i < data->chip->num_regs; i++) {
> +			unsigned int val = data->status_buf[i];
> +
> +			data->status_buf[i] ^= data->prev_status_buf[i];
> +			data->prev_status_buf[i] = val;
> +		}
> +	}

...

> +		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.
Mathieu Dubois-Briand Feb. 14, 2025, 3:49 p.m. UTC | #2
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:
> > 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).
>
> ...
>
> > +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?

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

> ...
>
> > +		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.
Andy Shevchenko Feb. 14, 2025, 4:02 p.m. UTC | #3
On Fri, Feb 14, 2025 at 04:49:57PM +0100, Mathieu Dubois-Briand wrote:
> 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:

...

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

I would change in the code you touched,

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

Same, it falls under the "common sense" category.
diff mbox series

Patch

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 0bcd81389a29..531ea799383b 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -33,6 +33,7 @@  struct regmap_irq_chip_data {
 	void *status_reg_buf;
 	unsigned int *main_status_buf;
 	unsigned int *status_buf;
+	unsigned int *prev_status_buf;
 	unsigned int *mask_buf;
 	unsigned int *mask_buf_def;
 	unsigned int *wake_buf;
@@ -332,27 +333,13 @@  static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
 	return ret;
 }
 
-static irqreturn_t regmap_irq_thread(int irq, void *d)
+static int read_irq_data(struct regmap_irq_chip_data *data)
 {
-	struct regmap_irq_chip_data *data = d;
 	const struct regmap_irq_chip *chip = data->chip;
 	struct regmap *map = data->map;
 	int ret, i;
-	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) {
-			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
-				ret);
-			goto exit;
-		}
-	}
-
 	/*
 	 * Read only registers with active IRQs if the chip has 'main status
 	 * register'. Else read in the statuses, using a single bulk read if
@@ -382,7 +369,7 @@  static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev,
 					"Failed to read IRQ status %d\n",
 					ret);
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -401,7 +388,7 @@  static irqreturn_t regmap_irq_thread(int irq, void *d)
 					dev_err(map->dev,
 						"Failed to read IRQ status %d\n",
 						ret);
-					goto exit;
+					return ret;
 				}
 			}
 
@@ -420,7 +407,7 @@  static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
 				ret);
-			goto exit;
+			return ret;
 		}
 
 		for (i = 0; i < data->chip->num_regs; i++) {
@@ -436,7 +423,7 @@  static irqreturn_t regmap_irq_thread(int irq, void *d)
 				break;
 			default:
 				BUG();
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -450,7 +437,7 @@  static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev,
 					"Failed to read IRQ status: %d\n",
 					ret);
-				goto exit;
+				return ret;
 			}
 		}
 	}
@@ -459,6 +446,43 @@  static irqreturn_t regmap_irq_thread(int irq, void *d)
 		for (i = 0; i < data->chip->num_regs; i++)
 			data->status_buf[i] = ~data->status_buf[i];
 
+	return 0;
+}
+
+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;
+	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) {
+			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
+				ret);
+			goto exit;
+		}
+	}
+
+	ret = read_irq_data(data);
+	if (ret < 0)
+		goto exit;
+
+	if (chip->status_is_level) {
+		for (i = 0; i < data->chip->num_regs; i++) {
+			unsigned int val = data->status_buf[i];
+
+			data->status_buf[i] ^= data->prev_status_buf[i];
+			data->prev_status_buf[i] = val;
+		}
+	}
+
 	/*
 	 * Ignore masked IRQs and ack if we need to; we ack early so
 	 * there is no race between handling and acknowledging the
@@ -705,6 +729,13 @@  int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	if (!d->status_buf)
 		goto err_alloc;
 
+	if (chip->status_is_level) {
+		d->prev_status_buf = kcalloc(chip->num_regs, sizeof(*d->prev_status_buf),
+					     GFP_KERNEL);
+		if (!d->prev_status_buf)
+			goto err_alloc;
+	}
+
 	d->mask_buf = kcalloc(chip->num_regs, sizeof(*d->mask_buf),
 			      GFP_KERNEL);
 	if (!d->mask_buf)
@@ -881,6 +912,16 @@  int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 	}
 
+	/* Store current levels */
+	if (chip->status_is_level) {
+		ret = read_irq_data(d);
+		if (ret < 0)
+			goto err_alloc;
+
+		for (i = 0; i < d->chip->num_regs; i++)
+			d->prev_status_buf[i] = d->status_buf[i];
+	}
+
 	ret = regmap_irq_create_domain(fwnode, irq_base, chip, d);
 	if (ret)
 		goto err_alloc;
@@ -907,6 +948,7 @@  int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 	kfree(d->mask_buf_def);
 	kfree(d->mask_buf);
 	kfree(d->status_buf);
+	kfree(d->prev_status_buf);
 	kfree(d->status_reg_buf);
 	if (d->config_buf) {
 		for (i = 0; i < chip->num_config_bases; i++)
@@ -983,6 +1025,7 @@  void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 	kfree(d->mask_buf);
 	kfree(d->status_reg_buf);
 	kfree(d->status_buf);
+	kfree(d->prev_status_buf);
 	if (d->config_buf) {
 		for (i = 0; i < d->chip->num_config_bases; i++)
 			kfree(d->config_buf[i]);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3a96d068915f..159527e97f00 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1640,6 +1640,8 @@  struct regmap_irq_chip_data;
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @clear_ack:  Use this to set 1 and 0 or vice-versa to clear interrupts.
  * @status_invert: Inverted status register: cleared bits are active interrupts.
+ * @status_is_level: Status register is actuall signal level: Xor status
+ *		     register with previous value to get active interrupts.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_in_mask: Use the mask registers for controlling irq type. Use this if
  *		  the hardware provides separate bits for rising/falling edge
@@ -1703,6 +1705,7 @@  struct regmap_irq_chip {
 	unsigned int ack_invert:1;
 	unsigned int clear_ack:1;
 	unsigned int status_invert:1;
+	unsigned int status_is_level:1;
 	unsigned int wake_invert:1;
 	unsigned int type_in_mask:1;
 	unsigned int clear_on_unmask:1;