| Message ID | 20220607155324.118102-3-aidanmacdonald.0x0@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for AXP192 PMIC | expand |
On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote: > - if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { > + if (chip->get_irq_reg) { > + reg = chip->get_irq_reg(base_reg, i); > + } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { It seems like it would be cleaner and clearer to refactor things so that we always have a get_irq_reg() with standard chips getting given a default implementation which implements the current behaviour.
Mark Brown <broonie@kernel.org> writes: > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote: > >> - if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { >> + if (chip->get_irq_reg) { >> + reg = chip->get_irq_reg(base_reg, i); >> + } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { > > It seems like it would be cleaner and clearer to refactor things so that > we always have a get_irq_reg() with standard chips getting given a > default implementation which implements the current behaviour. I don't think that is a good way to clean things up. I only intended get_irq_reg() to be a quick hack to solve a problem; in my opinion it would be a poor abstraction to base the API around. What I'd suggest is something that will simplify regmap-irq. Instead of defining the base registers, etc. in the chip, introduce a new struct to describe a register group: struct regmap_irq_reg_group { unsigned int status_base; unsigned int mask_base; ... unsigned int irq_reg_stride; int num_regs; }; The idea is that the registers in a group are linearly mapped using the formula "base + (i * irq_reg_stride)". Then it's possible to allow for multiple register groups in regmap_irq_chip: struct regmap_irq_chip { const struct regmap_irq_reg_group *groups; unsigned int num_groups; unsigned int main_status_base; unsigned int num_main_status_bits; int num_main_regs; ... }; It should be straightforward to fit existing chips into this model. - "Normal" chips which do not use sub_reg_offsets or not_fixed_stride will have a single register group describing their register layout. - Chips which use not_fixed_stride=1 (eg. qcom-pm8008.c) will define multiple register groups instead of using sub_reg_offsets, so they will look more like a normal chip. - Chips that use a main status + sub-block IRQ layout will define one register group for each sub-block and continue to describe the location of the main status registers inside of regmap_irq_chip. A group will only get polled if the corresponding main status bit is set -- n'th group is polled if n'th bit is set. I think this scheme is easier to understand than having three or four different hacks to deal with minor deviations from the simple cases. It's also more flexible because groups do not need to be homogenous, unlike the way sub_reg_offsets works. For the AXP192, I'd just need to add two register groups, one for IRQ0-3 and another with IRQ4 off by itself. So this would remove the need for get_irq_reg() entirely. On the other hand, basing the public API around get_irq_reg() doesn't appear to simplify any existing use case. What do you think? If you're happy with the idea I don't mind doing the refactoring in a separate patch series. I had hoped to avoid a big refactor just to add one chip, though. Best regards, Aidan
Hi dee Ho peeps! Sorry for the late reply. pe 10. kesäk. 2022 klo 18.43 Aidan MacDonald (aidanmacdonald.0x0@gmail.com) kirjoitti: > > Mark Brown <broonie@kernel.org> writes: > > > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote: > > > >> - if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { > >> + if (chip->get_irq_reg) { > >> + reg = chip->get_irq_reg(base_reg, i); > >> + } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { > > > > It seems like it would be cleaner and clearer to refactor things so that > > we always have a get_irq_reg() with standard chips getting given a > > default implementation which implements the current behaviour. > > I don't think that is a good way to clean things up. I only intended > get_irq_reg() to be a quick hack to solve a problem; in my opinion it > would be a poor abstraction to base the API around. > > What I'd suggest is something that will simplify regmap-irq. Instead of > defining the base registers, etc. in the chip, introduce a new struct > to describe a register group: > > struct regmap_irq_reg_group { > unsigned int status_base; > unsigned int mask_base; > ... > > unsigned int irq_reg_stride; > > int num_regs; > }; > > The idea is that the registers in a group are linearly mapped using the > formula "base + (i * irq_reg_stride)". Then it's possible to allow for > multiple register groups in regmap_irq_chip: > > struct regmap_irq_chip { > const struct regmap_irq_reg_group *groups; > unsigned int num_groups; > > unsigned int main_status_base; > unsigned int num_main_status_bits; > int num_main_regs; > > ... > }; > > It should be straightforward to fit existing chips into this model. > > - Chips that use a main status + sub-block IRQ layout will define > one register group for each sub-block and continue to describe the > location of the main status registers inside of regmap_irq_chip. > A group will only get polled if the corresponding main status bit > is set -- n'th group is polled if n'th bit is set. Does this work for devices where a single main status bit can flag IRQs in more than one sub-registers? Best Regards -- Matti
Matti Vaittinen <mazziesaccount@gmail.com> writes: > Hi dee Ho peeps! > > Sorry for the late reply. > > pe 10. kesäk. 2022 klo 18.43 Aidan MacDonald > (aidanmacdonald.0x0@gmail.com) kirjoitti: >> >> Mark Brown <broonie@kernel.org> writes: >> >> > On Tue, Jun 07, 2022 at 04:53:09PM +0100, Aidan MacDonald wrote: >> > >> >> - if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { >> >> + if (chip->get_irq_reg) { >> >> + reg = chip->get_irq_reg(base_reg, i); >> >> + } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { >> > >> > It seems like it would be cleaner and clearer to refactor things so that >> > we always have a get_irq_reg() with standard chips getting given a >> > default implementation which implements the current behaviour. >> >> I don't think that is a good way to clean things up. I only intended >> get_irq_reg() to be a quick hack to solve a problem; in my opinion it >> would be a poor abstraction to base the API around. >> >> What I'd suggest is something that will simplify regmap-irq. Instead of >> defining the base registers, etc. in the chip, introduce a new struct >> to describe a register group: >> >> struct regmap_irq_reg_group { >> unsigned int status_base; >> unsigned int mask_base; >> ... >> >> unsigned int irq_reg_stride; >> >> int num_regs; >> }; >> >> The idea is that the registers in a group are linearly mapped using the >> formula "base + (i * irq_reg_stride)". Then it's possible to allow for >> multiple register groups in regmap_irq_chip: >> >> struct regmap_irq_chip { >> const struct regmap_irq_reg_group *groups; >> unsigned int num_groups; >> >> unsigned int main_status_base; >> unsigned int num_main_status_bits; >> int num_main_regs; >> >> ... >> }; >> >> It should be straightforward to fit existing chips into this model. >> >> - Chips that use a main status + sub-block IRQ layout will define >> one register group for each sub-block and continue to describe the >> location of the main status registers inside of regmap_irq_chip. >> A group will only get polled if the corresponding main status bit >> is set -- n'th group is polled if n'th bit is set. > > Does this work for devices where a single main status bit can flag > IRQs in more than one sub-registers? > > Best Regards > -- Matti No, I realized once I got into the refactor that what I outlined here wouldn't fit that use case well, which is what rohm-bd71828 needs. There are some other complications with this approach, like how to go between IRQs and register groups efficiently, and it's generally a rather heavyweight solution. It might be useful for handling very hierarchical chips, but I couldn't justify the added complexity when most chips don't need it -- after all most chips behind slow busses will have a small number of interrupts and a fairly flat structure. In the end I went with Mark's suggestion to factor things out around ->get_irq_reg(). At first I thought there might be too many "gotchas" that'd limit its usefulness, but in the end it proved to be a better option and a lot easier to implement.
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index 4a2e1f6aa94d..e50437b18284 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -55,7 +55,9 @@ static int sub_irq_reg(struct regmap_irq_chip_data *data, unsigned int offset; int reg = 0; - if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { + if (chip->get_irq_reg) { + reg = chip->get_irq_reg(base_reg, i); + } else if (!chip->sub_reg_offsets || !chip->not_fixed_stride) { /* Assume linear mapping */ reg = base_reg + (i * map->reg_stride * data->irq_reg_stride); } else { @@ -479,7 +481,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) { + data->irq_reg_stride == 1 && !chip->get_irq_reg) { u8 *buf8 = data->status_reg_buf; u16 *buf16 = data->status_reg_buf; diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 8952fa3d0d59..4828021ab9e8 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1495,6 +1495,10 @@ struct regmap_irq_sub_irq_map { * after handling the interrupts in regmap_irq_handler(). * @set_type_virt: Driver specific callback to extend regmap_irq_set_type() * and configure virt regs. + * @get_irq_reg: Callback to map a register index in range [0, num_regs[ + * to a register, relative to a specific base register. This + * is mainly useful for devices where the register offsets + * change depending on the base register. * @irq_drv_data: Driver specific IRQ data which is passed as parameter when * driver specific pre/post interrupt handler is called. * @@ -1545,6 +1549,7 @@ struct regmap_irq_chip { int (*handle_post_irq)(void *irq_drv_data); int (*set_type_virt)(unsigned int **buf, unsigned int type, unsigned long hwirq, int reg); + int (*get_irq_reg)(unsigned int base_reg, int i); void *irq_drv_data; };
Add a new callback, get_irq_reg, for regmap IRQ chips, to support devices with unusual register layouts. This is required in the rare cases where the offset of an IRQ register is not constant with respect to the base register. This is probably best illustrated with an example (taken from the AXP192 PMIC): mask status IRQ0 0x40 0x44 IRQ1 0x41 0x45 IRQ2 0x42 0x46 IRQ3 0x43 0x47 IRQ4 0x4a 0x4d If we set mask_base = 0x40 and status_base = 0x44, the offsets of each register relative to the base are: mask status IRQ0 0 0 IRQ1 1 1 IRQ2 2 2 IRQ3 3 3 IRQ4 10 9 The existing mapping mechanisms can't include IRQ4 in the same irqchip as IRQ0-3 because the offset of IRQ4's register depends on which type of register we're asking for, ie. which base register is used. The get_irq_reg callback allows drivers to specify an arbitrary mapping of (base register, register index) pairs to register addresses, instead of the default linear mapping "base_register + register_index". This allows unusual layouts, like the one above, to be handled using a single regmap IRQ chip. The drawback is that when get_irq_reg is used, it's impossible to use bulk reads for status registers even if some of them are contiguous, because the mapping is opaque to regmap-irq. This should be acceptable for the case of a few infrequently-polled status registers. Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> --- drivers/base/regmap/regmap-irq.c | 6 ++++-- include/linux/regmap.h | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-)