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
