| Message ID | 53e9e89cc9d7e9c20cbdfc13b360dcb43d07f832.1668129763.git.william.gray@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Migrate i8255 GPIO drivers to regmap API | expand |
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
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 4ef9488d05cd..968681fa8d09 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -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; + } } } diff --git a/include/linux/regmap.h b/include/linux/regmap.h index ca3434dca3a0..62ede456af99 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -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,
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(-)