| Message ID | 4b77a308ccdabbe96ed68623bd6eead9510e1fc9.1615423027.git.gurus@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Qualcomm MFD PMIC register layout | expand |
On Wed, Mar 10, 2021 at 04:39:53PM -0800, Guru Das Srinagesh wrote: > If an interrupt is already configured as either edge- or > level-triggered, setting the corresponding bit for it in the POLARITY_HI > register further configures it as rising-edge or level-high triggered > (as the case may be), while setting the same bit in POLARITY_LO further > configures it as falling-edge or level-low triggered. I think you probably need to bring these three fields together into a single virtual field and then map the values within that field using the existing type stuff.
On Fri, Mar 12, 2021 at 12:19:16PM +0000, Mark Brown wrote: > On Wed, Mar 10, 2021 at 04:39:53PM -0800, Guru Das Srinagesh wrote: > > If an interrupt is already configured as either edge- or > > level-triggered, setting the corresponding bit for it in the POLARITY_HI > > register further configures it as rising-edge or level-high triggered > > (as the case may be), while setting the same bit in POLARITY_LO further > > configures it as falling-edge or level-low triggered. > > I think you probably need to bring these three fields together into a > single virtual field and then map the values within that field using > the existing type stuff. Sure, how about this scheme then, for patches 2 and 3 in this series? (Patch 1 will remain the same, pending your review of it.) Since I do need to write to two extra registers, I'll need two register_base's and two buffers to hold their data. This can be generalized to "extra config registers" in the framework as follows: - Add these two fields to `struct regmap_irq_chip`: unsigned int *extra_config_base; /* Points to array of extra regs */ int num_extra_config_regs; /* = ARRAY_SIZE(array above) */ - Add this field to `struct regmap_irq_chip_data`: unsigned int **extra_config_buf; /* Will be dynamically allocated to size of: * [chip->num_extra_config_regs][chip->num_regs] */ - Add a new function callback in `struct regmap_irq_chip`: int (*configure_extra_regs)(void *irq_drv_data, unsigned int type) This callback will be called at the end of regmap_irq_set_type(): if (d->chip->configure_extra_regs) d->chip->configure_extra_regs(); The callback, defined in the client driver, will specifically address the changes to regmap_irq_set_type() made in patches 2 and 3 of this series, viz. overriding how type_buf is to be handled, plus the populating of polarity_*_buf's (rechristened as extra_config_bufs in this proposed new scheme). This new scheme thus makes v2 more generic. I thought I'd discuss this with you before implementing it as v3 RFC. Could you please let me know your thoughts? Thank you. Guru Das.
On Mon, Mar 15, 2021 at 01:33:36PM -0700, Guru Das Srinagesh wrote: > On Fri, Mar 12, 2021 at 12:19:16PM +0000, Mark Brown wrote: > > On Wed, Mar 10, 2021 at 04:39:53PM -0800, Guru Das Srinagesh wrote: > > > If an interrupt is already configured as either edge- or > > > level-triggered, setting the corresponding bit for it in the POLARITY_HI > > > register further configures it as rising-edge or level-high triggered > > > (as the case may be), while setting the same bit in POLARITY_LO further > > > configures it as falling-edge or level-low triggered. > > > > I think you probably need to bring these three fields together into a > > single virtual field and then map the values within that field using > > the existing type stuff. > > Sure, how about this scheme then, for patches 2 and 3 in this series? > (Patch 1 will remain the same, pending your review of it.) > > Since I do need to write to two extra registers, I'll need two > register_base's and two buffers to hold their data. This can be > generalized to "extra config registers" in the framework as follows: > > - Add these two fields to `struct regmap_irq_chip`: > > unsigned int *extra_config_base; /* Points to array of extra regs */ > int num_extra_config_regs; /* = ARRAY_SIZE(array above) */ > > - Add this field to `struct regmap_irq_chip_data`: > > unsigned int **extra_config_buf; > /* Will be dynamically allocated to size of: > * [chip->num_extra_config_regs][chip->num_regs] > */ > > - Add a new function callback in `struct regmap_irq_chip`: > > int (*configure_extra_regs)(void *irq_drv_data, unsigned int > type) > > This callback will be called at the end of regmap_irq_set_type(): > > if (d->chip->configure_extra_regs) > d->chip->configure_extra_regs(); > > The callback, defined in the client driver, will specifically address > the changes to regmap_irq_set_type() made in patches 2 and 3 of this > series, viz. overriding how type_buf is to be handled, plus the > populating of polarity_*_buf's (rechristened as extra_config_bufs in > this proposed new scheme). > > This new scheme thus makes v2 more generic. I thought I'd discuss this > with you before implementing it as v3 RFC. Could you please let me know > your thoughts? Typo. I meant: This new scheme thus makes *v3* more generic. I thought I'd discuss this with you before implementing it as *v4* RFC. Guru Das.
On Mon, Mar 15, 2021 at 01:33:37PM -0700, Guru Das Srinagesh wrote: > Since I do need to write to two extra registers, I'll need two > register_base's and two buffers to hold their data. This can be > generalized to "extra config registers" in the framework as follows: > > - Add these two fields to `struct regmap_irq_chip`: > > unsigned int *extra_config_base; /* Points to array of extra regs */ > int num_extra_config_regs; /* = ARRAY_SIZE(array above) */ I'm having a hard time loving this but I'm also not able to think of any better ideas so sure. I'd change the name to virtual (or virt) rather than extra since that's what they are so it makes it a bit omre clear.
On Wed, Mar 17, 2021 at 08:42:12PM +0000, Mark Brown wrote: > On Mon, Mar 15, 2021 at 01:33:37PM -0700, Guru Das Srinagesh wrote: > > > Since I do need to write to two extra registers, I'll need two > > register_base's and two buffers to hold their data. This can be > > generalized to "extra config registers" in the framework as follows: > > > > - Add these two fields to `struct regmap_irq_chip`: > > > > unsigned int *extra_config_base; /* Points to array of extra regs */ > > int num_extra_config_regs; /* = ARRAY_SIZE(array above) */ > > I'm having a hard time loving this but I'm also not able to think of any > better ideas so sure. I'd change the name to virtual (or virt) rather > than extra since that's what they are so it makes it a bit omre clear. Thanks for accepting the first patch in this series. I will test out my proposed changes and then send a new patchset sometime next week. Thank you. Guru Das.
diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c index e1d8fc9e..cb13855 100644 --- a/drivers/base/regmap/regmap-irq.c +++ b/drivers/base/regmap/regmap-irq.c @@ -38,6 +38,8 @@ struct regmap_irq_chip_data { unsigned int *wake_buf; unsigned int *type_buf; unsigned int *type_buf_def; + unsigned int *polarity_hi_buf; + unsigned int *polarity_lo_buf; unsigned int irq_reg_stride; unsigned int type_reg_stride; @@ -215,6 +217,22 @@ static void regmap_irq_sync_unlock(struct irq_data *data) if (ret != 0) dev_err(d->map->dev, "Failed to sync type in %x\n", reg); + + if (!d->chip->polarity_hi_base || + !d->chip->polarity_lo_base) + continue; + + reg = sub_irq_reg(d, d->chip->polarity_hi_base, i); + ret = regmap_write(map, reg, d->polarity_hi_buf[i]); + if (ret != 0) + dev_err(d->map->dev, "Failed to sync polarity hi in %x\n", + reg); + + reg = sub_irq_reg(d, d->chip->polarity_lo_base, i); + ret = regmap_write(map, reg, d->polarity_lo_buf[i]); + if (ret != 0) + dev_err(d->map->dev, "Failed to sync polarity lo in %x\n", + reg); } } @@ -318,6 +336,41 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type) default: return -EINVAL; } + + + if (d->chip->polarity_hi_base && d->chip->polarity_lo_base) { + switch (type) { + case IRQ_TYPE_EDGE_FALLING: + d->polarity_hi_buf[reg] &= ~t->type_falling_val; + d->polarity_lo_buf[reg] |= t->type_falling_val; + break; + + case IRQ_TYPE_EDGE_RISING: + d->polarity_hi_buf[reg] |= t->type_rising_val; + d->polarity_lo_buf[reg] &= ~t->type_rising_val; + break; + + case IRQ_TYPE_EDGE_BOTH: + d->polarity_hi_buf[reg] |= (t->type_falling_val | + t->type_rising_val); + d->polarity_lo_buf[reg] |= (t->type_falling_val | + t->type_rising_val); + break; + + case IRQ_TYPE_LEVEL_HIGH: + d->polarity_hi_buf[reg] |= t->type_level_high_val; + d->polarity_lo_buf[reg] &= ~t->type_level_high_val; + break; + + case IRQ_TYPE_LEVEL_LOW: + d->polarity_hi_buf[reg] &= ~t->type_level_low_val; + d->polarity_lo_buf[reg] |= t->type_level_low_val; + break; + default: + return -EINVAL; + } + } + return 0; } @@ -691,6 +744,16 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, goto err_alloc; } + d->polarity_hi_buf = kcalloc(chip->num_regs, sizeof(unsigned int), + GFP_KERNEL); + if (!d->polarity_hi_buf) + goto err_alloc; + + d->polarity_lo_buf = kcalloc(chip->num_regs, sizeof(unsigned int), + GFP_KERNEL); + if (!d->polarity_lo_buf) + goto err_alloc; + d->irq_chip = regmap_irq_chip; d->irq_chip.name = chip->name; d->irq = irq; @@ -857,6 +920,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, /* Should really dispose of the domain but... */ err_alloc: kfree(d->type_buf); + kfree(d->polarity_hi_buf); + kfree(d->polarity_lo_buf); kfree(d->type_buf_def); kfree(d->wake_buf); kfree(d->mask_buf_def); @@ -927,6 +992,8 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d) irq_domain_remove(d->domain); kfree(d->type_buf); + kfree(d->polarity_hi_buf); + kfree(d->polarity_lo_buf); kfree(d->type_buf_def); kfree(d->wake_buf); kfree(d->mask_buf_def); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 18910bd..0f1329d 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -1393,6 +1393,8 @@ 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. + * @polarity_hi_base: Base address for specifying edge- or level-high triggered + * @polarity_lo_base: Base address for specifying edge- or level-low triggered * @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. @@ -1444,6 +1446,8 @@ struct regmap_irq_chip { unsigned int ack_base; unsigned int wake_base; unsigned int type_base; + unsigned int polarity_hi_base; + unsigned int polarity_lo_base; unsigned int irq_reg_stride; bool mask_writeonly:1; bool init_ack_masked:1;
If an interrupt is already configured as either edge- or level-triggered, setting the corresponding bit for it in the POLARITY_HI register further configures it as rising-edge or level-high triggered (as the case may be), while setting the same bit in POLARITY_LO further configures it as falling-edge or level-low triggered. Therefore, in QCOM's case, the type_*_val are all to be configured as BIT() masks. These two new configuration registers are QCOM-specific and hence, the code is not generic in its current form. I'm looking for feedback on how this may be made generic. Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org> --- drivers/base/regmap/regmap-irq.c | 67 ++++++++++++++++++++++++++++++++++++++++ include/linux/regmap.h | 4 +++ 2 files changed, 71 insertions(+)