[v2,2/4] regmap-irq: Add handle_mask_sync() callback
Commit Message
Provide a public callback handle_mask_sync() that drivers can use when
they have more complex IRQ masking logic. The default implementation is
regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
callback.
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
drivers/base/regmap/regmap-irq.c | 44 ++++++++++++++++++++++----------
include/linux/regmap.h | 5 ++++
2 files changed, 36 insertions(+), 13 deletions(-)
Comments
On Thu, Nov 10, 2022 at 08:55:51PM -0500, William Breathitt Gray wrote:
> Provide a public callback handle_mask_sync() that drivers can use when
> they have more complex IRQ masking logic. The default implementation is
> regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
> callback.
...
> + * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
> + * in the range [0, num_regs[
Not sure if it's a typo ([ vs. ]), but if you want to say "not including the
last", use mathematical notation, i.e. "[0, num_regs)".
On Sun, Nov 13, 2022 at 02:42:49PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 10, 2022 at 08:55:51PM -0500, William Breathitt Gray wrote:
> > Provide a public callback handle_mask_sync() that drivers can use when
> > they have more complex IRQ masking logic. The default implementation is
> > regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
> > callback.
>
> ...
>
> > + * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
> > + * in the range [0, num_regs[
>
> Not sure if it's a typo ([ vs. ]), but if you want to say "not including the
> last", use mathematical notation, i.e. "[0, num_regs)".
I was following the convention used in the @get_irq_reg description, but
I agree that mathematical notation would be much clearer.
William Breathitt Gray
On Sun, Nov 13, 2022 at 08:08:40AM -0500, William Breathitt Gray wrote:
> On Sun, Nov 13, 2022 at 02:42:49PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 10, 2022 at 08:55:51PM -0500, William Breathitt Gray wrote:
> > > Provide a public callback handle_mask_sync() that drivers can use when
> > > they have more complex IRQ masking logic. The default implementation is
> > > regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
> > > callback.
...
> > > + * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
> > > + * in the range [0, num_regs[
> >
> > Not sure if it's a typo ([ vs. ]), but if you want to say "not including the
> > last", use mathematical notation, i.e. "[0, num_regs)".
>
> I was following the convention used in the @get_irq_reg description, but
> I agree that mathematical notation would be much clearer.
Ah, maybe cleaning up the rest then?
On Thu, Nov 10, 2022 at 08:55:51PM -0500, William Breathitt Gray wrote:
> Provide a public callback handle_mask_sync() that drivers can use when
> they have more complex IRQ masking logic. The default implementation is
> regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
> callback.
Can you provide examples of something that would make sense to
open code in a driver rather than factoring out? It looks like
this has been added due to one of the devices you're looking at
for some reason disabling it's upstream interrupt when all of the
downstream interrupts are masked, while weird that doesn't seem
especally device specific.
On Tue, Nov 15, 2022 at 05:14:14PM +0000, Mark Brown wrote:
> On Thu, Nov 10, 2022 at 08:55:51PM -0500, William Breathitt Gray wrote:
>
> > Provide a public callback handle_mask_sync() that drivers can use when
> > they have more complex IRQ masking logic. The default implementation is
> > regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
> > callback.
>
> Can you provide examples of something that would make sense to
> open code in a driver rather than factoring out? It looks like
> this has been added due to one of the devices you're looking at
> for some reason disabling it's upstream interrupt when all of the
> downstream interrupts are masked, while weird that doesn't seem
> especally device specific.
Sure, I actually intend to use this callback for the 104-idi-48 module
as well in the v3 submission so I'll describe that situations well.
For the 104-dio-48e we have the following:
Base Address +B (Write): Enable Interrupt
Base Address +B (Read): Disable Interrupt
Base Address +F (Read/Write): Clear Interrupt
So for 104-dio-48e, any write to 0xB will enable interrupts, while any
read will disable interrupts; interrupts are with either a read or any
write to 0xF. There's no status register either so software just has to
assume that if an interrupt is raised then it was for the
104-dio-48e device.
For the 104-idi-48, we do get a status register and some basic masking
but it's broken down by banks rather than individual GPIO; there are six
8-bit banks (Port 0 Low Byte, Port 0 Mid Byte, Port 0 High Byte, Port 1
Low Byte, Port 1 Mid Byte, Port 1 High Byte):
Base Address + 0 (Read/Write): Port 0 Low Byte
Base Address + 1 (Read/Write): Port 0 Mid Byte
Base Address + 2 (Read/Write): Port 0 High Byte
Base Address + 3: N/A
Base Address + 4 (Read/Write): Port 1 Low Byte
Base Address + 5 (Read/Write): Port 1 Mid Byte
Base Address + 6 (Read/Write): Port 1 High Byte
Base Address + 7 (Read): IRQ Status Register/IRQ Clear
Bit 0-5: Respective Bank IRQ Statuses
Bit 6: IRQ Status (Active Low)
Bit 7: IRQ Enable Status
Base Address + 7 (Write): IRQ Enable/Disable
Bit 0-5: Respective Bank IRQ Enable/Disable
In this case, masking a bank will mask all 8 GPIO within that bank;
so ideally I want a way to only mask a bank when all GPIO are masked,
and unmasking when at least one is unmasked.
Are there existing ways to support these kinds of configuration in
regmap_irq?
William Breathitt Gray
On Thu, Nov 17, 2022 at 10:00:17AM -0500, William Breathitt Gray wrote:
> On Tue, Nov 15, 2022 at 05:14:14PM +0000, Mark Brown wrote:
> > On Thu, Nov 10, 2022 at 08:55:51PM -0500, William Breathitt Gray wrote:
> >
> > > Provide a public callback handle_mask_sync() that drivers can use when
> > > they have more complex IRQ masking logic. The default implementation is
> > > regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
> > > callback.
> >
> > Can you provide examples of something that would make sense to
> > open code in a driver rather than factoring out? It looks like
> > this has been added due to one of the devices you're looking at
> > for some reason disabling it's upstream interrupt when all of the
> > downstream interrupts are masked, while weird that doesn't seem
> > especally device specific.
>
> Sure, I actually intend to use this callback for the 104-idi-48 module
> as well in the v3 submission so I'll describe that situations well.
>
> For the 104-dio-48e we have the following:
>
> Base Address +B (Write): Enable Interrupt
> Base Address +B (Read): Disable Interrupt
> Base Address +F (Read/Write): Clear Interrupt
>
> So for 104-dio-48e, any write to 0xB will enable interrupts, while any
> read will disable interrupts; interrupts are with either a read or any
> write to 0xF. There's no status register either so software just has to
> assume that if an interrupt is raised then it was for the
> 104-dio-48e device.
>
> For the 104-idi-48, we do get a status register and some basic masking
> but it's broken down by banks rather than individual GPIO; there are six
> 8-bit banks (Port 0 Low Byte, Port 0 Mid Byte, Port 0 High Byte, Port 1
> Low Byte, Port 1 Mid Byte, Port 1 High Byte):
>
> Base Address + 0 (Read/Write): Port 0 Low Byte
> Base Address + 1 (Read/Write): Port 0 Mid Byte
> Base Address + 2 (Read/Write): Port 0 High Byte
> Base Address + 3: N/A
> Base Address + 4 (Read/Write): Port 1 Low Byte
> Base Address + 5 (Read/Write): Port 1 Mid Byte
> Base Address + 6 (Read/Write): Port 1 High Byte
> Base Address + 7 (Read): IRQ Status Register/IRQ Clear
> Bit 0-5: Respective Bank IRQ Statuses
> Bit 6: IRQ Status (Active Low)
> Bit 7: IRQ Enable Status
> Base Address + 7 (Write): IRQ Enable/Disable
> Bit 0-5: Respective Bank IRQ Enable/Disable
>
> In this case, masking a bank will mask all 8 GPIO within that bank;
> so ideally I want a way to only mask a bank when all GPIO are masked,
> and unmasking when at least one is unmasked.
>
> Are there existing ways to support these kinds of configuration in
> regmap_irq?
>
> William Breathitt Gray
After trying to implement a handle_mask_sync() callback for the
104-idi-48 I discovered that it's not so straight-forward a task. The
mask_buf parameter is unsigned int so I can only represent 32 GPIO at a
time.
I could set the struct regmap_irq_chip num_regs member to '2' to
increase the number of mask_buf elements, but that creates side effects
because the regmap-irq API believes there there are more registers than
the device actually has.
For now with the 104-idi-48 module utilizing the regmap-irq API, we'll
have to leave it where masking one GPIO line masks the entire bank, and
vice versa for unmasking.
William Breathitt Gray
@@ -115,12 +115,20 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
*/
for (i = 0; i < d->chip->num_regs; i++) {
if (d->mask_base) {
- reg = d->get_irq_reg(d, d->mask_base, i);
- ret = regmap_update_bits(d->map, reg,
- d->mask_buf_def[i], d->mask_buf[i]);
- if (ret)
- dev_err(d->map->dev, "Failed to sync masks in %x\n",
- reg);
+ if (d->chip->handle_mask_sync)
+ d->chip->handle_mask_sync(d->map, i,
+ d->mask_buf_def[i],
+ d->mask_buf[i],
+ d->chip->irq_drv_data);
+ else {
+ reg = d->get_irq_reg(d, d->mask_base, i);
+ ret = regmap_update_bits(d->map, reg,
+ d->mask_buf_def[i],
+ d->mask_buf[i]);
+ if (ret)
+ dev_err(d->map->dev, "Failed to sync masks in %x\n",
+ reg);
+ }
}
if (d->unmask_base) {
@@ -917,13 +925,23 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
d->mask_buf[i] = d->mask_buf_def[i];
if (d->mask_base) {
- reg = d->get_irq_reg(d, d->mask_base, i);
- ret = regmap_update_bits(d->map, reg,
- d->mask_buf_def[i], d->mask_buf[i]);
- if (ret) {
- dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
- reg, ret);
- goto err_alloc;
+ if (chip->handle_mask_sync) {
+ ret = chip->handle_mask_sync(d->map, i,
+ d->mask_buf_def[i],
+ d->mask_buf[i],
+ chip->irq_drv_data);
+ if (ret)
+ goto err_alloc;
+ } else {
+ reg = d->get_irq_reg(d, d->mask_base, i);
+ ret = regmap_update_bits(d->map, reg,
+ d->mask_buf_def[i],
+ d->mask_buf[i]);
+ if (ret) {
+ dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
+ reg, ret);
+ goto err_alloc;
+ }
}
}
@@ -1542,6 +1542,8 @@ struct regmap_irq_chip_data;
* before regmap_irq_handler process the interrupts.
* @handle_post_irq: Driver specific callback to handle interrupt from device
* after handling the interrupts in regmap_irq_handler().
+ * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
+ * in the range [0, num_regs[
* @set_type_virt: Driver specific callback to extend regmap_irq_set_type()
* and configure virt regs. Deprecated, use @set_type_config
* callback and config registers instead.
@@ -1603,6 +1605,9 @@ struct regmap_irq_chip {
int (*handle_pre_irq)(void *irq_drv_data);
int (*handle_post_irq)(void *irq_drv_data);
+ int (*handle_mask_sync)(struct regmap *map, int index,
+ unsigned int mask_buf_def,
+ unsigned int mask_buf, void *irq_drv_data);
int (*set_type_virt)(unsigned int **buf, unsigned int type,
unsigned long hwirq, int reg);
int (*set_type_config)(unsigned int **buf, unsigned int type,