Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 21 Jun 2022 11:40:12 +0200 | Subject | Re: [PATCH 20/49] regmap-irq: Fix inverted handling of unmask registers |
| |
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);
-- With Best Regards, Andy Shevchenko
| |