| Message ID | 20220620200644.1961936-1-aidanmacdonald.0x0@gmail.com |
|---|---|
| Headers | show |
| Series | regmap-irq cleanups and refactoring | expand |
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > Config registers provide a more uniform approach to handling irq type > registers. They are essentially an extension of the virtual registers > used by the qcom-pm8008 driver. > > Config registers can be represented as a 2D array: > > config_base[0] reg0,0 reg0,1 reg0,2 reg0,3 > config_base[1] reg1,0 reg1,1 reg1,2 reg1,3 > config_base[2] reg2,0 reg2,1 reg2,2 reg2,3 > > There are 'num_config_bases' base registers, each of which is used to > address 'num_config_regs' registers. The addresses are calculated in > the same way as for other bases. It is assumed that an irq's type is > controlled by one column of registers; that column is identified by > the irq's 'type_reg_offset'. > > The set_type_config() callback is responsible for updating the config > register contents. It receives an array of buffers (each represents a > row of registers) and the index of the column to update, along with > the 'struct regmap_irq' description and requested irq type. > > Buffered values are written to registers in regmap_irq_sync_unlock(). > Note that the entire register contents are overwritten, which is a > minor change in behavior from type registers via 'type_base'. ... > + ret = regmap_write(map, reg, d->config_buf[i][j]); > + if (ret != 0) if (ret) > + dev_err(d->map->dev, > + "Failed to write config %x: %d\n", > + reg, ret); > + } ... > + * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback. > + * Redundant line. ... > + d->config_buf = kcalloc(chip->num_config_bases, > + sizeof(*d->config_buf), GFP_KERNEL); > + if (!d->config_buf) > + goto err_alloc; > + > + for (i = 0; i < chip->num_config_regs; i++) { > + d->config_buf[i] = kcalloc(chip->num_config_regs, > + sizeof(unsigned int), Can it be sizeof(**d->config_buf) ? > + GFP_KERNEL); > + if (!d->config_buf[i]) > + goto err_alloc; > + }
On Mon, Jun 20, 2022 at 10:07 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > Hi Mark, > > Here's a bunch of cleanups for regmap-irq focused on simplifying the API > and generalizing it a bit. It's broken up into three refactors, focusing > on one area at a time. > > * Patches 01 and 02 are straightforward bugfixes, independent of the > rest of the series. Neither of the bugs are triggered by in-tree > drivers but they might be worth picking up early anyhow. > > * Patches 03-13 clean up everything related to configuring IRQ types. > > * Patches 14-45 deal with mask/unmask registers. First, make unmask > registers behave more intuitively and usefully, and get rid of the > mask_invert flag in favor of describing inverted mask registers as > unmask registers. Second, make the mask_writeonly flag more useful > and enable it for two chips where it makes sense. > > * Patches 46-49 refactor sub_irq_reg() as a get_irq_reg() callback, > and use that to eliminate the not_fixed_stride flag. > > The approach I used when refactoring is pretty simple: (1) introduce new > functionality in regmap-irq, (2) convert the drivers, and (3) remove any > old code. Nothing should break in the middle. > > The patches can be re-ordered to some extent if that's preferable, but > it's best to add get_irq_reg() last to avoid having to think about how > it interacts with features that'll be removed anyway. > > I can't test most of the devices affected by this series so a lot of the > code is only build tested. I've tested on real hardware with my AXP192 > patchset[1], although it only provides limited code coverage. > > qcom-pm8008 in particular deserves careful testing - it used all of the > features touched by the refactors and required the most changes. Other > drivers only required trivial changes but there are three of them worth > mentioning: wcd943x, wcd9335, and wcd938x. They have suspicious looking > IRQ type definitions and I'm pretty sure aren't working properly, but > I can't fix them myself. The refactor shouldn't affect their behavior > so how / when / if they get fixed shouldn't be much of an issue. > > Oh, and I added the 'mask_writeonly' flag and volatile ranges to the > stpmic1 driver based on its datasheet[2] as a small optimization. It's > probably fine but testing would be a good idea. > > [1]: https://lore.kernel.org/linux-iio/20220618214009.2178567-1-aidanmacdonald.0x0@gmailcom/ > [2]: https://www.st.com/resource/en/datasheet/stpmic1.pdf Cool series, thanks for cleaning this up!
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > This flag is necessary to prepare for fixing the behavior of unmask > registers. Existing chips that set mask_base and unmask_base must > set broken_mask_unmask=1 to declare that they expect the mask bits Boolean should take true/false. > will be inverted in both registers, contrary to the usual behavior > of mask registers. > diff --git a/include/linux/regmap.h b/include/linux/regmap.h > index ee2567a0465c..21a70fd99493 100644 > --- a/include/linux/regmap.h > +++ b/include/linux/regmap.h > @@ -1523,6 +1523,7 @@ struct regmap_irq_chip { > bool clear_on_unmask:1; > bool not_fixed_stride:1; > bool status_invert:1; > + bool broken_mask_unmask:1; Looking at the given context, I would group it with clean_on_unmask above. The above is weird enough on its own. Can you prepare a precursor patch that either drops the bit fields of booleans or moves them to unsigned int? Note, bit fields in C are beasts when it goes to concurrent access. It would be nice to ensure these are not the cases of a such.
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > The STPMIC1 has a normal "1 to disable" mask register with > separate set and clear registers. It's relying on masks and > unmasks being inverted from their intuitive meaning, so it > needs the broken_mask_unmask flag. Same comment as per previous patch and continues to all patches of a kind.
On Mon, Jun 20, 2022 at 10:10 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > An inverted mask register can be described more directly > as an unmask register. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > drivers/gpio/gpio-sl28cpld.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-sl28cpld.c b/drivers/gpio/gpio-sl28cpld.c > index 52404736ac86..2195f88c2048 100644 > --- a/drivers/gpio/gpio-sl28cpld.c > +++ b/drivers/gpio/gpio-sl28cpld.c > @@ -70,8 +70,7 @@ static int sl28cpld_gpio_irq_init(struct platform_device *pdev, > irq_chip->num_irqs = ARRAY_SIZE(sl28cpld_gpio_irqs); > irq_chip->num_regs = 1; > irq_chip->status_base = base + GPIO_REG_IP; > - irq_chip->mask_base = base + GPIO_REG_IE; > - irq_chip->mask_invert = true; > + irq_chip->unmask_base = base + GPIO_REG_IE; > irq_chip->ack_base = base + GPIO_REG_IP; > > ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), > -- > 2.35.1 >
On Mon, Jun 20, 2022 at 10:12 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > Replace the internal sub_irq_reg() function with a public callback > that drivers can use when they have more complex register layouts. > The default implementation is regmap_irq_get_irq_reg_linear(), used > if the chip doesn't provide its own callback. ... > + /* > + * While possible that a user-defined get_irq_reg callback might be ->get_irq_reg() > + * linear enough to support bulk reads, most of the time it won't. > + * Therefore only allow them if the default callback is being used. > + */ > return !map->use_single_read && map->reg_stride == 1 && > - data->irq_reg_stride == 1; > + data->irq_reg_stride == 1 && > + data->get_irq_reg == regmap_irq_get_irq_reg_linear; If initially this had been as return _reg_stride && irq_reg_stride && !map->use_single_read; you would have done less changes by squeezing a new condition just in between the other two. It will preserve the grouping of the checks as well. > } ... > + /* > + * Note we can't use get_irq_reg() here because the offsets ->get_irq_reg() > + * in 'subreg' are *not* interchangeable with indices. > + */ ... > + /* > + * For not_fixed_stride, don't use get_irq_reg(). Ditto. > + * It would produce an incorrect result. > + */ ... > + reg = chip->main_status + > + (i * map->reg_stride * > + data->irq_reg_stride); Parentheses are not necessary. And perhaps the last two can be put on a single line. ... > + /* > + * NOTE: This is for backward compatibility only and will be removed FIXME ? > + * when not_fixed_stride is dropped (it's only used by qcom-pm8008). > + */
On Mon, Jun 20, 2022 at 09:05:55PM +0100, Aidan MacDonald wrote: > Here's a bunch of cleanups for regmap-irq focused on simplifying the API > and generalizing it a bit. It's broken up into three refactors, focusing > on one area at a time. This series is very large and the way it is interleaving patches for several different subsystems adds to the difficulty managing it. As you've identified there's several different subserieses in here, if you need to resend any of this (I've not even started looking at the actual patches yet) it would be easier to digest with some combination of sending as separate serieses and reordering things so that all the things for each subsystem are grouped together. That'd help with both review and with merging, both large serieses and cross subsystem dependencies tend to slow things down.
Mark Brown <broonie@kernel.org> writes: > On Mon, Jun 20, 2022 at 09:05:55PM +0100, Aidan MacDonald wrote: > >> Here's a bunch of cleanups for regmap-irq focused on simplifying the API >> and generalizing it a bit. It's broken up into three refactors, focusing >> on one area at a time. > > This series is very large and the way it is interleaving patches for > several different subsystems adds to the difficulty managing it. As > you've identified there's several different subserieses in here, if you > need to resend any of this (I've not even started looking at the actual > patches yet) it would be easier to digest with some combination of > sending as separate serieses and reordering things so that all the > things for each subsystem are grouped together. That'd help with both > review and with merging, both large serieses and cross subsystem > dependencies tend to slow things down. Thanks for the advice. After reading this and some of Andy's comments I think I understand how to organize this better. I'll send a trimmed down series including only regmap changes, and instead of removing things right away I'll just mark them deprecated. Then the driver patches can go by subsystem (one series per subsystem?) and once everything is merged the deprecated stuff in regmap-irq can be removed at a later date.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald > <aidanmacdonald.0x0@gmail.com> wrote: >> >> This flag is necessary to prepare for fixing the behavior of unmask >> registers. Existing chips that set mask_base and unmask_base must >> set broken_mask_unmask=1 to declare that they expect the mask bits > > Boolean should take true/false. > >> will be inverted in both registers, contrary to the usual behavior >> of mask registers. > >> diff --git a/include/linux/regmap.h b/include/linux/regmap.h >> index ee2567a0465c..21a70fd99493 100644 >> --- a/include/linux/regmap.h >> +++ b/include/linux/regmap.h >> @@ -1523,6 +1523,7 @@ struct regmap_irq_chip { >> bool clear_on_unmask:1; >> bool not_fixed_stride:1; >> bool status_invert:1; >> + bool broken_mask_unmask:1; > > Looking at the given context, I would group it with clean_on_unmask above. > > The above is weird enough on its own. Can you prepare a precursor > patch that either drops the bit fields of booleans or moves them to > unsigned int? Sure. > Note, bit fields in C are beasts when it goes to concurrent access. It > would be nice to ensure these are not the cases of a such. These are read-only so there's no danger here.
On Mon, 20 Jun 2022 21:05:55 +0100, Aidan MacDonald wrote: > Here's a bunch of cleanups for regmap-irq focused on simplifying the API > and generalizing it a bit. It's broken up into three refactors, focusing > on one area at a time. > > * Patches 01 and 02 are straightforward bugfixes, independent of the > rest of the series. Neither of the bugs are triggered by in-tree > drivers but they might be worth picking up early anyhow. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next Thanks! [01/49] regmap-irq: Fix a bug in regmap_irq_enable() for type_in_mask chips commit: 485037ae9a095491beb7f893c909a76cc4f9d1e7 [02/49] regmap-irq: Fix offset/index mismatch in read_sub_irq_data() commit: 3f05010f243be06478a9b11cfce0ce994f5a0890 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark