| 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:08 PM Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > No drivers currently use mask_writeonly, and in its current form > it seems a bit misleading. When set, mask registers will be > updated with regmap_write_bits() instead of regmap_update_bits(), > but regmap_write_bits() still does a read-modify-write under the > hood. It's not a write-only operation. > > Performing a simple regmap_write() is probably more useful, since > it can be used for chips that have separate set & clear registers > for controlling mask bits. Such registers are normally volatile > and read as 0, so avoiding a register read minimizes bus traffic. Reading your explanations and the code, I would rather think about fixing the regmap_write_bits() to be writeonly op. Otherwise it's unclear what's the difference between regmap_write_bits() vs. regmap_update_bits(). ... > if (d->chip->mask_writeonly) > - return regmap_write_bits(d->map, reg, mask, val); > + return regmap_write(d->map, reg, val & mask); > else > return regmap_update_bits(d->map, reg, mask, val);
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 >
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: >> >> No drivers currently use mask_writeonly, and in its current form >> it seems a bit misleading. When set, mask registers will be >> updated with regmap_write_bits() instead of regmap_update_bits(), >> but regmap_write_bits() still does a read-modify-write under the >> hood. It's not a write-only operation. >> >> Performing a simple regmap_write() is probably more useful, since >> it can be used for chips that have separate set & clear registers >> for controlling mask bits. Such registers are normally volatile >> and read as 0, so avoiding a register read minimizes bus traffic. > > Reading your explanations and the code, I would rather think about > fixing the regmap_write_bits() to be writeonly op. That's impossible without special hardware support. > Otherwise it's unclear what's the difference between > regmap_write_bits() vs. regmap_update_bits(). This was not obvious to me either. They're the same except in how they issue the low-level write op -- regmap_update_bits() will only do the write if the new value differs from the current one. regmap_write_bits() will always do a write, even if the new value is the same. I think the problem is lack of documentation. I only figured this out by reading the implementation. >> if (d->chip->mask_writeonly) >> - return regmap_write_bits(d->map, reg, mask, val); >> + return regmap_write(d->map, reg, val & mask); >> else >> return regmap_update_bits(d->map, reg, mask, val);
On Tuesday, June 21, 2022, Aidan MacDonald <aidanmacdonald.0x0@gmail.com> wrote: > > Andy Shevchenko <andy.shevchenko@gmail.com> writes: > > > On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald > > <aidanmacdonald.0x0@gmail.com> wrote: > >> > >> No drivers currently use mask_writeonly, and in its current form > >> it seems a bit misleading. When set, mask registers will be > >> updated with regmap_write_bits() instead of regmap_update_bits(), > >> but regmap_write_bits() still does a read-modify-write under the > >> hood. It's not a write-only operation. > >> > >> Performing a simple regmap_write() is probably more useful, since > >> it can be used for chips that have separate set & clear registers > >> for controlling mask bits. Such registers are normally volatile > >> and read as 0, so avoiding a register read minimizes bus traffic. > > > > Reading your explanations and the code, I would rather think about > > fixing the regmap_write_bits() to be writeonly op. > > That's impossible without special hardware support. > > > Otherwise it's unclear what's the difference between > > regmap_write_bits() vs. regmap_update_bits(). > > This was not obvious to me either. They're the same except in how they > issue the low-level write op -- regmap_update_bits() will only do the > write if the new value differs from the current one. regmap_write_bits() > will always do a write, even if the new value is the same. Okay, it makes a lot of sense for W1C type of bits in the register. Also, “reading” might imply to restore last value from cache, no? > > I think the problem is lack of documentation. I only figured this out > by reading the implementation. > > >> if (d->chip->mask_writeonly) > >> - return regmap_write_bits(d->map, reg, mask, val); > >> + return regmap_write(d->map, reg, val & mask); > >> else > >> return regmap_update_bits(d->map, reg, mask, val); >
Am 2022-06-20 22:06, schrieb Aidan MacDonald: > An inverted mask register can be described more directly > as an unmask register. > > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> Reviewed-by: Michael Walle <michael@walle.cc>
On 6/20/22 23:05, Aidan MacDonald wrote: > We need to divide the sub-irq status register offset by register > stride to get an index for the status buffer to avoid an out of > bounds write when the register stride is greater than 1. > > Fixes: a2d21848d921 ("regmap: regmap-irq: Add main status register support") > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > drivers/base/regmap/regmap-irq.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c > index 4f785bc7981c..a6db605707b0 100644 > --- a/drivers/base/regmap/regmap-irq.c > +++ b/drivers/base/regmap/regmap-irq.c > @@ -387,6 +387,7 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, > subreg = &chip->sub_reg_offsets[b]; > for (i = 0; i < subreg->num_regs; i++) { > unsigned int offset = subreg->offset[i]; > + unsigned int index = offset / map->reg_stride; > > if (chip->not_fixed_stride) > ret = regmap_read(map, > @@ -395,7 +396,7 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, > else > ret = regmap_read(map, > chip->status_base + offset, > - &data->status_buf[offset]); > + &data->status_buf[index]); > > if (ret) > break; Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>