[PATCH 00/49] regmap-irq cleanups and refactoring
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.0x... [2]: https://www.st.com/resource/en/datasheet/stpmic1.pdf
Aidan MacDonald (49): regmap-irq: Fix a bug in regmap_irq_enable() for type_in_mask chips regmap-irq: Fix offset/index mismatch in read_sub_irq_data() regmap-irq: Remove an unnecessary restriction on type_in_mask regmap-irq: Introduce config registers for irq types mfd: qcom-pm8008: Convert irq chip to config regs mfd: wcd934x: Convert irq chip to config regs sound: soc: codecs: wcd9335: Convert irq chip to config regs sound: soc: codecs: wcd938x: Remove spurious type_base from irq chip mfd: max77650: Remove useless type_invert flag regmap-irq: Remove virtual registers support regmap-irq: Remove old type register support, refactor regmap-irq: Remove unused type_reg_stride field regmap-irq: Remove unused type_invert flag regmap-irq: Do not use regmap_irq_update_bits() for wake regs regmap-irq: Change the behavior of mask_writeonly regmap-irq: Rename regmap_irq_update_bits() regmap-irq: Add broken_mask_unmask flag mfd: qcom-pm8008: Add broken_mask_unmask irq chip flag mfd: stpmic1: Add broken_mask_unmask irq chip flag regmap-irq: Fix inverted handling of unmask registers mfd: tps65090: replace irqchip mask_invert with unmask_base mfd: sun4i-gpadc: replace irqchip mask_invert with unmask_base mfd: sprd-sc27xx-spi: replace irqchip mask_invert with unmask_base mfd: rt5033: replace irqchip mask_invert with unmask_base mfd: rohm-bd71828: replace irqchip mask_invert with unmask_base mfd: rn5t618: replace irqchip mask_invert with unmask_base mfd: gateworks-gsc: replace irqchip mask_invert with unmask_base mfd: axp20x: replace irqchip mask_invert with unmask_base mfd: atc260x: replace irqchip mask_invert with unmask_base mfd: 88pm800: replace irqchip mask_invert with unmask_base mfd: max14577: replace irqchip mask_invert with unmask_base mfd: max77693: replace irqchip mask_invert with unmask_base mfd: rohm-bd718x7: drop useless mask_invert flag on irqchip mfd: max77843: drop useless mask_invert flag on irqchip extcon: max77843: replace irqchip mask_invert with unmask_base extcon: sm5502: drop useless mask_invert flag on irqchip extcon: rt8973a: drop useless mask_invert flag on irqchip irqchip: sl28cpld: replace irqchip mask_invert with unmask_base gpio: sl28cpld: replace irqchip mask_invert with unmask_base mfd: stpmic1: Fix broken mask/unmask in irq chip mfd: stpmic1: Enable mask_writeonly flag for irq chip mfd: qcom-pm8008: Fix broken mask/unmask in irq chip mfd: qcom-pm8008: Enable mask_writeonly flag for irq chip regmap-irq: Remove broken_mask_unmask flag regmap-irq: Remove mask_invert flag regmap-irq: Refactor checks for status bulk read support regmap-irq: Add get_irq_reg() callback mfd: qcom-pm8008: Use get_irq_reg() for irq chip regmap-irq: Remove not_fixed_stride flag
drivers/base/regmap/regmap-irq.c | 457 ++++++++++++++----------------- drivers/extcon/extcon-max77843.c | 3 +- drivers/extcon/extcon-rt8973a.c | 1 - drivers/extcon/extcon-sm5502.c | 2 - drivers/gpio/gpio-sl28cpld.c | 3 +- drivers/irqchip/irq-sl28cpld.c | 3 +- drivers/mfd/88pm800.c | 3 +- drivers/mfd/atc260x-core.c | 6 +- drivers/mfd/axp20x.c | 21 +- drivers/mfd/gateworks-gsc.c | 3 +- drivers/mfd/max14577.c | 7 +- drivers/mfd/max77650.c | 1 - drivers/mfd/max77693.c | 6 +- drivers/mfd/max77843.c | 1 - drivers/mfd/qcom-pm8008.c | 131 ++++----- drivers/mfd/rn5t618.c | 3 +- drivers/mfd/rohm-bd71828.c | 6 +- drivers/mfd/rohm-bd718x7.c | 1 - drivers/mfd/rt5033.c | 3 +- drivers/mfd/sprd-sc27xx-spi.c | 3 +- drivers/mfd/stpmic1.c | 7 +- drivers/mfd/sun4i-gpadc.c | 3 +- drivers/mfd/tps65090.c | 3 +- drivers/mfd/wcd934x.c | 11 +- include/linux/regmap.h | 59 ++-- sound/soc/codecs/wcd9335.c | 10 +- sound/soc/codecs/wcd938x.c | 1 - 27 files changed, 332 insertions(+), 426 deletions(-)
When enabling a type_in_mask irq, the type_buf contents must be AND'd with the mask of the IRQ we're enabling to avoid enabling other IRQs by accident, which can happen if several type_in_mask irqs share a mask register.
Fixes: bc998a730367 ("regmap: irq: handle HW using separate rising/falling edge interrupts") Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 400c7412a7dc..4f785bc7981c 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -252,6 +252,7 @@ static void regmap_irq_enable(struct irq_data *data) struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data); struct regmap *map = d->map; const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq); + unsigned int reg = irq_data->reg_offset / map->reg_stride; unsigned int mask, type;
type = irq_data->type.type_falling_val | irq_data->type.type_rising_val; @@ -268,14 +269,14 @@ static void regmap_irq_enable(struct irq_data *data) * at the corresponding offset in regmap_irq_set_type(). */ if (d->chip->type_in_mask && type) - mask = d->type_buf[irq_data->reg_offset / map->reg_stride]; + mask = d->type_buf[reg] & irq_data->mask; else mask = irq_data->mask;
if (d->chip->clear_on_unmask) d->clear_status = true;
- d->mask_buf[irq_data->reg_offset / map->reg_stride] &= ~mask; + d->mask_buf[reg] &= ~mask; }
static void regmap_irq_disable(struct irq_data *data)
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;
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
Check types_supported instead of checking type_rising/falling_val when using type_in_mask interrupts. This makes the intent clearer and allows a type_in_mask irq to support level or edge triggers, rather than only edge triggers. Update the comment to reflect the new behavior.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index a6db605707b0..59cfd4000e63 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -253,22 +253,19 @@ static void regmap_irq_enable(struct irq_data *data) struct regmap *map = d->map; const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq); unsigned int reg = irq_data->reg_offset / map->reg_stride; - unsigned int mask, type; - - type = irq_data->type.type_falling_val | irq_data->type.type_rising_val; + unsigned int mask;
/* * The type_in_mask flag means that the underlying hardware uses - * separate mask bits for rising and falling edge interrupts, but - * we want to make them into a single virtual interrupt with - * configurable edge. + * separate mask bits for each interrupt trigger type, but we want + * to have a single logical interrupt with a configurable type. * - * If the interrupt we're enabling defines the falling or rising - * masks then instead of using the regular mask bits for this - * interrupt, use the value previously written to the type buffer - * at the corresponding offset in regmap_irq_set_type(). + * If the interrupt we're enabling defines any supported types + * then instead of using the regular mask bits for this interrupt, + * use the value previously written to the type buffer at the + * corresponding offset in regmap_irq_set_type(). */ - if (d->chip->type_in_mask && type) + if (d->chip->type_in_mask && irq_data->type.types_supported) mask = d->type_buf[reg] & irq_data->mask; else mask = irq_data->mask;
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'.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 102 ++++++++++++++++++++++++++++++- include/linux/regmap.h | 12 ++++ 2 files changed, 113 insertions(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 59cfd4000e63..be35f2e41b8c 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -39,6 +39,7 @@ struct regmap_irq_chip_data { unsigned int *type_buf; unsigned int *type_buf_def; unsigned int **virt_buf; + unsigned int **config_buf;
unsigned int irq_reg_stride; unsigned int type_reg_stride; @@ -231,6 +232,17 @@ static void regmap_irq_sync_unlock(struct irq_data *data) } }
+ for (i = 0; i < d->chip->num_config_bases; i++) { + for (j = 0; j < d->chip->num_config_regs; j++) { + reg = sub_irq_reg(d, d->chip->config_base[i], j); + ret = regmap_write(map, reg, d->config_buf[i][j]); + if (ret != 0) + dev_err(d->map->dev, + "Failed to write config %x: %d\n", + reg, ret); + } + } + if (d->chip->runtime_pm) pm_runtime_put(map->dev);
@@ -298,6 +310,10 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
reg = t->type_reg_offset / map->reg_stride;
+ if (d->chip->set_type_config) + return d->chip->set_type_config(d->config_buf, type, + irq_data, reg); + if (t->type_reg_mask) d->type_buf[reg] &= ~t->type_reg_mask; else @@ -603,6 +619,62 @@ static const struct irq_domain_ops regmap_domain_ops = { .xlate = irq_domain_xlate_onetwocell, };
+/** + * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback. + * + * @buf: Buffer containing configuration register values, this is a 2D array of + * `num_config_bases` rows, each of `num_config_regs` elements. + * @type: The requested IRQ type. + * @irq_data: The IRQ being configured. + * @idx: Index of the irq's config registers within each array `buf[i]` + * + * This is a &struct regmap_irq_chip->set_type_config callback suitable for + * chips with one config register. Register values are updated according to + * the &struct regmap_irq_type data associated with an IRQ. + */ +int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type, + const struct regmap_irq *irq_data, int idx) +{ + const struct regmap_irq_type *t = &irq_data->type; + + if (t->type_reg_mask) + buf[0][idx] &= ~t->type_reg_mask; + else + buf[0][idx] &= ~(t->type_falling_val | + t->type_rising_val | + t->type_level_low_val | + t->type_level_high_val); + + switch (type) { + case IRQ_TYPE_EDGE_FALLING: + buf[0][idx] |= t->type_falling_val; + break; + + case IRQ_TYPE_EDGE_RISING: + buf[0][idx] |= t->type_rising_val; + break; + + case IRQ_TYPE_EDGE_BOTH: + buf[0][idx] |= (t->type_falling_val | + t->type_rising_val); + break; + + case IRQ_TYPE_LEVEL_HIGH: + buf[0][idx] |= t->type_level_high_val; + break; + + case IRQ_TYPE_LEVEL_LOW: + buf[0][idx] |= t->type_level_low_val; + break; + + default: + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL_GPL(regmap_irq_set_type_config_simple); + /** * regmap_add_irq_chip_fwnode() - Use standard regmap IRQ controller handling * @@ -728,6 +800,24 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, } }
+ if (chip->num_config_bases && chip->num_config_regs) { + /* + * Create config_buf[num_config_bases][num_config_regs] + */ + 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), + GFP_KERNEL); + if (!d->config_buf[i]) + goto err_alloc; + } + } + d->irq_chip = regmap_irq_chip; d->irq_chip.name = chip->name; d->irq = irq; @@ -904,6 +994,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, kfree(d->virt_buf[i]); kfree(d->virt_buf); } + if (d->config_buf) { + for (i = 0; i < chip->num_config_bases; i++) + kfree(d->config_buf[i]); + kfree(d->config_buf); + } kfree(d); return ret; } @@ -944,7 +1039,7 @@ EXPORT_SYMBOL_GPL(regmap_add_irq_chip); void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d) { unsigned int virq; - int hwirq; + int i, hwirq;
if (!d) return; @@ -974,6 +1069,11 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d) kfree(d->mask_buf); kfree(d->status_reg_buf); kfree(d->status_buf); + if (d->config_buf) { + for (i = 0; i < d->chip->num_config_bases; i++) + kfree(d->config_buf[i]); + kfree(d->config_buf); + } kfree(d); } EXPORT_SYMBOL_GPL(regmap_del_irq_chip); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 8952fa3d0d59..e48d65756fb4 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1460,6 +1460,7 @@ struct regmap_irq_sub_irq_map { * @wake_base: Base address for wake enables. If zero unsupported. * @type_base: Base address for irq type. If zero unsupported. * @virt_reg_base: Base addresses for extra config regs. + * @config_base: Base address for IRQ type config regs. If null unsupported. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. * @mask_invert: Inverted mask register: cleared bits are masked out. @@ -1489,12 +1490,15 @@ struct regmap_irq_sub_irq_map { * If zero unsupported. * @type_reg_stride: Stride to use for chips where type registers are not * contiguous. + * @num_config_bases: Number of config base registers. + * @num_config_regs: Number of config registers for each config base register. * @handle_pre_irq: Driver specific callback to handle interrupt from device * 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(). * @set_type_virt: Driver specific callback to extend regmap_irq_set_type() * and configure virt regs. + * @set_type_config: Callback used for configuring irq types. * @irq_drv_data: Driver specific IRQ data which is passed as parameter when * driver specific pre/post interrupt handler is called. * @@ -1517,6 +1521,7 @@ struct regmap_irq_chip { unsigned int wake_base; unsigned int type_base; unsigned int *virt_reg_base; + const unsigned int *config_base; unsigned int irq_reg_stride; bool mask_writeonly:1; bool init_ack_masked:1; @@ -1539,17 +1544,24 @@ struct regmap_irq_chip {
int num_type_reg; int num_virt_regs; + int num_config_bases; + int num_config_regs; unsigned int type_reg_stride;
int (*handle_pre_irq)(void *irq_drv_data); int (*handle_post_irq)(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, + const struct regmap_irq *irq_data, int idx); void *irq_drv_data; };
struct regmap_irq_chip_data;
+int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type, + const struct regmap_irq *irq_data, int idx); + int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags, int irq_base, const struct regmap_irq_chip *chip, struct regmap_irq_chip_data **data);
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,3There 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;}
Use config registers to simplify the driver, putting all of the code for irq type configuration in one place instead of splitting it up arbitrarily between type and virtual registers.
Remove the initial register setting in pm8008_init(). The comment indicates this is a hack to work around quirks in regmap-irq, but this is not necessary if using config registers.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/qcom-pm8008.c | 76 +++++++++++---------------------------- 1 file changed, 21 insertions(+), 55 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c index c472d7f8103c..da16566f7883 100644 --- a/drivers/mfd/qcom-pm8008.c +++ b/drivers/mfd/qcom-pm8008.c @@ -73,15 +73,16 @@ static struct regmap_irq_sub_irq_map pm8008_sub_reg_offsets[] = { REGMAP_IRQ_MAIN_REG_OFFSET(p3_offs), };
-static unsigned int pm8008_virt_regs[] = { +static unsigned int pm8008_config_regs[] = { + PM8008_TYPE_BASE, PM8008_POLARITY_HI_BASE, PM8008_POLARITY_LO_BASE, };
enum { + SET_TYPE_INDEX, POLARITY_HI_INDEX, POLARITY_LO_INDEX, - PM8008_NUM_VIRT_REGS, };
static struct regmap_irq pm8008_irqs[] = { @@ -95,32 +96,36 @@ static struct regmap_irq pm8008_irqs[] = { REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)), };
-static int pm8008_set_type_virt(unsigned int **virt_buf, - unsigned int type, unsigned long hwirq, - int reg) +static int pm8008_set_type_config(unsigned int **buf, unsigned int type, + const struct regmap_irq *irq_data, int idx) { switch (type) { case IRQ_TYPE_EDGE_FALLING: case IRQ_TYPE_LEVEL_LOW: - virt_buf[POLARITY_HI_INDEX][reg] &= ~pm8008_irqs[hwirq].mask; - virt_buf[POLARITY_LO_INDEX][reg] |= pm8008_irqs[hwirq].mask; + buf[POLARITY_HI_INDEX][idx] &= ~irq_data->mask; + buf[POLARITY_LO_INDEX][idx] |= irq_data->mask; break;
case IRQ_TYPE_EDGE_RISING: case IRQ_TYPE_LEVEL_HIGH: - virt_buf[POLARITY_HI_INDEX][reg] |= pm8008_irqs[hwirq].mask; - virt_buf[POLARITY_LO_INDEX][reg] &= ~pm8008_irqs[hwirq].mask; + buf[POLARITY_HI_INDEX][idx] |= irq_data->mask; + buf[POLARITY_LO_INDEX][idx] &= ~irq_data->mask; break;
case IRQ_TYPE_EDGE_BOTH: - virt_buf[POLARITY_HI_INDEX][reg] |= pm8008_irqs[hwirq].mask; - virt_buf[POLARITY_LO_INDEX][reg] |= pm8008_irqs[hwirq].mask; + buf[POLARITY_HI_INDEX][idx] |= irq_data->mask; + buf[POLARITY_LO_INDEX][idx] |= irq_data->mask; break;
default: return -EINVAL; }
+ if (type & IRQ_TYPE_EDGE_BOTH) + buf[SET_TYPE_INDEX][idx] |= irq_data->mask; + else + buf[SET_TYPE_INDEX][idx] &= ~irq_data->mask; + return 0; }
@@ -128,20 +133,19 @@ static struct regmap_irq_chip pm8008_irq_chip = { .name = "pm8008_irq", .main_status = I2C_INTR_STATUS_BASE, .num_main_regs = 1, - .num_virt_regs = PM8008_NUM_VIRT_REGS, .irqs = pm8008_irqs, .num_irqs = ARRAY_SIZE(pm8008_irqs), .num_regs = PM8008_NUM_PERIPHS, .not_fixed_stride = true, .sub_reg_offsets = pm8008_sub_reg_offsets, - .set_type_virt = pm8008_set_type_virt, .status_base = PM8008_STATUS_BASE, .mask_base = PM8008_MASK_BASE, .unmask_base = PM8008_UNMASK_BASE, - .type_base = PM8008_TYPE_BASE, .ack_base = PM8008_ACK_BASE, - .virt_reg_base = pm8008_virt_regs, - .num_type_reg = PM8008_NUM_PERIPHS, + .config_base = pm8008_config_regs, + .num_config_bases = ARRAY_SIZE(pm8008_config_regs), + .num_config_regs = PM8008_NUM_PERIPHS, + .set_type_config = pm8008_set_type_config, };
static struct regmap_config qcom_mfd_regmap_cfg = { @@ -150,34 +154,6 @@ static struct regmap_config qcom_mfd_regmap_cfg = { .max_register = 0xFFFF, };
-static int pm8008_init(struct pm8008_data *chip) -{ - int rc; - - /* - * Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework - * reads this as the default value instead of zero, the HW default. - * This is required to enable the writing of TYPE registers in - * regmap_irq_sync_unlock(). - */ - rc = regmap_write(chip->regmap, - (PM8008_TEMP_ALARM_ADDR | INT_SET_TYPE_OFFSET), - BIT(0)); - if (rc) - return rc; - - /* Do the same for GPIO1 and GPIO2 peripherals */ - rc = regmap_write(chip->regmap, - (PM8008_GPIO1_ADDR | INT_SET_TYPE_OFFSET), BIT(0)); - if (rc) - return rc; - - rc = regmap_write(chip->regmap, - (PM8008_GPIO2_ADDR | INT_SET_TYPE_OFFSET), BIT(0)); - - return rc; -} - static int pm8008_probe_irq_peripherals(struct pm8008_data *chip, int client_irq) { @@ -185,20 +161,10 @@ static int pm8008_probe_irq_peripherals(struct pm8008_data *chip, struct regmap_irq_type *type; struct regmap_irq_chip_data *irq_data;
- rc = pm8008_init(chip); - if (rc) { - dev_err(chip->dev, "Init failed: %d\n", rc); - return rc; - } - for (i = 0; i < ARRAY_SIZE(pm8008_irqs); i++) { type = &pm8008_irqs[i].type;
- type->type_reg_offset = pm8008_irqs[i].reg_offset; - type->type_rising_val = pm8008_irqs[i].mask; - type->type_falling_val = pm8008_irqs[i].mask; - type->type_level_high_val = 0; - type->type_level_low_val = 0; + type->type_reg_offset = pm8008_irqs[i].reg_offset;
if (type->type_reg_offset == PM8008_MISC) type->types_supported = IRQ_TYPE_EDGE_RISING;
Switch the driver to config registers. This will allow the old type register code in regmap-irq to be removed.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/wcd934x.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/wcd934x.c b/drivers/mfd/wcd934x.c index 68e2fa2fda99..07e884087f2c 100644 --- a/drivers/mfd/wcd934x.c +++ b/drivers/mfd/wcd934x.c @@ -55,17 +55,22 @@ static const struct regmap_irq wcd934x_irqs[] = { WCD934X_REGMAP_IRQ_REG(WCD934X_IRQ_SOUNDWIRE, 2, BIT(4)), };
+static const unsigned int wcd934x_config_regs[] = { + WCD934X_INTR_LEVEL0, +}; + static const struct regmap_irq_chip wcd934x_regmap_irq_chip = { .name = "wcd934x_irq", .status_base = WCD934X_INTR_PIN1_STATUS0, .mask_base = WCD934X_INTR_PIN1_MASK0, .ack_base = WCD934X_INTR_PIN1_CLEAR0, - .type_base = WCD934X_INTR_LEVEL0, - .num_type_reg = 4, - .type_in_mask = false, .num_regs = 4, .irqs = wcd934x_irqs, .num_irqs = ARRAY_SIZE(wcd934x_irqs), + .config_base = wcd934x_config_regs, + .num_config_bases = ARRAY_SIZE(wcd934x_config_regs), + .num_config_regs = 4, + .set_type_config = regmap_irq_set_type_config_simple, };
static bool wcd934x_is_volatile_register(struct device *dev, unsigned int reg)
Switch the driver to config registers. This will allow the old type register code in regmap-irq to be removed.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- sound/soc/codecs/wcd9335.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 617a36a89dfe..727d4436142a 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -5020,16 +5020,22 @@ static const struct regmap_irq wcd9335_codec_irqs[] = { }, };
+static const unsigned int wcd9335_config_regs[] = { + WCD9335_INTR_LEVEL0, +}; + static const struct regmap_irq_chip wcd9335_regmap_irq1_chip = { .name = "wcd9335_pin1_irq", .status_base = WCD9335_INTR_PIN1_STATUS0, .mask_base = WCD9335_INTR_PIN1_MASK0, .ack_base = WCD9335_INTR_PIN1_CLEAR0, - .type_base = WCD9335_INTR_LEVEL0, - .num_type_reg = 4, .num_regs = 4, .irqs = wcd9335_codec_irqs, .num_irqs = ARRAY_SIZE(wcd9335_codec_irqs), + .config_base = wcd9335_config_regs, + .num_config_bases = ARRAY_SIZE(wcd9335_config_regs), + .num_config_regs = 4, + .set_type_config = regmap_irq_set_type_config_simple, };
static int wcd9335_parse_dt(struct wcd9335_codec *wcd)
This chip does not set num_type_regs or define any supported IRQ types, so regmap-irq can't configure its IRQ types. Including type_base in the chip definition is therefore redundant.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- sound/soc/codecs/wcd938x.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c index c1b61b997f69..acba253b791e 100644 --- a/sound/soc/codecs/wcd938x.c +++ b/sound/soc/codecs/wcd938x.c @@ -1298,7 +1298,6 @@ static struct regmap_irq_chip wcd938x_regmap_irq_chip = { .num_regs = 3, .status_base = WCD938X_DIGITAL_INTR_STATUS_0, .mask_base = WCD938X_DIGITAL_INTR_MASK_0, - .type_base = WCD938X_DIGITAL_INTR_LEVEL_0, .ack_base = WCD938X_DIGITAL_INTR_CLEAR_0, .use_ack = 1, .runtime_pm = true,
The type_invert flag does nothing when type_in_mask is set, so get rid of it.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/max77650.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c index 777485a33bc0..3c07fcdd9d07 100644 --- a/drivers/mfd/max77650.c +++ b/drivers/mfd/max77650.c @@ -138,7 +138,6 @@ static const struct regmap_irq_chip max77650_irq_chip = { .status_base = MAX77650_REG_INT_GLBL, .mask_base = MAX77650_REG_INTM_GLBL, .type_in_mask = true, - .type_invert = true, .init_ack_masked = true, .clear_on_unmask = true, };
Virtual registers can be removed, since config registers implement the same functionality.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 42 -------------------------------- include/linux/regmap.h | 9 ------- 2 files changed, 51 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index be35f2e41b8c..5a3e255816fd 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -38,7 +38,6 @@ struct regmap_irq_chip_data { unsigned int *wake_buf; unsigned int *type_buf; unsigned int *type_buf_def; - unsigned int **virt_buf; unsigned int **config_buf;
unsigned int irq_reg_stride; @@ -218,20 +217,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data) } }
- if (d->chip->num_virt_regs) { - for (i = 0; i < d->chip->num_virt_regs; i++) { - for (j = 0; j < d->chip->num_regs; j++) { - reg = sub_irq_reg(d, d->chip->virt_reg_base[i], - j); - ret = regmap_write(map, reg, d->virt_buf[i][j]); - if (ret != 0) - dev_err(d->map->dev, - "Failed to write virt 0x%x: %d\n", - reg, ret); - } - } - } - for (i = 0; i < d->chip->num_config_bases; i++) { for (j = 0; j < d->chip->num_config_regs; j++) { reg = sub_irq_reg(d, d->chip->config_base[i], j); @@ -346,10 +331,6 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type) return -EINVAL; }
- if (d->chip->set_type_virt) - return d->chip->set_type_virt(d->virt_buf, type, data->hwirq, - reg); - return 0; }
@@ -782,24 +763,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; }
- if (chip->num_virt_regs) { - /* - * Create virt_buf[chip->num_extra_config_regs][chip->num_regs] - */ - d->virt_buf = kcalloc(chip->num_virt_regs, sizeof(*d->virt_buf), - GFP_KERNEL); - if (!d->virt_buf) - goto err_alloc; - - for (i = 0; i < chip->num_virt_regs; i++) { - d->virt_buf[i] = kcalloc(chip->num_regs, - sizeof(unsigned int), - GFP_KERNEL); - if (!d->virt_buf[i]) - goto err_alloc; - } - } - if (chip->num_config_bases && chip->num_config_regs) { /* * Create config_buf[num_config_bases][num_config_regs] @@ -989,11 +952,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, kfree(d->mask_buf); kfree(d->status_buf); kfree(d->status_reg_buf); - if (d->virt_buf) { - for (i = 0; i < chip->num_virt_regs; i++) - kfree(d->virt_buf[i]); - kfree(d->virt_buf); - } if (d->config_buf) { for (i = 0; i < chip->num_config_bases; i++) kfree(d->config_buf[i]); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index e48d65756fb4..bb8c89a83b51 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1459,7 +1459,6 @@ struct regmap_irq_sub_irq_map { * Using zero value is possible with @use_ack bit. * @wake_base: Base address for wake enables. If zero unsupported. * @type_base: Base address for irq type. If zero unsupported. - * @virt_reg_base: Base addresses for extra config regs. * @config_base: Base address for IRQ type config regs. If null unsupported. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. @@ -1486,8 +1485,6 @@ struct regmap_irq_sub_irq_map { * assigned based on the index in the array of the interrupt. * @num_irqs: Number of descriptors. * @num_type_reg: Number of type registers. - * @num_virt_regs: Number of non-standard irq configuration registers. - * If zero unsupported. * @type_reg_stride: Stride to use for chips where type registers are not * contiguous. * @num_config_bases: Number of config base registers. @@ -1496,8 +1493,6 @@ struct regmap_irq_sub_irq_map { * 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(). - * @set_type_virt: Driver specific callback to extend regmap_irq_set_type() - * and configure virt regs. * @set_type_config: Callback used for configuring irq types. * @irq_drv_data: Driver specific IRQ data which is passed as parameter when * driver specific pre/post interrupt handler is called. @@ -1520,7 +1515,6 @@ struct regmap_irq_chip { unsigned int ack_base; unsigned int wake_base; unsigned int type_base; - unsigned int *virt_reg_base; const unsigned int *config_base; unsigned int irq_reg_stride; bool mask_writeonly:1; @@ -1543,15 +1537,12 @@ struct regmap_irq_chip { int num_irqs;
int num_type_reg; - int num_virt_regs; int num_config_bases; int num_config_regs; unsigned int type_reg_stride;
int (*handle_pre_irq)(void *irq_drv_data); int (*handle_post_irq)(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, const struct regmap_irq *irq_data, int idx); void *irq_drv_data;
Now that all users have been converted to use config registers for setting IRQ types, the old type register handling code can be removed. Also refactor the parts related to type_in_mask.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 102 +++++-------------------------- include/linux/regmap.h | 4 -- 2 files changed, 14 insertions(+), 92 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 5a3e255816fd..85d7fd4e07d7 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -36,8 +36,7 @@ struct regmap_irq_chip_data { unsigned int *mask_buf; unsigned int *mask_buf_def; unsigned int *wake_buf; - unsigned int *type_buf; - unsigned int *type_buf_def; + unsigned int *mask_type_buf; unsigned int **config_buf;
unsigned int irq_reg_stride; @@ -199,24 +198,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data) } }
- /* Don't update the type bits if we're using mask bits for irq type. */ - if (!d->chip->type_in_mask) { - for (i = 0; i < d->chip->num_type_reg; i++) { - if (!d->type_buf_def[i]) - continue; - reg = sub_irq_reg(d, d->chip->type_base, i); - if (d->chip->type_invert) - ret = regmap_irq_update_bits(d, reg, - d->type_buf_def[i], ~d->type_buf[i]); - else - ret = regmap_irq_update_bits(d, reg, - d->type_buf_def[i], d->type_buf[i]); - if (ret != 0) - dev_err(d->map->dev, "Failed to sync type in %x\n", - reg); - } - } - for (i = 0; i < d->chip->num_config_bases; i++) { for (j = 0; j < d->chip->num_config_regs; j++) { reg = sub_irq_reg(d, d->chip->config_base[i], j); @@ -259,11 +240,11 @@ static void regmap_irq_enable(struct irq_data *data) * * If the interrupt we're enabling defines any supported types * then instead of using the regular mask bits for this interrupt, - * use the value previously written to the type buffer at the + * use the value previously written to the mask_type buffer at the * corresponding offset in regmap_irq_set_type(). */ if (d->chip->type_in_mask && irq_data->type.types_supported) - mask = d->type_buf[reg] & irq_data->mask; + mask = d->mask_type_buf[reg] & irq_data->mask; else mask = irq_data->mask;
@@ -287,50 +268,21 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type) struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data); struct regmap *map = d->map; const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq); - int reg; const struct regmap_irq_type *t = &irq_data->type; + unsigned int reg;
- if ((t->types_supported & type) != type) + if ((irq_data->type.types_supported & type) != type) return 0;
reg = t->type_reg_offset / map->reg_stride;
+ if (d->chip->type_in_mask) + return regmap_irq_set_type_config_simple(&d->mask_type_buf, + type, irq_data, reg); if (d->chip->set_type_config) return d->chip->set_type_config(d->config_buf, type, irq_data, reg);
- if (t->type_reg_mask) - d->type_buf[reg] &= ~t->type_reg_mask; - else - d->type_buf[reg] &= ~(t->type_falling_val | - t->type_rising_val | - t->type_level_low_val | - t->type_level_high_val); - switch (type) { - case IRQ_TYPE_EDGE_FALLING: - d->type_buf[reg] |= t->type_falling_val; - break; - - case IRQ_TYPE_EDGE_RISING: - d->type_buf[reg] |= t->type_rising_val; - break; - - case IRQ_TYPE_EDGE_BOTH: - d->type_buf[reg] |= (t->type_falling_val | - t->type_rising_val); - break; - - case IRQ_TYPE_LEVEL_HIGH: - d->type_buf[reg] |= t->type_level_high_val; - break; - - case IRQ_TYPE_LEVEL_LOW: - d->type_buf[reg] |= t->type_level_low_val; - break; - default: - return -EINVAL; - } - return 0; }
@@ -682,7 +634,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, struct regmap_irq_chip_data *d; int i; int ret = -ENOMEM; - int num_type_reg; u32 reg; u32 unmask_offset;
@@ -750,16 +701,10 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; }
- num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg; - if (num_type_reg) { - d->type_buf_def = kcalloc(num_type_reg, - sizeof(unsigned int), GFP_KERNEL); - if (!d->type_buf_def) - goto err_alloc; - - d->type_buf = kcalloc(num_type_reg, sizeof(unsigned int), - GFP_KERNEL); - if (!d->type_buf) + if (chip->type_in_mask) { + d->mask_type_buf = kcalloc(chip->num_regs, + sizeof(unsigned int), GFP_KERNEL); + if (!d->mask_type_buf) goto err_alloc; }
@@ -899,23 +844,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, } }
- if (chip->num_type_reg && !chip->type_in_mask) { - for (i = 0; i < chip->num_type_reg; ++i) { - reg = sub_irq_reg(d, d->chip->type_base, i); - - ret = regmap_read(map, reg, &d->type_buf_def[i]); - - if (d->chip->type_invert) - d->type_buf_def[i] = ~d->type_buf_def[i]; - - if (ret) { - dev_err(map->dev, "Failed to get type defaults at 0x%x: %d\n", - reg, ret); - goto err_alloc; - } - } - } - if (irq_base) d->domain = irq_domain_create_legacy(fwnode, chip->num_irqs, irq_base, 0, @@ -945,8 +873,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, err_domain: /* Should really dispose of the domain but... */ err_alloc: - kfree(d->type_buf); - kfree(d->type_buf_def); + kfree(d->mask_type_buf); kfree(d->wake_buf); kfree(d->mask_buf_def); kfree(d->mask_buf); @@ -1020,8 +947,7 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d) }
irq_domain_remove(d->domain); - kfree(d->type_buf); - kfree(d->type_buf_def); + kfree(d->mask_type_buf); kfree(d->wake_buf); kfree(d->mask_buf_def); kfree(d->mask_buf); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index bb8c89a83b51..879afdc81526 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1458,7 +1458,6 @@ struct regmap_irq_sub_irq_map { * @ack_base: Base ack address. If zero then the chip is clear on read. * Using zero value is possible with @use_ack bit. * @wake_base: Base address for wake enables. If zero unsupported. - * @type_base: Base address for irq type. If zero unsupported. * @config_base: Base address for IRQ type config regs. If null unsupported. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. @@ -1484,7 +1483,6 @@ struct regmap_irq_sub_irq_map { * @irqs: Descriptors for individual IRQs. Interrupt numbers are * assigned based on the index in the array of the interrupt. * @num_irqs: Number of descriptors. - * @num_type_reg: Number of type registers. * @type_reg_stride: Stride to use for chips where type registers are not * contiguous. * @num_config_bases: Number of config base registers. @@ -1514,7 +1512,6 @@ struct regmap_irq_chip { unsigned int unmask_base; unsigned int ack_base; unsigned int wake_base; - unsigned int type_base; const unsigned int *config_base; unsigned int irq_reg_stride; bool mask_writeonly:1; @@ -1536,7 +1533,6 @@ struct regmap_irq_chip { const struct regmap_irq *irqs; int num_irqs;
- int num_type_reg; int num_config_bases; int num_config_regs; unsigned int type_reg_stride;
It appears that no chip ever required a nonzero type_reg_stride and commit 1066cfbdfa3f ("regmap-irq: Extend sub-irq to support non-fixed reg strides") broke support. Just remove the field.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 6 ------ include/linux/regmap.h | 3 --- 2 files changed, 9 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 85d7fd4e07d7..b24818ad36e6 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -40,7 +40,6 @@ struct regmap_irq_chip_data { unsigned int **config_buf;
unsigned int irq_reg_stride; - unsigned int type_reg_stride;
bool clear_status:1; }; @@ -738,11 +737,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, else d->irq_reg_stride = 1;
- if (chip->type_reg_stride) - d->type_reg_stride = chip->type_reg_stride; - else - d->type_reg_stride = 1; - if (!map->use_single_read && map->reg_stride == 1 && d->irq_reg_stride == 1) { d->status_reg_buf = kmalloc_array(chip->num_regs, diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 879afdc81526..1966ad4d0fa5 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1483,8 +1483,6 @@ struct regmap_irq_sub_irq_map { * @irqs: Descriptors for individual IRQs. Interrupt numbers are * assigned based on the index in the array of the interrupt. * @num_irqs: Number of descriptors. - * @type_reg_stride: Stride to use for chips where type registers are not - * contiguous. * @num_config_bases: Number of config base registers. * @num_config_regs: Number of config registers for each config base register. * @handle_pre_irq: Driver specific callback to handle interrupt from device @@ -1535,7 +1533,6 @@ struct regmap_irq_chip {
int num_config_bases; int num_config_regs; - unsigned int type_reg_stride;
int (*handle_pre_irq)(void *irq_drv_data); int (*handle_post_irq)(void *irq_drv_data);
No chip has ever required this flag except for the max77650 where it didn't have any effect. Drop it. The code that checked for it has already been removed.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- include/linux/regmap.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 1966ad4d0fa5..ee2567a0465c 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1466,7 +1466,6 @@ struct regmap_irq_sub_irq_map { * @ack_invert: Inverted ack register: cleared bits for ack. * @clear_ack: Use this to set 1 and 0 or vice-versa to clear interrupts. * @wake_invert: Inverted wake register: cleared bits are wake enabled. - * @type_invert: Invert the type flags. * @type_in_mask: Use the mask registers for controlling irq type. For * interrupts defining type_rising/falling_mask use mask_base * for edge configuration and never update bits in type_base. @@ -1520,7 +1519,6 @@ struct regmap_irq_chip { bool clear_ack:1; bool wake_invert:1; bool runtime_pm:1; - bool type_invert:1; bool type_in_mask:1; bool clear_on_unmask:1; bool not_fixed_stride:1;
regmap_irq_update_bits() is misnamed and should only be used for updating mask registers, since it checks the mask_writeonly flag. As there are no users of mask_writeonly, it is safe to replace the wake register updates with regmap_update_bits().
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index b24818ad36e6..dd22d13c54c8 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -157,11 +157,11 @@ static void regmap_irq_sync_unlock(struct irq_data *data) reg = sub_irq_reg(d, d->chip->wake_base, i); if (d->wake_buf) { if (d->chip->wake_invert) - ret = regmap_irq_update_bits(d, reg, + ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], ~d->wake_buf[i]); else - ret = regmap_irq_update_bits(d, reg, + ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], d->wake_buf[i]); if (ret != 0) @@ -823,11 +823,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, reg = sub_irq_reg(d, d->chip->wake_base, i);
if (chip->wake_invert) - ret = regmap_irq_update_bits(d, reg, + ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], 0); else - ret = regmap_irq_update_bits(d, reg, + ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i], d->wake_buf[i]); if (ret != 0) {
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.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index dd22d13c54c8..4c0d7f7aa544 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -84,7 +84,7 @@ static int regmap_irq_update_bits(struct regmap_irq_chip_data *d, unsigned int val) { 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:
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);
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);
This function should only be used for updating mask bits, since it checks the mask_writeonly flag. To avoid confusion, rename it to regmap_irq_update_mask_bits().
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 4c0d7f7aa544..875415fc3133 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -79,9 +79,9 @@ static void regmap_irq_lock(struct irq_data *data) mutex_lock(&d->lock); }
-static int regmap_irq_update_bits(struct regmap_irq_chip_data *d, - unsigned int reg, unsigned int mask, - unsigned int val) +static int regmap_irq_update_mask_bits(struct regmap_irq_chip_data *d, + unsigned int reg, unsigned int mask, + unsigned int val) { if (d->chip->mask_writeonly) return regmap_write(d->map, reg, val & mask); @@ -129,11 +129,11 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
reg = sub_irq_reg(d, d->chip->mask_base, i); if (d->chip->mask_invert) { - ret = regmap_irq_update_bits(d, reg, + ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); } else if (d->chip->unmask_base) { /* set mask with mask_base register */ - ret = regmap_irq_update_bits(d, reg, + ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret < 0) dev_err(d->map->dev, @@ -142,12 +142,12 @@ static void regmap_irq_sync_unlock(struct irq_data *data) unmask_offset = d->chip->unmask_base - d->chip->mask_base; /* clear mask with unmask_base register */ - ret = regmap_irq_update_bits(d, + ret = regmap_irq_update_mask_bits(d, reg + unmask_offset, d->mask_buf_def[i], d->mask_buf[i]); } else { - ret = regmap_irq_update_bits(d, reg, + ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], d->mask_buf[i]); } if (ret != 0) @@ -761,17 +761,17 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, reg = sub_irq_reg(d, d->chip->mask_base, i);
if (chip->mask_invert) - ret = regmap_irq_update_bits(d, reg, + ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf[i], ~d->mask_buf[i]); else if (d->chip->unmask_base) { unmask_offset = d->chip->unmask_base - d->chip->mask_base; - ret = regmap_irq_update_bits(d, + ret = regmap_irq_update_mask_bits(d, reg + unmask_offset, d->mask_buf[i], d->mask_buf[i]); } else - ret = regmap_irq_update_bits(d, reg, + ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf[i], d->mask_buf[i]); if (ret != 0) { dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
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 will be inverted in both registers, contrary to the usual behavior of mask registers.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- include/linux/regmap.h | 1 + 1 file changed, 1 insertion(+)
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;
int num_regs;
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.
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.
The qcom-pm8008 appears to use "1 to enable" convention for enabling interrupts, 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.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/qcom-pm8008.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c index da16566f7883..18095e72714e 100644 --- a/drivers/mfd/qcom-pm8008.c +++ b/drivers/mfd/qcom-pm8008.c @@ -141,6 +141,7 @@ static struct regmap_irq_chip pm8008_irq_chip = { .status_base = PM8008_STATUS_BASE, .mask_base = PM8008_MASK_BASE, .unmask_base = PM8008_UNMASK_BASE, + .broken_mask_unmask = true, .ack_base = PM8008_ACK_BASE, .config_base = pm8008_config_regs, .num_config_bases = ARRAY_SIZE(pm8008_config_regs),
On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald aidanmacdonald.0x0@gmail.com wrote:
The qcom-pm8008 appears to use "1 to enable" convention for enabling interrupts, with separate set and clear registers. It's relying on masks and unmasks being inverted from their
It relies
intuitive meaning, so it needs the broken_mask_unmask flag.
How has it worked until now?
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.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/stpmic1.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c index eb3da558c3fb..2307d1b0269d 100644 --- a/drivers/mfd/stpmic1.c +++ b/drivers/mfd/stpmic1.c @@ -110,6 +110,7 @@ static const struct regmap_irq_chip stpmic1_regmap_irq_chip = { .status_base = INT_PENDING_R1, .mask_base = INT_CLEAR_MASK_R1, .unmask_base = INT_SET_MASK_R1, + .broken_mask_unmask = true, .ack_base = INT_CLEAR_R1, .num_regs = STPMIC1_PMIC_NUM_IRQ_REGS, .irqs = stpmic1_irqs,
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.
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.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 96 +++++++++++++++++--------------- include/linux/regmap.h | 7 ++- 2 files changed, 55 insertions(+), 48 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 875415fc3133..082a2981120c 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -30,6 +30,9 @@ struct regmap_irq_chip_data { int irq; int wake_count;
+ unsigned int mask_base; + unsigned int unmask_base; + void *status_reg_buf; unsigned int *main_status_buf; unsigned int *status_buf; @@ -95,7 +98,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data) struct regmap *map = d->map; int i, j, ret; u32 reg; - u32 unmask_offset; u32 val;
if (d->chip->runtime_pm) { @@ -124,35 +126,23 @@ static void regmap_irq_sync_unlock(struct irq_data *data) * suppress pointless writes. */ for (i = 0; i < d->chip->num_regs; i++) { - if (!d->chip->mask_base) - continue; - - reg = sub_irq_reg(d, d->chip->mask_base, i); - if (d->chip->mask_invert) { + if (d->mask_base) { + reg = sub_irq_reg(d, d->mask_base, i); ret = regmap_irq_update_mask_bits(d, reg, - d->mask_buf_def[i], ~d->mask_buf[i]); - } else if (d->chip->unmask_base) { - /* set mask with mask_base register */ + d->mask_buf_def[i], d->mask_buf[i]); + if (ret != 0) + dev_err(d->map->dev, "Failed to sync masks in %x\n", + reg); + } + + if (d->unmask_base) { + reg = sub_irq_reg(d, d->unmask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); - if (ret < 0) - dev_err(d->map->dev, - "Failed to sync unmasks in %x\n", + if (ret != 0) + dev_err(d->map->dev, "Failed to sync masks in %x\n", reg); - unmask_offset = d->chip->unmask_base - - d->chip->mask_base; - /* clear mask with unmask_base register */ - ret = regmap_irq_update_mask_bits(d, - reg + unmask_offset, - d->mask_buf_def[i], - d->mask_buf[i]); - } else { - ret = regmap_irq_update_mask_bits(d, reg, - d->mask_buf_def[i], d->mask_buf[i]); } - if (ret != 0) - dev_err(d->map->dev, "Failed to sync masks in %x\n", - reg);
reg = sub_irq_reg(d, d->chip->wake_base, i); if (d->wake_buf) { @@ -634,7 +624,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, int i; int ret = -ENOMEM; u32 reg; - u32 unmask_offset;
if (chip->num_regs <= 0) return -EINVAL; @@ -732,6 +721,24 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, d->chip = chip; d->irq_base = irq_base;
+ /* + * Swap role of mask_base and unmask_base if mask bits are inverted. + * + * 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. + */ + if (chip->mask_invert || chip->broken_mask_unmask) { + d->mask_base = chip->unmask_base; + d->unmask_base = chip->mask_base; + } else { + d->mask_base = chip->mask_base; + d->unmask_base = chip->unmask_base; + } + if (chip->irq_reg_stride) d->irq_reg_stride = chip->irq_reg_stride; else @@ -755,28 +762,27 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, /* Mask all the interrupts by default */ for (i = 0; i < chip->num_regs; i++) { d->mask_buf[i] = d->mask_buf_def[i]; - if (!chip->mask_base) - continue;
- reg = sub_irq_reg(d, d->chip->mask_base, i); - - if (chip->mask_invert) + if (d->mask_base) { + reg = sub_irq_reg(d, d->mask_base, i); ret = regmap_irq_update_mask_bits(d, reg, - d->mask_buf[i], ~d->mask_buf[i]); - else if (d->chip->unmask_base) { - unmask_offset = d->chip->unmask_base - - d->chip->mask_base; - ret = regmap_irq_update_mask_bits(d, - reg + unmask_offset, - d->mask_buf[i], - d->mask_buf[i]); - } else + d->mask_buf_def[i], d->mask_buf[i]); + if (ret != 0) { + dev_err(map->dev, "Failed to set masks in 0x%x: %d\n", + reg, ret); + goto err_alloc; + } + } + + if (d->unmask_base) { + reg = sub_irq_reg(d, d->unmask_base, i); ret = regmap_irq_update_mask_bits(d, reg, - d->mask_buf[i], d->mask_buf[i]); - if (ret != 0) { - dev_err(map->dev, "Failed to set masks in 0x%x: %d\n", - reg, ret); - goto err_alloc; + d->mask_buf_def[i], ~d->mask_buf[i]); + if (ret != 0) { + dev_err(map->dev, "Failed to set masks in 0x%x: %d\n", + reg, ret); + goto err_alloc; + } }
if (!chip->init_ack_masked) diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 21a70fd99493..0cf3c4a66946 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1451,10 +1451,11 @@ struct regmap_irq_sub_irq_map { * main_status set. * * @status_base: Base status register address. - * @mask_base: Base mask register address. + * @mask_base: Base mask register address. Mask bits are set to 1 when an + * interrupt is masked, 0 when unmasked. * @mask_writeonly: Base mask register is write only. - * @unmask_base: Base unmask register address. for chips who have - * separate mask and unmask registers + * @unmask_base: Base unmask register address. Unmask bits are set to 1 when + * an interrupt is unmasked and 0 when masked. * @ack_base: Base ack address. If zero then the chip is clear on read. * Using zero value is possible with @use_ack bit. * @wake_base: Base address for wake enables. If zero unsupported.
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);
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/tps65090.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c index bd6235308c6b..e474e1ca253a 100644 --- a/drivers/mfd/tps65090.c +++ b/drivers/mfd/tps65090.c @@ -127,8 +127,7 @@ static struct regmap_irq_chip tps65090_irq_chip = { .num_irqs = ARRAY_SIZE(tps65090_irqs), .num_regs = NUM_INT_REG, .status_base = TPS65090_REG_INTR_STS, - .mask_base = TPS65090_REG_INTR_MASK, - .mask_invert = true, + .unmask_base = TPS65090_REG_INTR_MASK, };
static bool is_volatile_reg(struct device *dev, unsigned int reg)
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/sun4i-gpadc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/sun4i-gpadc.c b/drivers/mfd/sun4i-gpadc.c index cfe14d9bf6dc..edc180d83a4b 100644 --- a/drivers/mfd/sun4i-gpadc.c +++ b/drivers/mfd/sun4i-gpadc.c @@ -34,9 +34,8 @@ static const struct regmap_irq_chip sun4i_gpadc_regmap_irq_chip = { .name = "sun4i_gpadc_irq_chip", .status_base = SUN4I_GPADC_INT_FIFOS, .ack_base = SUN4I_GPADC_INT_FIFOS, - .mask_base = SUN4I_GPADC_INT_FIFOC, + .unmask_base = SUN4I_GPADC_INT_FIFOC, .init_ack_masked = true, - .mask_invert = true, .irqs = sun4i_gpadc_regmap_irq, .num_irqs = ARRAY_SIZE(sun4i_gpadc_regmap_irq), .num_regs = 1,
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/sprd-sc27xx-spi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/sprd-sc27xx-spi.c b/drivers/mfd/sprd-sc27xx-spi.c index d05a47c5187f..a4a9b81a952b 100644 --- a/drivers/mfd/sprd-sc27xx-spi.c +++ b/drivers/mfd/sprd-sc27xx-spi.c @@ -181,11 +181,10 @@ static int sprd_pmic_probe(struct spi_device *spi) ddata->irq_chip.name = dev_name(&spi->dev); ddata->irq_chip.status_base = pdata->irq_base + SPRD_PMIC_INT_MASK_STATUS; - ddata->irq_chip.mask_base = pdata->irq_base + SPRD_PMIC_INT_EN; + ddata->irq_chip.unmask_base = pdata->irq_base + SPRD_PMIC_INT_EN; ddata->irq_chip.ack_base = 0; ddata->irq_chip.num_regs = 1; ddata->irq_chip.num_irqs = pdata->num_irqs; - ddata->irq_chip.mask_invert = true;
ddata->irqs = devm_kcalloc(&spi->dev, pdata->num_irqs, sizeof(struct regmap_irq),
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/rt5033.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c index f1236a9acf30..dc9bf4057a09 100644 --- a/drivers/mfd/rt5033.c +++ b/drivers/mfd/rt5033.c @@ -29,8 +29,7 @@ static const struct regmap_irq rt5033_irqs[] = { static const struct regmap_irq_chip rt5033_irq_chip = { .name = "rt5033", .status_base = RT5033_REG_PMIC_IRQ_STAT, - .mask_base = RT5033_REG_PMIC_IRQ_CTRL, - .mask_invert = true, + .unmask_base = RT5033_REG_PMIC_IRQ_CTRL, .num_regs = 1, .irqs = rt5033_irqs, .num_irqs = ARRAY_SIZE(rt5033_irqs),
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/rohm-bd71828.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c index 714d9fcbf07b..3c5c6c393650 100644 --- a/drivers/mfd/rohm-bd71828.c +++ b/drivers/mfd/rohm-bd71828.c @@ -413,9 +413,8 @@ static struct regmap_irq_chip bd71828_irq_chip = { .irqs = &bd71828_irqs[0], .num_irqs = ARRAY_SIZE(bd71828_irqs), .status_base = BD71828_REG_INT_BUCK, - .mask_base = BD71828_REG_INT_MASK_BUCK, + .unmask_base = BD71828_REG_INT_MASK_BUCK, .ack_base = BD71828_REG_INT_BUCK, - .mask_invert = true, .init_ack_masked = true, .num_regs = 12, .num_main_regs = 1, @@ -430,9 +429,8 @@ static struct regmap_irq_chip bd71815_irq_chip = { .irqs = &bd71815_irqs[0], .num_irqs = ARRAY_SIZE(bd71815_irqs), .status_base = BD71815_REG_INT_STAT_01, - .mask_base = BD71815_REG_INT_EN_01, + .unmask_base = BD71815_REG_INT_EN_01, .ack_base = BD71815_REG_INT_STAT_01, - .mask_invert = true, .init_ack_masked = true, .num_regs = 12, .num_main_regs = 1,
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/rn5t618.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/rn5t618.c b/drivers/mfd/rn5t618.c index 384acb459427..7ed002d090bd 100644 --- a/drivers/mfd/rn5t618.c +++ b/drivers/mfd/rn5t618.c @@ -80,8 +80,7 @@ static const struct regmap_irq_chip rc5t619_irq_chip = { .num_irqs = ARRAY_SIZE(rc5t619_irqs), .num_regs = 1, .status_base = RN5T618_INTMON, - .mask_base = RN5T618_INTEN, - .mask_invert = true, + .unmask_base = RN5T618_INTEN, };
static struct i2c_client *rn5t618_pm_power_off;
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/gateworks-gsc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/gateworks-gsc.c b/drivers/mfd/gateworks-gsc.c index d87876747b91..28ec167a9861 100644 --- a/drivers/mfd/gateworks-gsc.c +++ b/drivers/mfd/gateworks-gsc.c @@ -189,8 +189,7 @@ static const struct regmap_irq_chip gsc_irq_chip = { .num_irqs = ARRAY_SIZE(gsc_irqs), .num_regs = 1, .status_base = GSC_IRQ_STATUS, - .mask_base = GSC_IRQ_ENABLE, - .mask_invert = true, + .unmask_base = GSC_IRQ_ENABLE, .ack_base = GSC_IRQ_STATUS, .ack_invert = true, };
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/axp20x.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c index 8161a5dc68e8..3be0d0aa8b34 100644 --- a/drivers/mfd/axp20x.c +++ b/drivers/mfd/axp20x.c @@ -506,8 +506,7 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = { .name = "axp152_irq_chip", .status_base = AXP152_IRQ1_STATE, .ack_base = AXP152_IRQ1_STATE, - .mask_base = AXP152_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP152_IRQ1_EN, .init_ack_masked = true, .irqs = axp152_regmap_irqs, .num_irqs = ARRAY_SIZE(axp152_regmap_irqs), @@ -518,8 +517,7 @@ static const struct regmap_irq_chip axp20x_regmap_irq_chip = { .name = "axp20x_irq_chip", .status_base = AXP20X_IRQ1_STATE, .ack_base = AXP20X_IRQ1_STATE, - .mask_base = AXP20X_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP20X_IRQ1_EN, .init_ack_masked = true, .irqs = axp20x_regmap_irqs, .num_irqs = ARRAY_SIZE(axp20x_regmap_irqs), @@ -531,8 +529,7 @@ static const struct regmap_irq_chip axp22x_regmap_irq_chip = { .name = "axp22x_irq_chip", .status_base = AXP20X_IRQ1_STATE, .ack_base = AXP20X_IRQ1_STATE, - .mask_base = AXP20X_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP20X_IRQ1_EN, .init_ack_masked = true, .irqs = axp22x_regmap_irqs, .num_irqs = ARRAY_SIZE(axp22x_regmap_irqs), @@ -543,8 +540,7 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = { .name = "axp288_irq_chip", .status_base = AXP20X_IRQ1_STATE, .ack_base = AXP20X_IRQ1_STATE, - .mask_base = AXP20X_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP20X_IRQ1_EN, .init_ack_masked = true, .irqs = axp288_regmap_irqs, .num_irqs = ARRAY_SIZE(axp288_regmap_irqs), @@ -556,8 +552,7 @@ static const struct regmap_irq_chip axp803_regmap_irq_chip = { .name = "axp803", .status_base = AXP20X_IRQ1_STATE, .ack_base = AXP20X_IRQ1_STATE, - .mask_base = AXP20X_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP20X_IRQ1_EN, .init_ack_masked = true, .irqs = axp803_regmap_irqs, .num_irqs = ARRAY_SIZE(axp803_regmap_irqs), @@ -568,8 +563,7 @@ static const struct regmap_irq_chip axp806_regmap_irq_chip = { .name = "axp806", .status_base = AXP20X_IRQ1_STATE, .ack_base = AXP20X_IRQ1_STATE, - .mask_base = AXP20X_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP20X_IRQ1_EN, .init_ack_masked = true, .irqs = axp806_regmap_irqs, .num_irqs = ARRAY_SIZE(axp806_regmap_irqs), @@ -580,8 +574,7 @@ static const struct regmap_irq_chip axp809_regmap_irq_chip = { .name = "axp809", .status_base = AXP20X_IRQ1_STATE, .ack_base = AXP20X_IRQ1_STATE, - .mask_base = AXP20X_IRQ1_EN, - .mask_invert = true, + .unmask_base = AXP20X_IRQ1_EN, .init_ack_masked = true, .irqs = axp809_regmap_irqs, .num_irqs = ARRAY_SIZE(axp809_regmap_irqs),
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/atc260x-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/atc260x-core.c b/drivers/mfd/atc260x-core.c index 7148ff5b05b1..7c5de3ae776e 100644 --- a/drivers/mfd/atc260x-core.c +++ b/drivers/mfd/atc260x-core.c @@ -100,8 +100,7 @@ static const struct regmap_irq_chip atc2603c_regmap_irq_chip = { .num_irqs = ARRAY_SIZE(atc2603c_regmap_irqs), .num_regs = 1, .status_base = ATC2603C_INTS_PD, - .mask_base = ATC2603C_INTS_MSK, - .mask_invert = true, + .unmask_base = ATC2603C_INTS_MSK, };
static const struct regmap_irq_chip atc2609a_regmap_irq_chip = { @@ -110,8 +109,7 @@ static const struct regmap_irq_chip atc2609a_regmap_irq_chip = { .num_irqs = ARRAY_SIZE(atc2609a_regmap_irqs), .num_regs = 1, .status_base = ATC2609A_INTS_PD, - .mask_base = ATC2609A_INTS_MSK, - .mask_invert = true, + .unmask_base = ATC2609A_INTS_MSK, };
static const struct resource atc2603c_onkey_resources[] = {
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/88pm800.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c index eaf9845633b4..6d1192db13c1 100644 --- a/drivers/mfd/88pm800.c +++ b/drivers/mfd/88pm800.c @@ -398,9 +398,8 @@ static struct regmap_irq_chip pm800_irq_chip = {
.num_regs = 4, .status_base = PM800_INT_STATUS1, - .mask_base = PM800_INT_ENA_1, + .unmask_base = PM800_INT_ENA_1, .ack_base = PM800_INT_STATUS1, - .mask_invert = 1, };
static int pm800_pages_init(struct pm80x_chip *chip)
An inverted mask register can be described more directly as an unmask register.
Also drop useless mask_invert flag from the pmic irq chip.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/max14577.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/mfd/max14577.c b/drivers/mfd/max14577.c index 6c487fa14e9c..7a501dcc48f6 100644 --- a/drivers/mfd/max14577.c +++ b/drivers/mfd/max14577.c @@ -209,8 +209,7 @@ static const struct regmap_irq max14577_irqs[] = { static const struct regmap_irq_chip max14577_irq_chip = { .name = "max14577", .status_base = MAX14577_REG_INT1, - .mask_base = MAX14577_REG_INTMASK1, - .mask_invert = true, + .unmask_base = MAX14577_REG_INTMASK1, .num_regs = 3, .irqs = max14577_irqs, .num_irqs = ARRAY_SIZE(max14577_irqs), @@ -239,8 +238,7 @@ static const struct regmap_irq max77836_muic_irqs[] = { static const struct regmap_irq_chip max77836_muic_irq_chip = { .name = "max77836-muic", .status_base = MAX14577_REG_INT1, - .mask_base = MAX14577_REG_INTMASK1, - .mask_invert = true, + .unmask_base = MAX14577_REG_INTMASK1, .num_regs = 3, .irqs = max77836_muic_irqs, .num_irqs = ARRAY_SIZE(max77836_muic_irqs), @@ -255,7 +253,6 @@ static const struct regmap_irq_chip max77836_pmic_irq_chip = { .name = "max77836-pmic", .status_base = MAX77836_PMIC_REG_TOPSYS_INT, .mask_base = MAX77836_PMIC_REG_TOPSYS_INT_MASK, - .mask_invert = false, .num_regs = 1, .irqs = max77836_pmic_irqs, .num_irqs = ARRAY_SIZE(max77836_pmic_irqs),
An inverted mask register can be described more directly as an unmask register.
Also drop useless mask_invert flag from other irq chips.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/max77693.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c index 4e6244e17559..fadea37b97cc 100644 --- a/drivers/mfd/max77693.c +++ b/drivers/mfd/max77693.c @@ -66,7 +66,6 @@ static const struct regmap_irq_chip max77693_led_irq_chip = { .name = "max77693-led", .status_base = MAX77693_LED_REG_FLASH_INT, .mask_base = MAX77693_LED_REG_FLASH_INT_MASK, - .mask_invert = false, .num_regs = 1, .irqs = max77693_led_irqs, .num_irqs = ARRAY_SIZE(max77693_led_irqs), @@ -82,7 +81,6 @@ static const struct regmap_irq_chip max77693_topsys_irq_chip = { .name = "max77693-topsys", .status_base = MAX77693_PMIC_REG_TOPSYS_INT, .mask_base = MAX77693_PMIC_REG_TOPSYS_INT_MASK, - .mask_invert = false, .num_regs = 1, .irqs = max77693_topsys_irqs, .num_irqs = ARRAY_SIZE(max77693_topsys_irqs), @@ -100,7 +98,6 @@ static const struct regmap_irq_chip max77693_charger_irq_chip = { .name = "max77693-charger", .status_base = MAX77693_CHG_REG_CHG_INT, .mask_base = MAX77693_CHG_REG_CHG_INT_MASK, - .mask_invert = false, .num_regs = 1, .irqs = max77693_charger_irqs, .num_irqs = ARRAY_SIZE(max77693_charger_irqs), @@ -136,8 +133,7 @@ static const struct regmap_irq max77693_muic_irqs[] = { static const struct regmap_irq_chip max77693_muic_irq_chip = { .name = "max77693-muic", .status_base = MAX77693_MUIC_REG_INT1, - .mask_base = MAX77693_MUIC_REG_INTMASK1, - .mask_invert = true, + .unmask_base = MAX77693_MUIC_REG_INTMASK1, .num_regs = 3, .irqs = max77693_muic_irqs, .num_irqs = ARRAY_SIZE(max77693_muic_irqs),
There's no need to set the flag explicitly to false, since that is the default value from zero initialization.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/rohm-bd718x7.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c index bfd81f78beae..ad6c0971a997 100644 --- a/drivers/mfd/rohm-bd718x7.c +++ b/drivers/mfd/rohm-bd718x7.c @@ -70,7 +70,6 @@ static struct regmap_irq_chip bd718xx_irq_chip = { .mask_base = BD718XX_REG_MIRQ, .ack_base = BD718XX_REG_IRQ, .init_ack_masked = true, - .mask_invert = false, };
static const struct regmap_range pmic_status_range = {
On 6/20/22 23:06, Aidan MacDonald wrote:
There's no need to set the flag explicitly to false, since that is the default value from zero initialization.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com
drivers/mfd/rohm-bd718x7.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/mfd/rohm-bd718x7.c b/drivers/mfd/rohm-bd718x7.c index bfd81f78beae..ad6c0971a997 100644 --- a/drivers/mfd/rohm-bd718x7.c +++ b/drivers/mfd/rohm-bd718x7.c @@ -70,7 +70,6 @@ static struct regmap_irq_chip bd718xx_irq_chip = { .mask_base = BD718XX_REG_MIRQ, .ack_base = BD718XX_REG_IRQ, .init_ack_masked = true,
.mask_invert = false, };
static const struct regmap_range pmic_status_range = {
Reviewed-by: Matti Vaittinen mazziesaccount@gmail.com
There's no need to set the flag explicitly to false, since that is the default value from zero initialization.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/max77843.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/mfd/max77843.c b/drivers/mfd/max77843.c index 209ee24d9ce1..4da58eab1603 100644 --- a/drivers/mfd/max77843.c +++ b/drivers/mfd/max77843.c @@ -59,7 +59,6 @@ static const struct regmap_irq_chip max77843_irq_chip = { .name = "max77843", .status_base = MAX77843_SYS_REG_SYSINTSRC, .mask_base = MAX77843_SYS_REG_SYSINTMASK, - .mask_invert = false, .num_regs = 1, .irqs = max77843_irqs, .num_irqs = ARRAY_SIZE(max77843_irqs),
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/extcon/extcon-max77843.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/extcon/extcon-max77843.c b/drivers/extcon/extcon-max77843.c index 8e6e97ec65a8..1bc0426ce3f1 100644 --- a/drivers/extcon/extcon-max77843.c +++ b/drivers/extcon/extcon-max77843.c @@ -189,8 +189,7 @@ static const struct regmap_irq max77843_muic_irq[] = { static const struct regmap_irq_chip max77843_muic_irq_chip = { .name = "max77843-muic", .status_base = MAX77843_MUIC_REG_INT1, - .mask_base = MAX77843_MUIC_REG_INTMASK1, - .mask_invert = true, + .unmask_base = MAX77843_MUIC_REG_INTMASK1, .num_regs = 3, .irqs = max77843_muic_irq, .num_irqs = ARRAY_SIZE(max77843_muic_irq),
There's no need to set the flag explicitly to false, since that is the default value from zero initialization.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/extcon/extcon-sm5502.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c index f706f5288257..8401e8b27788 100644 --- a/drivers/extcon/extcon-sm5502.c +++ b/drivers/extcon/extcon-sm5502.c @@ -227,7 +227,6 @@ static const struct regmap_irq_chip sm5502_muic_irq_chip = { .name = "sm5502", .status_base = SM5502_REG_INT1, .mask_base = SM5502_REG_INTMASK1, - .mask_invert = false, .num_regs = 2, .irqs = sm5502_irqs, .num_irqs = ARRAY_SIZE(sm5502_irqs), @@ -276,7 +275,6 @@ static const struct regmap_irq_chip sm5504_muic_irq_chip = { .name = "sm5504", .status_base = SM5502_REG_INT1, .mask_base = SM5502_REG_INTMASK1, - .mask_invert = false, .num_regs = 2, .irqs = sm5504_irqs, .num_irqs = ARRAY_SIZE(sm5504_irqs),
There's no need to set the flag explicitly to false, since that is the default value from zero initialization.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/extcon/extcon-rt8973a.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/extcon/extcon-rt8973a.c b/drivers/extcon/extcon-rt8973a.c index 40c07f4d656e..02ba770acb27 100644 --- a/drivers/extcon/extcon-rt8973a.c +++ b/drivers/extcon/extcon-rt8973a.c @@ -192,7 +192,6 @@ static const struct regmap_irq_chip rt8973a_muic_irq_chip = { .name = "rt8973a", .status_base = RT8973A_REG_INT1, .mask_base = RT8973A_REG_INTM1, - .mask_invert = false, .num_regs = 2, .irqs = rt8973a_irqs, .num_irqs = ARRAY_SIZE(rt8973a_irqs),
An inverted mask register can be described more directly as an unmask register.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/irqchip/irq-sl28cpld.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-sl28cpld.c b/drivers/irqchip/irq-sl28cpld.c index fbb354413ffa..f2172240172c 100644 --- a/drivers/irqchip/irq-sl28cpld.c +++ b/drivers/irqchip/irq-sl28cpld.c @@ -65,8 +65,7 @@ static int sl28cpld_intc_probe(struct platform_device *pdev) irqchip->chip.num_irqs = ARRAY_SIZE(sl28cpld_irqs); irqchip->chip.num_regs = 1; irqchip->chip.status_base = base + INTC_IP; - irqchip->chip.mask_base = base + INTC_IE; - irqchip->chip.mask_invert = true; + irqchip->chip.unmask_base = base + INTC_IE; irqchip->chip.ack_base = base + INTC_IP;
return devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev),
An inverted mask register can be described more directly as an unmask register.
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),
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
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
Swap mask_base and unmask_base, and drop the broken_mask_unmask flag since we're now expecting the registers to have their usual behavior.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/stpmic1.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c index 2307d1b0269d..11f3d92acbc0 100644 --- a/drivers/mfd/stpmic1.c +++ b/drivers/mfd/stpmic1.c @@ -108,9 +108,8 @@ static const struct regmap_irq stpmic1_irqs[] = { static const struct regmap_irq_chip stpmic1_regmap_irq_chip = { .name = "pmic_irq", .status_base = INT_PENDING_R1, - .mask_base = INT_CLEAR_MASK_R1, - .unmask_base = INT_SET_MASK_R1, - .broken_mask_unmask = true, + .mask_base = INT_SET_MASK_R1, + .unmask_base = INT_CLEAR_MASK_R1, .ack_base = INT_CLEAR_R1, .num_regs = STPMIC1_PMIC_NUM_IRQ_REGS, .irqs = stpmic1_irqs,
The STPMIC1 has separate set and clear registers for controlling its interrupt masks. These are volatile registers; writing a '1' will set or clear the corresponding mask bit, and they read as 0.
Marking the registers volatile and using the mask_writeonly flag should reduce bus traffic by avoiding a read-modify-write on the mask set/clear registers.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/stpmic1.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c index 11f3d92acbc0..a99f7b45df57 100644 --- a/drivers/mfd/stpmic1.c +++ b/drivers/mfd/stpmic1.c @@ -42,6 +42,8 @@ static const struct regmap_range stpmic1_volatile_ranges[] = { regmap_reg_range(WCHDG_CR, WCHDG_CR), regmap_reg_range(INT_PENDING_R1, INT_PENDING_R4), regmap_reg_range(INT_SRC_R1, INT_SRC_R4), + regmap_reg_range(INT_SET_MASK_R1, INT_SET_MASK_R4), + regmap_reg_range(INT_CLEAR_MASK_R1, INT_CLEAR_MASK_R4), };
static const struct regmap_access_table stpmic1_readable_table = { @@ -110,6 +112,7 @@ static const struct regmap_irq_chip stpmic1_regmap_irq_chip = { .status_base = INT_PENDING_R1, .mask_base = INT_SET_MASK_R1, .unmask_base = INT_CLEAR_MASK_R1, + .mask_writeonly = true, .ack_base = INT_CLEAR_R1, .num_regs = STPMIC1_PMIC_NUM_IRQ_REGS, .irqs = stpmic1_irqs,
Swap mask_base and unmask_base, and drop the broken_mask_unmask flag since we're now expecting the registers to have their usual behavior.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/qcom-pm8008.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c index 18095e72714e..7bc6becfe7f4 100644 --- a/drivers/mfd/qcom-pm8008.c +++ b/drivers/mfd/qcom-pm8008.c @@ -45,8 +45,8 @@ enum { #define PM8008_GPIO2_ADDR PM8008_PERIPH_3_BASE
#define PM8008_STATUS_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_STS_OFFSET) -#define PM8008_MASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_SET_OFFSET) -#define PM8008_UNMASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET) +#define PM8008_MASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET) +#define PM8008_UNMASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_SET_OFFSET) #define PM8008_TYPE_BASE (PM8008_PERIPH_0_BASE | INT_SET_TYPE_OFFSET) #define PM8008_ACK_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_CLR_OFFSET) #define PM8008_POLARITY_HI_BASE (PM8008_PERIPH_0_BASE | INT_POL_HIGH_OFFSET) @@ -141,7 +141,6 @@ static struct regmap_irq_chip pm8008_irq_chip = { .status_base = PM8008_STATUS_BASE, .mask_base = PM8008_MASK_BASE, .unmask_base = PM8008_UNMASK_BASE, - .broken_mask_unmask = true, .ack_base = PM8008_ACK_BASE, .config_base = pm8008_config_regs, .num_config_bases = ARRAY_SIZE(pm8008_config_regs),
The PM8008 has separate set and clear registers for controlling its interrupt masks. These are likely volatile registers which read as 0, and writing a '1' bit sets or clears the corresponding bit in the mask register.
The PM8008's regmap config doesn't enable a cache, so all register access is already volatile. Adding the mask_writeonly flag should reduce bus traffic by avoiding a read-modify-write on the mask set/clear registers.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/qcom-pm8008.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c index 7bc6becfe7f4..c778f2f87a17 100644 --- a/drivers/mfd/qcom-pm8008.c +++ b/drivers/mfd/qcom-pm8008.c @@ -141,6 +141,7 @@ static struct regmap_irq_chip pm8008_irq_chip = { .status_base = PM8008_STATUS_BASE, .mask_base = PM8008_MASK_BASE, .unmask_base = PM8008_UNMASK_BASE, + .mask_writeonly = true, .ack_base = PM8008_ACK_BASE, .config_base = pm8008_config_regs, .num_config_bases = ARRAY_SIZE(pm8008_config_regs),
Drop broken_mask_unmask flag; no drivers are relying on it anymore.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 9 +-------- include/linux/regmap.h | 1 - 2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 082a2981120c..8a718615fd09 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -723,15 +723,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
/* * Swap role of mask_base and unmask_base if mask bits are inverted. - * - * 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. */ - if (chip->mask_invert || chip->broken_mask_unmask) { + if (chip->mask_invert) { d->mask_base = chip->unmask_base; d->unmask_base = chip->mask_base; } else { diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 0cf3c4a66946..a3103c88e936 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1524,7 +1524,6 @@ struct regmap_irq_chip { bool clear_on_unmask:1; bool not_fixed_stride:1; bool status_invert:1; - bool broken_mask_unmask:1;
int num_regs;
An inverted mask register can be represented more directly as an unmask register. Drop the redundant mask_invert flag.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 30 ++++++++---------------------- include/linux/regmap.h | 2 -- 2 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 8a718615fd09..0a8edaee064a 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -30,9 +30,6 @@ struct regmap_irq_chip_data { int irq; int wake_count;
- unsigned int mask_base; - unsigned int unmask_base; - void *status_reg_buf; unsigned int *main_status_buf; unsigned int *status_buf; @@ -126,8 +123,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data) * suppress pointless writes. */ for (i = 0; i < d->chip->num_regs; i++) { - if (d->mask_base) { - reg = sub_irq_reg(d, d->mask_base, i); + if (d->chip->mask_base) { + reg = sub_irq_reg(d, d->chip->mask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], d->mask_buf[i]); if (ret != 0) @@ -135,8 +132,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data) reg); }
- if (d->unmask_base) { - reg = sub_irq_reg(d, d->unmask_base, i); + if (d->chip->unmask_base) { + reg = sub_irq_reg(d, d->chip->unmask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret != 0) @@ -721,17 +718,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, d->chip = chip; d->irq_base = irq_base;
- /* - * Swap role of mask_base and unmask_base if mask bits are inverted. - */ - if (chip->mask_invert) { - d->mask_base = chip->unmask_base; - d->unmask_base = chip->mask_base; - } else { - d->mask_base = chip->mask_base; - d->unmask_base = chip->unmask_base; - } - if (chip->irq_reg_stride) d->irq_reg_stride = chip->irq_reg_stride; else @@ -756,8 +742,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, for (i = 0; i < chip->num_regs; i++) { d->mask_buf[i] = d->mask_buf_def[i];
- if (d->mask_base) { - reg = sub_irq_reg(d, d->mask_base, i); + if (d->chip->mask_base) { + reg = sub_irq_reg(d, d->chip->mask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], d->mask_buf[i]); if (ret != 0) { @@ -767,8 +753,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, } }
- if (d->unmask_base) { - reg = sub_irq_reg(d, d->unmask_base, i); + if (d->chip->unmask_base) { + reg = sub_irq_reg(d, d->chip->unmask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret != 0) { diff --git a/include/linux/regmap.h b/include/linux/regmap.h index a3103c88e936..bb625a1edef9 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1462,7 +1462,6 @@ struct regmap_irq_sub_irq_map { * @config_base: Base address for IRQ type config regs. If null unsupported. * @irq_reg_stride: Stride to use for chips where registers are not contiguous. * @init_ack_masked: Ack all masked interrupts once during initalization. - * @mask_invert: Inverted mask register: cleared bits are masked out. * @use_ack: Use @ack register even if it is zero. * @ack_invert: Inverted ack register: cleared bits for ack. * @clear_ack: Use this to set 1 and 0 or vice-versa to clear interrupts. @@ -1514,7 +1513,6 @@ struct regmap_irq_chip { unsigned int irq_reg_stride; bool mask_writeonly:1; bool init_ack_masked:1; - bool mask_invert:1; bool use_ack:1; bool ack_invert:1; bool clear_ack:1;
There are several conditions that must be satisfied to support bulk read of status registers. Move the check into a function to avoid duplicating it in two places.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 0a8edaee064a..7b5bd1d45fc0 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -72,6 +72,14 @@ struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data, return &data->chip->irqs[irq]; }
+static bool regmap_irq_can_bulk_read_status(struct regmap_irq_chip_data *data) +{ + struct regmap *map = data->map; + + return !map->use_single_read && map->reg_stride == 1 && + data->irq_reg_stride == 1; +} + static void regmap_irq_lock(struct irq_data *data) { struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data); @@ -413,8 +421,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) }
} - } else if (!map->use_single_read && map->reg_stride == 1 && - data->irq_reg_stride == 1) { + } else if (regmap_irq_can_bulk_read_status(data)) {
u8 *buf8 = data->status_reg_buf; u16 *buf16 = data->status_reg_buf; @@ -723,8 +730,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, else d->irq_reg_stride = 1;
- if (!map->use_single_read && map->reg_stride == 1 && - d->irq_reg_stride == 1) { + if (regmap_irq_can_bulk_read_status(d)) { d->status_reg_buf = kmalloc_array(chip->num_regs, map->format.val_bytes, GFP_KERNEL);
On Mon, Jun 20, 2022 at 10:11 PM Aidan MacDonald aidanmacdonald.0x0@gmail.com wrote:
There are several conditions that must be satisfied to support bulk read of status registers. Move the check into a function to avoid duplicating it in two places.
...
} else if (!map->use_single_read && map->reg_stride == 1 &&data->irq_reg_stride == 1) {
} else if (regmap_irq_can_bulk_read_status(data)) {
While at it, you may drop this unneeded blank line.
u8 *buf8 = data->status_reg_buf; u16 *buf16 = data->status_reg_buf;
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.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 122 ++++++++++++++++++++----------- include/linux/regmap.h | 15 +++- 2 files changed, 92 insertions(+), 45 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 7b5bd1d45fc0..acbd6e22b0cd 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -41,30 +41,12 @@ struct regmap_irq_chip_data {
unsigned int irq_reg_stride;
+ unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data, + unsigned int base, int index); + bool clear_status:1; };
-static int sub_irq_reg(struct regmap_irq_chip_data *data, - unsigned int base_reg, int i) -{ - const struct regmap_irq_chip *chip = data->chip; - struct regmap *map = data->map; - struct regmap_irq_sub_irq_map *subreg; - unsigned int offset; - int reg = 0; - - if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { - /* Assume linear mapping */ - reg = base_reg + (i * map->reg_stride * data->irq_reg_stride); - } else { - subreg = &chip->sub_reg_offsets[i]; - offset = subreg->offset[0]; - reg = base_reg + offset; - } - - return reg; -} - static inline const struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data, int irq) @@ -76,8 +58,14 @@ static bool regmap_irq_can_bulk_read_status(struct regmap_irq_chip_data *data) { struct regmap *map = data->map;
+ /* + * While possible that a user-defined get_irq_reg callback might be + * 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; }
static void regmap_irq_lock(struct irq_data *data) @@ -114,7 +102,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
if (d->clear_status) { for (i = 0; i < d->chip->num_regs; i++) { - reg = sub_irq_reg(d, d->chip->status_base, i); + reg = d->get_irq_reg(d, d->chip->status_base, i);
ret = regmap_read(map, reg, &val); if (ret) @@ -132,7 +120,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) */ for (i = 0; i < d->chip->num_regs; i++) { if (d->chip->mask_base) { - reg = sub_irq_reg(d, d->chip->mask_base, i); + reg = d->get_irq_reg(d, d->chip->mask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], d->mask_buf[i]); if (ret != 0) @@ -141,7 +129,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) }
if (d->chip->unmask_base) { - reg = sub_irq_reg(d, d->chip->unmask_base, i); + reg = d->get_irq_reg(d, d->chip->unmask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret != 0) @@ -149,7 +137,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) reg); }
- reg = sub_irq_reg(d, d->chip->wake_base, i); + reg = d->get_irq_reg(d, d->chip->wake_base, i); if (d->wake_buf) { if (d->chip->wake_invert) ret = regmap_update_bits(d->map, reg, @@ -173,7 +161,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data) * it'll be ignored in irq handler, then may introduce irq storm */ if (d->mask_buf[i] && (d->chip->ack_base || d->chip->use_ack)) { - reg = sub_irq_reg(d, d->chip->ack_base, i); + reg = d->get_irq_reg(d, d->chip->ack_base, i);
/* some chips ack by write 0 */ if (d->chip->ack_invert) @@ -194,7 +182,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
for (i = 0; i < d->chip->num_config_bases; i++) { for (j = 0; j < d->chip->num_config_regs; j++) { - reg = sub_irq_reg(d, d->chip->config_base[i], j); + reg = d->get_irq_reg(d, d->chip->config_base[i], j); ret = regmap_write(map, reg, d->config_buf[i][j]); if (ret != 0) dev_err(d->map->dev, @@ -316,14 +304,17 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, const struct regmap_irq_chip *chip = data->chip; struct regmap *map = data->map; struct regmap_irq_sub_irq_map *subreg; + unsigned int reg; int i, ret = 0;
if (!chip->sub_reg_offsets) { - /* Assume linear mapping */ - ret = regmap_read(map, chip->status_base + - (b * map->reg_stride * data->irq_reg_stride), - &data->status_buf[b]); + reg = data->get_irq_reg(data, chip->status_base, b); + ret = regmap_read(map, reg, &data->status_buf[b]); } else { + /* + * Note we can't use get_irq_reg() here because the offsets + * in 'subreg' are *not* interchangeable with indices. + */ subreg = &chip->sub_reg_offsets[b]; for (i = 0; i < subreg->num_regs; i++) { unsigned int offset = subreg->offset[i]; @@ -389,10 +380,19 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) * sake of simplicity. and add bulk reads only if needed */ for (i = 0; i < chip->num_main_regs; i++) { - ret = regmap_read(map, chip->main_status + - (i * map->reg_stride - * data->irq_reg_stride), - &data->main_status_buf[i]); + /* + * For not_fixed_stride, don't use get_irq_reg(). + * It would produce an incorrect result. + */ + if (data->chip->not_fixed_stride) + reg = chip->main_status + + (i * map->reg_stride * + data->irq_reg_stride); + else + reg = data->get_irq_reg(data, + chip->main_status, i); + + ret = regmap_read(map, reg, &data->main_status_buf[i]); if (ret) { dev_err(map->dev, "Failed to read IRQ status %d\n", @@ -457,7 +457,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
} else { for (i = 0; i < data->chip->num_regs; i++) { - unsigned int reg = sub_irq_reg(data, + unsigned int reg = data->get_irq_reg(data, data->chip->status_base, i); ret = regmap_read(map, reg, &data->status_buf[i]);
@@ -485,7 +485,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) data->status_buf[i] &= ~data->mask_buf[i];
if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) { - reg = sub_irq_reg(data, data->chip->ack_base, i); + reg = data->get_irq_reg(data, data->chip->ack_base, i);
if (chip->ack_invert) ret = regmap_write(map, reg, @@ -545,6 +545,37 @@ static const struct irq_domain_ops regmap_domain_ops = { .xlate = irq_domain_xlate_onetwocell, };
+/** + * regmap_irq_get_irq_reg_linear() - Linear IRQ register mapping callback. + * + * @data: Data for the &struct regmap_irq_chip + * @base: Base register + * @index: Register index + * + * Returns the register address corresponding to the given @base and @index + * by the formula ``base + index * regmap_stride * irq_reg_stride``. + */ +unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data, + unsigned int base, int index) +{ + const struct regmap_irq_chip *chip = data->chip; + struct regmap *map = data->map; + + /* + * NOTE: This is for backward compatibility only and will be removed + * when not_fixed_stride is dropped (it's only used by qcom-pm8008). + */ + if (chip->not_fixed_stride && chip->sub_reg_offsets) { + struct regmap_irq_sub_irq_map *subreg; + + subreg = &chip->sub_reg_offsets[0]; + return base + subreg->offset[0]; + } + + return base + index * (map->reg_stride * chip->irq_reg_stride); +} +EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear); + /** * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback. * @@ -730,6 +761,11 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, else d->irq_reg_stride = 1;
+ if (chip->get_irq_reg) + d->get_irq_reg = chip->get_irq_reg; + else + d->get_irq_reg = regmap_irq_get_irq_reg_linear; + if (regmap_irq_can_bulk_read_status(d)) { d->status_reg_buf = kmalloc_array(chip->num_regs, map->format.val_bytes, @@ -749,7 +785,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, d->mask_buf[i] = d->mask_buf_def[i];
if (d->chip->mask_base) { - reg = sub_irq_reg(d, d->chip->mask_base, i); + reg = d->get_irq_reg(d, d->chip->mask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], d->mask_buf[i]); if (ret != 0) { @@ -760,7 +796,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, }
if (d->chip->unmask_base) { - reg = sub_irq_reg(d, d->chip->unmask_base, i); + reg = d->get_irq_reg(d, d->chip->unmask_base, i); ret = regmap_irq_update_mask_bits(d, reg, d->mask_buf_def[i], ~d->mask_buf[i]); if (ret != 0) { @@ -774,7 +810,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, continue;
/* Ack masked but set interrupts */ - reg = sub_irq_reg(d, d->chip->status_base, i); + reg = d->get_irq_reg(d, d->chip->status_base, i); ret = regmap_read(map, reg, &d->status_buf[i]); if (ret != 0) { dev_err(map->dev, "Failed to read IRQ status: %d\n", @@ -786,7 +822,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, d->status_buf[i] = ~d->status_buf[i];
if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) { - reg = sub_irq_reg(d, d->chip->ack_base, i); + reg = d->get_irq_reg(d, d->chip->ack_base, i); if (chip->ack_invert) ret = regmap_write(map, reg, ~(d->status_buf[i] & d->mask_buf[i])); @@ -811,7 +847,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, if (d->wake_buf) { for (i = 0; i < chip->num_regs; i++) { d->wake_buf[i] = d->mask_buf_def[i]; - reg = sub_irq_reg(d, d->chip->wake_base, i); + reg = d->get_irq_reg(d, d->chip->wake_base, i);
if (chip->wake_invert) ret = regmap_update_bits(d->map, reg, diff --git a/include/linux/regmap.h b/include/linux/regmap.h index bb625a1edef9..be51af0a2425 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1424,6 +1424,8 @@ struct regmap_irq_sub_irq_map { unsigned int *offset; };
+struct regmap_irq_chip_data; + /** * struct regmap_irq_chip - Description of a generic regmap irq_chip. * @@ -1489,6 +1491,13 @@ struct regmap_irq_sub_irq_map { * @handle_post_irq: Driver specific callback to handle interrupt from device * after handling the interrupts in regmap_irq_handler(). * @set_type_config: Callback used for configuring irq types. + * @get_irq_reg: Callback for mapping (base register, index) pairs to register + * addresses. The base register will be one of @status_base, + * @mask_base, etc., @main_status, or any of @config_base. + * The index will be in the range [0, num_main_regs[ for the + * main status base, [0, num_type_settings[ for any config + * register base, and [0, num_regs[ for any other base. + * If unspecified then regmap_irq_get_irq_reg_linear() is used. * @irq_drv_data: Driver specific IRQ data which is passed as parameter when * driver specific pre/post interrupt handler is called. * @@ -1535,11 +1544,13 @@ struct regmap_irq_chip { int (*handle_post_irq)(void *irq_drv_data); int (*set_type_config)(unsigned int **buf, unsigned int type, const struct regmap_irq *irq_data, int idx); + unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data, + unsigned int base, int index); void *irq_drv_data; };
-struct regmap_irq_chip_data; - +unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data, + unsigned int base, int index); int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type, const struct regmap_irq *irq_data, int idx);
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).*/
Replace the not_fixed_stride flag with a get_irq_reg() callback, which expresses what we want to do here more directly instead of relying on a convoluted hierarchy of offsets.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/mfd/qcom-pm8008.c | 56 +++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 31 deletions(-)
diff --git a/drivers/mfd/qcom-pm8008.c b/drivers/mfd/qcom-pm8008.c index c778f2f87a17..f6407aa0bcfc 100644 --- a/drivers/mfd/qcom-pm8008.c +++ b/drivers/mfd/qcom-pm8008.c @@ -44,16 +44,6 @@ enum { #define PM8008_GPIO1_ADDR PM8008_PERIPH_2_BASE #define PM8008_GPIO2_ADDR PM8008_PERIPH_3_BASE
-#define PM8008_STATUS_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_STS_OFFSET) -#define PM8008_MASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET) -#define PM8008_UNMASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_SET_OFFSET) -#define PM8008_TYPE_BASE (PM8008_PERIPH_0_BASE | INT_SET_TYPE_OFFSET) -#define PM8008_ACK_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_CLR_OFFSET) -#define PM8008_POLARITY_HI_BASE (PM8008_PERIPH_0_BASE | INT_POL_HIGH_OFFSET) -#define PM8008_POLARITY_LO_BASE (PM8008_PERIPH_0_BASE | INT_POL_LOW_OFFSET) - -#define PM8008_PERIPH_OFFSET(paddr) (paddr - PM8008_PERIPH_0_BASE) - struct pm8008_data { struct device *dev; struct regmap *regmap; @@ -61,22 +51,10 @@ struct pm8008_data { struct regmap_irq_chip_data *irq_data; };
-static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_0_BASE)}; -static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_1_BASE)}; -static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_2_BASE)}; -static unsigned int p3_offs[] = {PM8008_PERIPH_OFFSET(PM8008_PERIPH_3_BASE)}; - -static struct regmap_irq_sub_irq_map pm8008_sub_reg_offsets[] = { - REGMAP_IRQ_MAIN_REG_OFFSET(p0_offs), - REGMAP_IRQ_MAIN_REG_OFFSET(p1_offs), - REGMAP_IRQ_MAIN_REG_OFFSET(p2_offs), - REGMAP_IRQ_MAIN_REG_OFFSET(p3_offs), -}; - static unsigned int pm8008_config_regs[] = { - PM8008_TYPE_BASE, - PM8008_POLARITY_HI_BASE, - PM8008_POLARITY_LO_BASE, + INT_SET_TYPE_OFFSET, + INT_POL_HIGH_OFFSET, + INT_POL_LOW_OFFSET, };
enum { @@ -96,6 +74,23 @@ static struct regmap_irq pm8008_irqs[] = { REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)), };
+static const unsigned int pm8008_periph_base[] = { + PM8008_PERIPH_0_BASE, + PM8008_PERIPH_1_BASE, + PM8008_PERIPH_2_BASE, + PM8008_PERIPH_3_BASE, +}; + +static unsigned int pm8008_get_irq_reg(struct regmap_irq_chip_data *data, + unsigned int base, int index) +{ + /* Simple linear addressing for the main status register */ + if (base == I2C_INTR_STATUS_BASE) + return base + index; + + return pm8008_periph_base[index] + base; +} + static int pm8008_set_type_config(unsigned int **buf, unsigned int type, const struct regmap_irq *irq_data, int idx) { @@ -136,17 +131,16 @@ static struct regmap_irq_chip pm8008_irq_chip = { .irqs = pm8008_irqs, .num_irqs = ARRAY_SIZE(pm8008_irqs), .num_regs = PM8008_NUM_PERIPHS, - .not_fixed_stride = true, - .sub_reg_offsets = pm8008_sub_reg_offsets, - .status_base = PM8008_STATUS_BASE, - .mask_base = PM8008_MASK_BASE, - .unmask_base = PM8008_UNMASK_BASE, + .status_base = INT_LATCHED_STS_OFFSET, + .mask_base = INT_EN_CLR_OFFSET, + .unmask_base = INT_EN_SET_OFFSET, .mask_writeonly = true, - .ack_base = PM8008_ACK_BASE, + .ack_base = INT_LATCHED_CLR_OFFSET, .config_base = pm8008_config_regs, .num_config_bases = ARRAY_SIZE(pm8008_config_regs), .num_config_regs = PM8008_NUM_PERIPHS, .set_type_config = pm8008_set_type_config, + .get_irq_reg = pm8008_get_irq_reg, };
static struct regmap_config qcom_mfd_regmap_cfg = {
Clean up all the cruft related to not_fixed_stride. The same thing can be accomplished with a custom get_irq_reg() callback.
Signed-off-by: Aidan MacDonald aidanmacdonald.0x0@gmail.com --- drivers/base/regmap/regmap-irq.c | 41 +++----------------------------- include/linux/regmap.h | 7 ------ 2 files changed, 3 insertions(+), 45 deletions(-)
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index acbd6e22b0cd..0c9dd218614a 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -320,15 +320,8 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data, unsigned int offset = subreg->offset[i]; unsigned int index = offset / map->reg_stride;
- if (chip->not_fixed_stride) - ret = regmap_read(map, - chip->status_base + offset, - &data->status_buf[b]); - else - ret = regmap_read(map, - chip->status_base + offset, - &data->status_buf[index]); - + ret = regmap_read(map, chip->status_base + offset, + &data->status_buf[index]); if (ret) break; } @@ -380,18 +373,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d) * sake of simplicity. and add bulk reads only if needed */ for (i = 0; i < chip->num_main_regs; i++) { - /* - * For not_fixed_stride, don't use get_irq_reg(). - * It would produce an incorrect result. - */ - if (data->chip->not_fixed_stride) - reg = chip->main_status + - (i * map->reg_stride * - data->irq_reg_stride); - else - reg = data->get_irq_reg(data, - chip->main_status, i); - + reg = data->get_irq_reg(data, chip->main_status, i); ret = regmap_read(map, reg, &data->main_status_buf[i]); if (ret) { dev_err(map->dev, @@ -561,17 +543,6 @@ unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data, const struct regmap_irq_chip *chip = data->chip; struct regmap *map = data->map;
- /* - * NOTE: This is for backward compatibility only and will be removed - * when not_fixed_stride is dropped (it's only used by qcom-pm8008). - */ - if (chip->not_fixed_stride && chip->sub_reg_offsets) { - struct regmap_irq_sub_irq_map *subreg; - - subreg = &chip->sub_reg_offsets[0]; - return base + subreg->offset[0]; - } - return base + index * (map->reg_stride * chip->irq_reg_stride); } EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear); @@ -674,12 +645,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, return -EINVAL; }
- if (chip->not_fixed_stride) { - for (i = 0; i < chip->num_regs; i++) - if (chip->sub_reg_offsets[i].num_regs != 1) - return -EINVAL; - } - if (irq_base) { irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0); if (irq_base < 0) { diff --git a/include/linux/regmap.h b/include/linux/regmap.h index be51af0a2425..ecd3682de269 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1446,9 +1446,6 @@ struct regmap_irq_chip_data; * status_base. Should contain num_regs arrays. * Can be provided for chips with more complex mapping than * 1.st bit to 1.st sub-reg, 2.nd bit to 2.nd sub-reg, ... - * When used with not_fixed_stride, each one-element array - * member contains offset calculated as address from each - * peripheral to first peripheral. * @num_main_regs: Number of 'main status' irq registers for chips which have * main_status set. * @@ -1474,9 +1471,6 @@ struct regmap_irq_chip_data; * @clear_on_unmask: For chips with interrupts cleared on read: read the status * registers before unmasking interrupts to clear any bits * set when they were masked. - * @not_fixed_stride: Used when chip peripherals are not laid out with fixed - * stride. Must be used with sub_reg_offsets containing the - * offsets to each peripheral. * @status_invert: Inverted status register: cleared bits are active interrupts. * @runtime_pm: Hold a runtime PM lock on the device when accessing it. * @@ -1529,7 +1523,6 @@ struct regmap_irq_chip { bool runtime_pm:1; bool type_in_mask:1; bool clear_on_unmask:1; - bool not_fixed_stride:1; bool status_invert:1;
int num_regs;
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.
Cool series, thanks for cleaning this up!
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.
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
participants (6)
-
Aidan MacDonald -
Andy Shevchenko -
Mark Brown -
Matti Vaittinen -
Michael Walle -
Vaittinen, Matti