On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald aidanmacdonald.0x0@gmail.com wrote:
To me "unmask" suggests that we write 1s to the register when an interrupt is enabled. This also makes sense because it's the opposite of what the "mask" register does (write 1s to disable an interrupt).
But regmap-irq does the opposite: for a disabled interrupt, it writes 1s to "unmask" and 0s to "mask". This is surprising and deviates from the usual way mask registers are handled.
Additionally, mask_invert didn't interact with unmask registers properly -- it caused them to be ignored entirely.
Fix this by making mask and unmask registers orthogonal, using the following behavior:
- Mask registers are written with 1s for disabled interrupts.
- Unmask registers are written with 1s for enabled interrupts.
This behavior supports both normal or inverted mask registers and separate set/clear registers via different combinations of mask_base/unmask_base. The mask_invert flag is made redundant, since an inverted mask register can be described more directly as an unmask register.
To cope with existing drivers that rely on the old "backward" behavior, check for the broken_mask_unmask flag and swap the roles of mask/unmask registers. This is a compatibility measure which can be dropped once the drivers are updated to use the new, more consistent behavior.
...
if (ret != 0)
if (ret)
dev_err(d->map->dev, "Failed to sync masks in %x\n",reg);
...
if (ret != 0)
Ditto.
dev_err(d->map->dev, "Failed to sync masks in %x\n",
...
/** Swap role of mask_base and unmask_base if mask bits are inverted.
the roles
** Historically, chips that specify both mask_base and unmask_base* got inverted mask behavior; this was arguably a bug in regmap-irq* and there was no way to get the normal, non-inverted behavior.* Those chips will set the broken_mask_unmask flag. They don't set* mask_invert so there is no need to worry about interactions with* that flag.*/
Reading this comment perhaps the code needs a validator part that will issue a WARN_ON / dev_warn() / etc in case where the above is not satisfied?
...
if (ret != 0) {
if (ret)
dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",reg, ret);
...
if (ret != 0) {
Ditto.
dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",reg, ret);