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(-)
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;
--
2.39.5On 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.
--
With Best Regards,
Andy Shevchenko
On Fri, Feb 14, 2025 at 05:18:17PM +0200, Andy Shevchenko wrote: > On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote: > > + int ret, i; > unsigned int i; > ? If it's just an iterator it's idiomatic to use signed ints. IIRC if it makes a difference to the code generation it's likely to be positive.
On Wed, Feb 26, 2025 at 01:18:16PM +0000, Mark Brown wrote: > On Fri, Feb 14, 2025 at 05:18:17PM +0200, Andy Shevchenko wrote: > > On Fri, Feb 14, 2025 at 12:49:56PM +0100, Mathieu Dubois-Briand wrote: > > > > + int ret, i; > > > unsigned int i; > > ? > > If it's just an iterator it's idiomatic to use signed ints. IIRC if it > makes a difference to the code generation it's likely to be positive. It depends on the subsystem, V4L2, for example, is strict to unsigned types when they are unsigned. It also helps to catch some strange conditionals at some point when one checks for negative value in the (incrementing) counter. But I'm not insisting as you may notice by question mark used. -- With Best Regards, Andy Shevchenko
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.
--
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
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.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.