Re: [RFC PATCH v3 1/3] regmap-irq: Extend sub-irq to support non-fixed reg strides

[Vaittinen, Matti]

Hi,

On Mon, 2021-04-12 at 09:05 +0300, Matti Vaittinen wrote:
> Hi All,
> 
> On Wed, 2021-03-10 at 16:39 -0800, Guru Das Srinagesh wrote:
> > Qualcomm's MFD chips have a top level interrupt status register and
> > sub-irqs (peripherals).  When a bit in the main status register
> > goes
> > high, it means that the peripheral corresponding to that bit has an
> > unserviced interrupt. If the bit is not set, this means that the
> > corresponding peripheral does not.
> > 
> > Commit a2d21848d9211d ("regmap: regmap-irq: Add main status
> > register
> > support") introduced the sub-irq logic that is currently applied
> > only
> > when reading status registers, but not for any other functions like
> > acking
> > or masking. Extend the use of sub-irq to all other functions, with
> > two
> > caveats regarding the specification of offsets:
> > 
> > - Each member of the sub_reg_offsets array should be of length 1
> > - The specified offsets should be the unequal strides for each sub-
> > irq
> >   device.
> > 
> > In QCOM's case, all the *_base registers are to be configured to
> > the
> > base addresses of the first sub-irq group, with offsets of each
> > subsequent group calculated as a difference from these addresses.
> > 
> > Continuing from the example mentioned in the cover letter:
> > 
> > 	/*
> > 	 * Address of MISC_INT_MASK		= 0x1011
> > 	 * Address of TEMP_ALARM_INT_MASK	= 0x2011
> > 	 * Address of GPIO01_INT_MASK		= 0x3011
> > 	 *
> > 	 * Calculate offsets as:
> > 	 * offset_0 = 0x1011 - 0x1011 = 0       (to access MISC's
> > 	 * 					 registers)
> > 	 * offset_1 = 0x2011 - 0x1011 = 0x1000
> > 	 * offset_2 = 0x3011 - 0x1011 = 0x2000
> > 	 */
> > 
> > 	static unsigned int sub_unit0_offsets[] = {0};
> > 	static unsigned int sub_unit1_offsets[] = {0x1000};
> > 	static unsigned int sub_unit2_offsets[] = {0x2000};
> > 
> > 	static struct regmap_irq_sub_irq_map chip_sub_irq_offsets[] = {
> > 		REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > 		REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > 		REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > 	};
> > 
> > 	static struct regmap_irq_chip chip_irq_chip = {
> > 	--------8<--------
> > 	.not_fixed_stride = true,
> > 	.mask_base	  = MISC_INT_MASK,
> > 	.type_base	  = MISC_INT_TYPE,
> > 	.ack_base	  = MISC_INT_ACK,
> > 	.sub_reg_offsets  = chip_sub_irq_offsets,
> > 	--------8<--------
> > 	};
> > 
> > Signed-off-by: Guru Das Srinagesh <gurus@xxxxxxxxxxxxxx>
> > ---
> >  drivers/base/regmap/regmap-irq.c | 81 ++++++++++++++++++++++++++--
> > ------------
> >  include/linux/regmap.h           |  7 ++++
> >  2 files changed, 60 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/base/regmap/regmap-irq.c
> > b/drivers/base/regmap/regmap-irq.c
> > index 19db764..e1d8fc9e 100644
> > --- a/drivers/base/regmap/regmap-irq.c
> > +++ b/drivers/base/regmap/regmap-irq.c
> > @@ -45,6 +45,27 @@ struct regmap_irq_chip_data {
> >  	bool clear_status:1;
> >  };
> > 
> 
> Sorry that I am late with the "review" but I only now noticed this
> change when I was following the references from PM8008 PMIC patch
> mail.
> 
> 
> >  
> > +static int sub_irq_reg(struct regmap_irq_chip_data *data,
> > +		       unsigned int base_reg, int i)
> 
> Do I read this correctly - this function should map the main status
> bit
> (given in i) to the (sub)IRQ register, right? How does this work for
> cases where one bit corresponds to more than one sub-register? Or do
> I
> misunderstand the purpose of this function? (This is the case with
> both
> the BD70528 and BD71828).

I did some quick test with BD71815 which I had at home. And it seems to
be I did indeed misunderstand this :) This is not for converting the
main-IRQ bits to sub-irq registers - this is for getting the sub-IRQ
register address based on the 'sub IRQ register index'.

So I do apologize the noise, it seems all is well and everything
(except my self confidence) keeps working as it did :)

Thanks for the improvement Guru Das!

Best Regards
	Matti Vaittinen