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.
This series is a rewrite of that driver while making use of the regmap-irq
framework, which needs some modifications to handle the register layout of
Qualcomm's PMICs. This is an RFC because I would like to get feedback on my
general approach before submitting as a patch per se.
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.
The differences between Qualcomm's register layout and what the regmap-irq
framework assumes are best described with an example:
Suppose that there are three peripherals onboard an MFD chip: MISC, TEMP_ALARM
and GPIO01. Each of these peripherals has the following IRQ configuration
registers: mask, type and ack. There is a top level interrupt status register
and per-peripheral status registers as well (not shown below).
The regmap-irq framework expects all similar registers to be at a fixed stride
from each other, for each peripheral, as below (with stride = 1).
/* All mask registers together */
MISC_INT_MASK 0x1011
TEMP_ALARM_INT_MASK 0x1012
GPIO01_INT_MASK 0x1013
/* All type registers together */
MISC_INT_TYPE 0x2011
TEMP_ALARM_INT_TYPE 0x2012
GPIO01_INT_TYPE 0x2013
/* All ack registers together */
MISC_INT_ACK 0x3011
TEMP_ALARM_INT_ACK 0x3012
GPIO01_INT_ACK 0x3013
In contrast, QCOM's registers are laid out as follows:
/* All of MISC peripheral's registers together */
MISC_INT_MASK 0x1011
MISC_INT_TYPE 0x1012
MISC_INT_ACK 0x1013
/* All of TEMP_ALARM peripheral's registers together */
TEMP_ALARM_INT_MASK 0x2011
TEMP_ALARM_INT_TYPE 0x2012
TEMP_ALARM_INT_ACK 0x2013
/* All of GPIO01 peripheral's registers together */
GPIO01_INT_MASK 0x3011
GPIO01_INT_TYPE 0x3012
GPIO01_INT_ACK 0x3013
As is evident, the different IRQ configuration registers are just (0x11, 0x12,
0x13) with offsets of (0x1000, 0x2000 and 0x3000) respectively in QCOM's case.
If the *_base registers fed to the struct regmap_irq_chip could be set to the
first peripheral (MISC in this example), then through the sub_reg_offsets
mechanism, we could add the offsets _to_ the *_base register values to get at
the configuration registers for each peripheral.
Hopefully this will help when reviewing the changes in this series.
[1] https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
Guru Das Srinagesh (3):
regmap-irq: Extend sub-irq to support non-fixed reg strides
regmap-irq: Add support for POLARITY_HI and POLARITY_LO config regs
regmap-irq: Modify type_buf handling for IRQ_TYPE_LEVEL_*
drivers/base/regmap/regmap-irq.c | 152 +++++++++++++++++++++++++++++++--------
include/linux/regmap.h | 11 +++
2 files changed, 133 insertions(+), 30 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
drivers/base/regmap/regmap-irq.c | 67 ++++++++++++++++++++++++++++++++++++++++
include/linux/regmap.h | 4 +++
2 files changed, 71 insertions(+)
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;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 <[email protected]>
---
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;
};
+static int sub_irq_reg(struct regmap_irq_chip_data *data,
+ unsigned int base_reg, int i)
+{
+ const struct regmap_irq_chip *chip = data->chip;
+ struct regmap *map = data->map;
+ struct regmap_irq_sub_irq_map *subreg;
+ unsigned int offset;
+ int reg = 0;
+
+ if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
+ /* Assume linear mapping */
+ reg = base_reg + (i * map->reg_stride * data->irq_reg_stride);
+ } else {
+ subreg = &chip->sub_reg_offsets[i];
+ offset = subreg->offset[0];
+ reg = base_reg + offset;
+ }
+
+ return reg;
+}
+
static inline const
struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data,
int irq)
@@ -87,8 +108,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
if (d->clear_status) {
for (i = 0; i < d->chip->num_regs; i++) {
- reg = d->chip->status_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->status_base, i);
ret = regmap_read(map, reg, &val);
if (ret)
@@ -108,8 +128,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
if (!d->chip->mask_base)
continue;
- reg = d->chip->mask_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->mask_base, i);
if (d->chip->mask_invert) {
ret = regmap_irq_update_bits(d, reg,
d->mask_buf_def[i], ~d->mask_buf[i]);
@@ -136,8 +155,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
dev_err(d->map->dev, "Failed to sync masks in %x\n",
reg);
- reg = d->chip->wake_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->wake_base, i);
if (d->wake_buf) {
if (d->chip->wake_invert)
ret = regmap_irq_update_bits(d, reg,
@@ -161,8 +179,8 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
* it'll be ignored in irq handler, then may introduce irq storm
*/
if (d->mask_buf[i] && (d->chip->ack_base || d->chip->use_ack)) {
- reg = d->chip->ack_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->ack_base, i);
+
/* some chips ack by write 0 */
if (d->chip->ack_invert)
ret = regmap_write(map, reg, ~d->mask_buf[i]);
@@ -187,8 +205,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
for (i = 0; i < d->chip->num_type_reg; i++) {
if (!d->type_buf_def[i])
continue;
- reg = d->chip->type_base +
- (i * map->reg_stride * d->type_reg_stride);
+ reg = sub_irq_reg(d, d->chip->type_base, i);
if (d->chip->type_invert)
ret = regmap_irq_update_bits(d, reg,
d->type_buf_def[i], ~d->type_buf[i]);
@@ -352,8 +369,15 @@ static inline int read_sub_irq_data(struct regmap_irq_chip_data *data,
for (i = 0; i < subreg->num_regs; i++) {
unsigned int offset = subreg->offset[i];
- ret = regmap_read(map, chip->status_base + offset,
- &data->status_buf[offset]);
+ if (chip->not_fixed_stride)
+ ret = regmap_read(map,
+ chip->status_base + offset,
+ &data->status_buf[b]);
+ else
+ ret = regmap_read(map,
+ chip->status_base + offset,
+ &data->status_buf[offset]);
+
if (ret)
break;
}
@@ -474,10 +498,9 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
} else {
for (i = 0; i < data->chip->num_regs; i++) {
- ret = regmap_read(map, chip->status_base +
- (i * map->reg_stride
- * data->irq_reg_stride),
- &data->status_buf[i]);
+ unsigned int reg = sub_irq_reg(data,
+ data->chip->status_base, i);
+ ret = regmap_read(map, reg, &data->status_buf[i]);
if (ret != 0) {
dev_err(map->dev,
@@ -499,8 +522,8 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
data->status_buf[i] &= ~data->mask_buf[i];
if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) {
- reg = chip->ack_base +
- (i * map->reg_stride * data->irq_reg_stride);
+ reg = sub_irq_reg(data, data->chip->ack_base, i);
+
if (chip->ack_invert)
ret = regmap_write(map, reg,
~data->status_buf[i]);
@@ -605,6 +628,12 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
return -EINVAL;
}
+ if (chip->not_fixed_stride) {
+ for (i = 0; i < chip->num_regs; i++)
+ if (chip->sub_reg_offsets[i].num_regs != 1)
+ return -EINVAL;
+ }
+
if (irq_base) {
irq_base = irq_alloc_descs(irq_base, 0, chip->num_irqs, 0);
if (irq_base < 0) {
@@ -700,8 +729,8 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
if (!chip->mask_base)
continue;
- reg = chip->mask_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->mask_base, i);
+
if (chip->mask_invert)
ret = regmap_irq_update_bits(d, reg,
d->mask_buf[i], ~d->mask_buf[i]);
@@ -725,8 +754,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
continue;
/* Ack masked but set interrupts */
- reg = chip->status_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->status_base, i);
ret = regmap_read(map, reg, &d->status_buf[i]);
if (ret != 0) {
dev_err(map->dev, "Failed to read IRQ status: %d\n",
@@ -735,8 +763,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
}
if (d->status_buf[i] && (chip->ack_base || chip->use_ack)) {
- reg = chip->ack_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->ack_base, i);
if (chip->ack_invert)
ret = regmap_write(map, reg,
~(d->status_buf[i] & d->mask_buf[i]));
@@ -765,8 +792,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
if (d->wake_buf) {
for (i = 0; i < chip->num_regs; i++) {
d->wake_buf[i] = d->mask_buf_def[i];
- reg = chip->wake_base +
- (i * map->reg_stride * d->irq_reg_stride);
+ reg = sub_irq_reg(d, d->chip->wake_base, i);
if (chip->wake_invert)
ret = regmap_irq_update_bits(d, reg,
@@ -786,8 +812,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
if (chip->num_type_reg && !chip->type_in_mask) {
for (i = 0; i < chip->num_type_reg; ++i) {
- reg = chip->type_base +
- (i * map->reg_stride * d->type_reg_stride);
+ reg = sub_irq_reg(d, d->chip->type_base, i);
ret = regmap_read(map, reg, &d->type_buf_def[i]);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 2cc4ecd..18910bd 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1378,6 +1378,9 @@ struct regmap_irq_sub_irq_map {
* status_base. Should contain num_regs arrays.
* Can be provided for chips with more complex mapping than
* 1.st bit to 1.st sub-reg, 2.nd bit to 2.nd sub-reg, ...
+ * When used with not_fixed_stride, each one-element array
+ * member contains offset calculated as address from each
+ * peripheral to first peripheral.
* @num_main_regs: Number of 'main status' irq registers for chips which have
* main_status set.
*
@@ -1404,6 +1407,9 @@ struct regmap_irq_sub_irq_map {
* @clear_on_unmask: For chips with interrupts cleared on read: read the status
* registers before unmasking interrupts to clear any bits
* set when they were masked.
+ * @not_fixed_stride: Used when chip peripherals are not laid out with fixed
+ * stride. Must be used with sub_reg_offsets containing the
+ * offsets to each peripheral.
* @runtime_pm: Hold a runtime PM lock on the device when accessing it.
*
* @num_regs: Number of registers in each control bank.
@@ -1450,6 +1456,7 @@ struct regmap_irq_chip {
bool type_invert:1;
bool type_in_mask:1;
bool clear_on_unmask:1;
+ bool not_fixed_stride:1;
int num_regs;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
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 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 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: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
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.
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 <[email protected]>
> ---
> 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).
> +{
> + const struct regmap_irq_chip *chip = data->chip;
> + struct regmap *map = data->map;
> + struct regmap_irq_sub_irq_map *subreg;
> + unsigned int offset;
> + int reg = 0;
> +
> + if (!chip->sub_reg_offsets || !chip->not_fixed_stride) {
> + /* Assume linear mapping */
> + reg = base_reg + (i * map->reg_stride * data-
> >irq_reg_stride);
> + } else {
> + subreg = &chip->sub_reg_offsets[i];
> + offset = subreg->offset[0];
> + reg = base_reg + offset;
> + }
> +
> + return reg;
> +}
> +
> static inline const
> struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data
> *data,
> int irq)
> @@ -87,8 +108,7 @@ static void regmap_irq_sync_unlock(struct irq_data
> *data)
>
> if (d->clear_status) {
> for (i = 0; i < d->chip->num_regs; i++) {
> - reg = d->chip->status_base +
> - (i * map->reg_stride * d-
> >irq_reg_stride);
> + reg = sub_irq_reg(d, d->chip->status_base, i);
How does this work with the case where we have many subregs pointed by
single main-status register bit? If I read this correctly, then the
chip->num_regs can be greater than amount of meaningful main-status
bits and thus greater than amount of map entries.
I don't have BD71828 or BD70528 at home so I haven't tested this. So
hopefully I misunderstand something - but if I don't, then this change
will break the existing main-IRQ functionality. I will visit the office
later this week and I'll see if I have a chance to test this - but I
hope you can check your change still supports the case where one main
status IRQ bit signals more than one sub-register with active IRQs.
Best Regards
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
K
iviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)
Hi Matti,
On Mon, Apr 12, 2021 at 11:08:49AM +0000, Vaittinen, Matti wrote:
> 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 <[email protected]>
> > > ---
> > > 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'.
Yes, that's right. With this change, the sub-irq concept which was
initially introduced to map the main-irq bits to sub-irq registers is
being extended and repurposed to cover the specific memory layout
described in the commit message and cover letter.
I've updated the comment block in the header file for `sub_reg_offsets`
to make this clarification as well.
The sub_irq_reg() function will not break existing functionality because
the crux of it will get executed only if not_fixed_stride is set.
>
> 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!
Thanks for testing this out and providing confirmation, Matti :)
Thank you.
Guru Das.
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 <[email protected]>
> > ---
> > 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