Message ID | cover.1615423027.git.gurus@codeaurora.org |
---|---|
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, 10 Mar 2021 16:39:51 -0800, Guru Das Srinagesh wrote: > Changes from v2: > - Split up framework changes patch for better comprehension. > - Dropped PM8008 driver example and converted it into example code in cover > letter and commit text. > - Added more info in cover letter and commit message as per v2 feedback. > > This is a follow-up as promised [1] to the earlier attempts [2] [3] to upstream > the driver that has been hitherto used to handle IRQs for Qualcomm's PMICs that > have multiple on-board peripherals when they are interfaced over the I2C > interface. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next Thanks! [1/3] regmap-irq: Extend sub-irq to support non-fixed reg strides commit: 1066cfbdfa3f5c401870fad577fe63d1171a5bcd 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