linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] regmap: irq: Add support for interrupt type
@ 2013-02-13 13:14 Laxman Dewangan
  2013-02-13 13:14 ` [PATCH 2/2] regmap: irq: do not write mask register if it is not supported Laxman Dewangan
  2013-02-13 13:54 ` [PATCH 1/2] regmap: irq: Add support for interrupt type Mark Brown
  0 siblings, 2 replies; 11+ messages in thread
From: Laxman Dewangan @ 2013-02-13 13:14 UTC (permalink / raw)
  To: broonie, gregkh; +Cc: linux-kernel, Laxman Dewangan

Most of the MFD module have the GPIO and gpio interrupt is
enabled/disabled through the rising/falling edge type. There
is no specific mask enable/disable registers for GPIO submodules.

Extend the regmap irq to support the irq_set_type() so that
gpio submodule of MFD devices can use the regmap-irq framework
for their interrupt registration.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/base/regmap/regmap-irq.c |   97 ++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           |   15 ++++++
 2 files changed, 112 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 4706c63..e7a1a26 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -39,8 +39,11 @@ struct regmap_irq_chip_data {
 	unsigned int *mask_buf;
 	unsigned int *mask_buf_def;
 	unsigned int *wake_buf;
+	unsigned int *type_buf;
+	unsigned int *type_buf_def;
 
 	unsigned int irq_reg_stride;
+	unsigned int type_reg_stride;
 };
 
 static inline const
@@ -107,6 +110,22 @@ 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);
+		if (d->chip->type_invert)
+			ret = regmap_update_bits(d->map, reg,
+				d->type_buf_def[i], ~d->type_buf[i]);
+		else
+			ret = regmap_update_bits(d->map, reg,
+				d->type_buf_def[i], d->type_buf[i]);
+		if (ret != 0)
+			dev_err(d->map->dev, "Failed to sync type in %x\n",
+				reg);
+	}
+
 	if (d->chip->runtime_pm)
 		pm_runtime_put(map->dev);
 
@@ -141,6 +160,38 @@ static void regmap_irq_disable(struct irq_data *data)
 	d->mask_buf[irq_data->reg_offset / map->reg_stride] |= irq_data->mask;
 }
 
+static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
+	struct regmap *map = d->map;
+	const struct regmap_irq *irq_data = irq_to_regmap_irq(d, data->hwirq);
+	int reg = irq_data->type_reg_offset / map->reg_stride;
+
+	if (!(irq_data->type_rising_mask | irq_data->type_falling_mask))
+		return 0;
+
+	d->type_buf[reg] &= ~(irq_data->type_falling_mask |
+					irq_data->type_rising_mask);
+	switch (type) {
+	case IRQ_TYPE_EDGE_FALLING:
+		d->type_buf[reg] |= irq_data->type_falling_mask;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		d->type_buf[reg] |= irq_data->type_rising_mask;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		d->type_buf[reg] |= (irq_data->type_falling_mask |
+					irq_data->type_rising_mask);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int regmap_irq_set_wake(struct irq_data *data, unsigned int on)
 {
 	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
@@ -167,6 +218,7 @@ static const struct irq_chip regmap_irq_chip = {
 	.irq_bus_sync_unlock	= regmap_irq_sync_unlock,
 	.irq_disable		= regmap_irq_disable,
 	.irq_enable		= regmap_irq_enable,
+	.irq_set_type		= regmap_irq_set_type,
 	.irq_set_wake		= regmap_irq_set_wake,
 };
 
@@ -375,6 +427,18 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 			goto err_alloc;
 	}
 
+	if (chip->num_type_reg) {
+		d->type_buf_def = kzalloc(sizeof(unsigned int) *
+					chip->num_type_reg, GFP_KERNEL);
+		if (!d->type_buf_def)
+			goto err_alloc;
+
+		d->type_buf = kzalloc(sizeof(unsigned int) * chip->num_type_reg,
+				      GFP_KERNEL);
+		if (!d->type_buf)
+			goto err_alloc;
+	}
+
 	d->irq_chip = regmap_irq_chip;
 	d->irq_chip.name = chip->name;
 	d->irq = irq;
@@ -387,6 +451,8 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	else
 		d->irq_reg_stride = 1;
 
+	d->type_reg_stride = chip->type_reg_stride ? : 1;
+
 	if (!map->use_single_rw && map->reg_stride == 1 &&
 	    d->irq_reg_stride == 1) {
 		d->status_reg_buf = kmalloc(map->format.val_bytes *
@@ -442,6 +508,33 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 		}
 	}
 
+	if (chip->num_type_reg) {
+		for (i = 0; i < chip->num_irqs; i++) {
+			reg = chip->irqs[i].type_reg_offset / map->reg_stride;
+			d->type_buf_def[reg] |= chip->irqs[i].type_rising_mask |
+					chip->irqs[i].type_falling_mask;
+		}
+		for (i = 0; i < chip->num_type_reg; ++i) {
+			if (!d->type_buf_def[i])
+				continue;
+
+			reg = chip->type_base +
+				(i * map->reg_stride * d->type_reg_stride);
+			if (chip->type_invert)
+				ret = regmap_update_bits(map, reg,
+					d->type_buf_def[i], ~d->type_buf[i]);
+			else
+				ret = regmap_update_bits(map, reg,
+					d->type_buf_def[i], d->type_buf[i]);
+			if (ret != 0) {
+				dev_err(map->dev,
+					"Failed to set type in 0x%x: %x\n",
+					reg, ret);
+				goto err_alloc;
+			}
+		}
+	}
+
 	if (irq_base)
 		d->domain = irq_domain_add_legacy(map->dev->of_node,
 						  chip->num_irqs, irq_base, 0,
@@ -468,6 +561,8 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 err_domain:
 	/* Should really dispose of the domain but... */
 err_alloc:
+	kfree(d->type_buf);
+	kfree(d->type_buf_def);
 	kfree(d->wake_buf);
 	kfree(d->mask_buf_def);
 	kfree(d->mask_buf);
@@ -491,6 +586,8 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *d)
 
 	free_irq(irq, d);
 	/* We should unmap the domain but... */
+	kfree(d->type_buf);
+	kfree(d->type_buf_def);
 	kfree(d->wake_buf);
 	kfree(d->mask_buf_def);
 	kfree(d->mask_buf);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 5e81abf..c98db7d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -348,10 +348,16 @@ bool regmap_reg_in_ranges(unsigned int reg,
  *
  * @reg_offset: Offset of the status/mask register within the bank
  * @mask:       Mask used to flag/control the register.
+ * @type_reg_offset: Offset register for the irq type setting.
+ * @type_rising_mask: Mask bit to configure RISING type irq.
+ * @type_falling_mask: Mask bit to configure FALLING type irq.
  */
 struct regmap_irq {
 	unsigned int reg_offset;
 	unsigned int mask;
+	unsigned int type_reg_offset;
+	unsigned int type_rising_mask;
+	unsigned int type_falling_mask;
 };
 
 /**
@@ -365,6 +371,7 @@ struct regmap_irq {
  * @mask_base:   Base mask register address.
  * @ack_base:    Base ack address.  If zero then the chip is clear on read.
  * @wake_base:   Base address for wake enables.  If zero unsupported.
+ * @type_base:   Base address for irq type.  If zero unsupported.
  * @irq_reg_stride:  Stride to use for chips where registers are not contiguous.
  * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  *
@@ -372,6 +379,9 @@ struct regmap_irq {
  * @irqs:        Descriptors for individual IRQs.  Interrupt numbers are
  *               assigned based on the index in the array of the interrupt.
  * @num_irqs:    Number of descriptors.
+ * @num_type_reg:    Number of type registers.
+ * @type_reg_stride: Stride to use for chips where type registers are not
+ *			contiguous.
  */
 struct regmap_irq_chip {
 	const char *name;
@@ -380,15 +390,20 @@ struct regmap_irq_chip {
 	unsigned int mask_base;
 	unsigned int ack_base;
 	unsigned int wake_base;
+	unsigned int type_base;
 	unsigned int irq_reg_stride;
 	unsigned int mask_invert;
 	unsigned int wake_invert;
+	unsigned int type_invert;
 	bool runtime_pm;
 
 	int num_regs;
 
 	const struct regmap_irq *irqs;
 	int num_irqs;
+
+	int num_type_reg;
+	unsigned int type_reg_stride;
 };
 
 struct regmap_irq_chip_data;
-- 
1.7.1.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-13 13:14 [PATCH 1/2] regmap: irq: Add support for interrupt type Laxman Dewangan
@ 2013-02-13 13:14 ` Laxman Dewangan
  2013-02-13 14:20   ` Mark Brown
  2013-02-13 13:54 ` [PATCH 1/2] regmap: irq: Add support for interrupt type Mark Brown
  1 sibling, 1 reply; 11+ messages in thread
From: Laxman Dewangan @ 2013-02-13 13:14 UTC (permalink / raw)
  To: broonie, gregkh; +Cc: linux-kernel, Laxman Dewangan

Ignore the mask register write if mask_base is not provided by
regmap irq client. This is useful when regmap irq framework is
used for the MFD's gpio interrupt support. Typically, gpio has
two registers related to interrupt, one is for setting interrupt
edge type like rising and falling which indirectly work as mask
interrupt register and second as interrupt status register. In
this case mask_base is set as 0.

Now when interrupt enabled/disabled, the mask_buf get set/reset as
it keeps the interrupt enable/disable state but need not to be write
into register. When interrupt occurs, it is masked with interrupt mask
to check interrupt is valid or not and accordingly the isr handler
get called.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/base/regmap/regmap-irq.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index e7a1a26..dbdd407 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -80,6 +80,9 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	 * suppress pointless writes.
 	 */
 	for (i = 0; i < d->chip->num_regs; i++) {
+		if (!d->chip->mask_base)
+			goto skip_mask_reg_update;
+
 		reg = d->chip->mask_base +
 			(i * map->reg_stride * d->irq_reg_stride);
 		if (d->chip->mask_invert)
@@ -92,6 +95,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 			dev_err(d->map->dev, "Failed to sync masks in %x\n",
 				reg);
 
+skip_mask_reg_update:
 		reg = d->chip->wake_base +
 			(i * map->reg_stride * d->irq_reg_stride);
 		if (d->wake_buf) {
@@ -470,6 +474,9 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	/* Mask all the interrupts by default */
 	for (i = 0; i < chip->num_regs; i++) {
 		d->mask_buf[i] = d->mask_buf_def[i];
+		if (!chip->mask_base)
+			continue;
+
 		reg = chip->mask_base +
 			(i * map->reg_stride * d->irq_reg_stride);
 		if (chip->mask_invert)
-- 
1.7.1.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] regmap: irq: Add support for interrupt type
  2013-02-13 13:14 [PATCH 1/2] regmap: irq: Add support for interrupt type Laxman Dewangan
  2013-02-13 13:14 ` [PATCH 2/2] regmap: irq: do not write mask register if it is not supported Laxman Dewangan
@ 2013-02-13 13:54 ` Mark Brown
  2013-02-14 11:01   ` Laxman Dewangan
  1 sibling, 1 reply; 11+ messages in thread
From: Mark Brown @ 2013-02-13 13:54 UTC (permalink / raw)
  To: Laxman Dewangan; +Cc: gregkh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On Wed, Feb 13, 2013 at 06:44:49PM +0530, Laxman Dewangan wrote:
> Most of the MFD module have the GPIO and gpio interrupt is
> enabled/disabled through the rising/falling edge type. There
> is no specific mask enable/disable registers for GPIO submodules.
> 
> Extend the regmap irq to support the irq_set_type() so that
> gpio submodule of MFD devices can use the regmap-irq framework
> for their interrupt registration.

This commit message needs to be rewritten for comprehensibility, MFD has
nothing to do with this - it's something to do with your particular
hardware that you're trying to support.  Describe the hardware feature,
not the subsystem your hardware happens to be used in.

> +	switch (type) {
> +	case IRQ_TYPE_EDGE_FALLING:
> +		d->type_buf[reg] |= irq_data->type_falling_mask;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_RISING:
> +		d->type_buf[reg] |= irq_data->type_rising_mask;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		d->type_buf[reg] |= (irq_data->type_falling_mask |
> +					irq_data->type_rising_mask);
> +		break;

This should handle the case where an output type is not supported, for
example if the device only supports falling edge.  It'd also seem
sensible to have an explicit value to set for each trigger type rather
than reyling on the device using separate bits for everything - you
might see an encoding like 0 for falling, 1 for rising, 2 for both and 3
for level for example.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-13 13:14 ` [PATCH 2/2] regmap: irq: do not write mask register if it is not supported Laxman Dewangan
@ 2013-02-13 14:20   ` Mark Brown
  2013-02-14 11:06     ` Laxman Dewangan
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2013-02-13 14:20 UTC (permalink / raw)
  To: Laxman Dewangan; +Cc: gregkh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

On Wed, Feb 13, 2013 at 06:44:50PM +0530, Laxman Dewangan wrote:
> Ignore the mask register write if mask_base is not provided by
> regmap irq client. This is useful when regmap irq framework is
> used for the MFD's gpio interrupt support. Typically, gpio has
> two registers related to interrupt, one is for setting interrupt

Again you're talking about specific devices as though these are generic
things related to the class of device.

>  	for (i = 0; i < d->chip->num_regs; i++) {
> +		if (!d->chip->mask_base)
> +			goto skip_mask_reg_update;
> +

Why is this inside the loop?

I'd also expect us to return an error if a caller tries to enable or
disable an interrupt, or possibly to give different ops to the IRQ
subsystem, rather than just silently claim we did what we were asked.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] regmap: irq: Add support for interrupt type
  2013-02-13 13:54 ` [PATCH 1/2] regmap: irq: Add support for interrupt type Mark Brown
@ 2013-02-14 11:01   ` Laxman Dewangan
  2013-02-14 11:06     ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Laxman Dewangan @ 2013-02-14 11:01 UTC (permalink / raw)
  To: Mark Brown; +Cc: gregkh, linux-kernel

On Wednesday 13 February 2013 07:24 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Wed, Feb 13, 2013 at 06:44:49PM +0530, Laxman Dewangan wrote:
>> Most of the MFD module have the GPIO and gpio interrupt is
>> enabled/disabled through the rising/falling edge type. There
>> is no specific mask enable/disable registers for GPIO submodules.
>>
>> Extend the regmap irq to support the irq_set_type() so that
>> gpio submodule of MFD devices can use the regmap-irq framework
>> for their interrupt registration.
> This commit message needs to be rewritten for comprehensibility, MFD has
> nothing to do with this - it's something to do with your particular
> hardware that you're trying to support.  Describe the hardware feature,
> not the subsystem your hardware happens to be used in.

Fine, I will try to write better.

>
>> +	switch (type) {
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		d->type_buf[reg] |= irq_data->type_falling_mask;
>> +		break;
>> +
>> +	case IRQ_TYPE_EDGE_RISING:
>> +		d->type_buf[reg] |= irq_data->type_rising_mask;
>> +		break;
>> +
>> +	case IRQ_TYPE_EDGE_BOTH:
>> +		d->type_buf[reg] |= (irq_data->type_falling_mask |
>> +					irq_data->type_rising_mask);
>> +		break;
> This should handle the case where an output type is not supported, for
> example if the device only supports falling edge.  It'd also seem
> sensible to have an explicit value to set for each trigger type rather
> than reyling on the device using separate bits for everything - you
> might see an encoding like 0 for falling, 1 for rising, 2 for both and 3
> for level for example.

is this mean we should have variable like
.supported_iflags which capture what it is supported or not.
And then irq_type_val[MAX_REGMAP_IRQ_TYPE] as array and define the 
REGMAP_IRQ_TYPE_[FALLING/RISING/BOTH] as index and client can fill this 
info as per the hardware or

Or we should have separate variable for each type like:
int irq_type_rising_val;
int irq_type_faling_val;
int ire_type_both_val;

along with
int irq_type_supported;
int irq_type_mask_val;







^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-13 14:20   ` Mark Brown
@ 2013-02-14 11:06     ` Laxman Dewangan
  2013-02-14 11:35       ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Laxman Dewangan @ 2013-02-14 11:06 UTC (permalink / raw)
  To: Mark Brown; +Cc: gregkh, linux-kernel

On Wednesday 13 February 2013 07:50 PM, Mark Brown wrote:
> *
>
>>   	for (i = 0; i < d->chip->num_regs; i++) {
>> +		if (!d->chip->mask_base)
>> +			goto skip_mask_reg_update;
>> +
> Why is this inside the loop?
>
> I'd also expect us to return an error if a caller tries to enable or
> disable an interrupt, or possibly to give different ops to the IRQ
> subsystem, rather than just silently claim we did what we were asked.

I tried to use regmap-irq for the gpio submodule and it has two sets of 
register:
GPIOx_CNFG: bit[7:6] interrupt rising/falling.
GPIO_INT_STS where each bit shows the interrupt status whether it 
occured or not.
There is no mask register.

In regmap-irq_thread() we see the interrupt status and compare against 
mask enable buffer wther this is enabled or not and accordingly call the 
handler.

hence I am still require irq_mask()/irq_unmask() to reflect the mask 
with interrupt status and type for actually configuring the GPIOx_CNFG.

if I remove the mask_buf at all then how do we tell the int_sts register 
is corresponding to which gpio handler?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] regmap: irq: Add support for interrupt type
  2013-02-14 11:01   ` Laxman Dewangan
@ 2013-02-14 11:06     ` Mark Brown
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2013-02-14 11:06 UTC (permalink / raw)
  To: Laxman Dewangan; +Cc: gregkh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

On Thu, Feb 14, 2013 at 04:31:50PM +0530, Laxman Dewangan wrote:

> is this mean we should have variable like
> .supported_iflags which capture what it is supported or not.
> And then irq_type_val[MAX_REGMAP_IRQ_TYPE] as array and define the
> REGMAP_IRQ_TYPE_[FALLING/RISING/BOTH] as index and client can fill
> this info as per the hardware or

I think this option is best; it's going to avoid us having to write
explicit cases for every single use case.  But perhaps it's going to be
too painful to work with.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-14 11:06     ` Laxman Dewangan
@ 2013-02-14 11:35       ` Mark Brown
  2013-02-14 11:55         ` Laxman Dewangan
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2013-02-14 11:35 UTC (permalink / raw)
  To: Laxman Dewangan; +Cc: gregkh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On Thu, Feb 14, 2013 at 04:36:08PM +0530, Laxman Dewangan wrote:
> On Wednesday 13 February 2013 07:50 PM, Mark Brown wrote:

> >>  	for (i = 0; i < d->chip->num_regs; i++) {
> >>+		if (!d->chip->mask_base)
> >>+			goto skip_mask_reg_update;

> >Why is this inside the loop?

You appear to have ignored this question.

> >I'd also expect us to return an error if a caller tries to enable or
> >disable an interrupt, or possibly to give different ops to the IRQ
> >subsystem, rather than just silently claim we did what we were asked.

> I tried to use regmap-irq for the gpio submodule and it has two sets
> of register:
> GPIOx_CNFG: bit[7:6] interrupt rising/falling.
> GPIO_INT_STS where each bit shows the interrupt status whether it
> occured or not.
> There is no mask register.

> In regmap-irq_thread() we see the interrupt status and compare
> against mask enable buffer wther this is enabled or not and
> accordingly call the handler.

> hence I am still require irq_mask()/irq_unmask() to reflect the mask
> with interrupt status and type for actually configuring the
> GPIOx_CNFG.

> if I remove the mask_buf at all then how do we tell the int_sts
> register is corresponding to which gpio handler?

This doesn't sound like something that should be open coded in
individual interrupt controller drivers, obviously it's a bit rubbish
that there's no way to enable or disable the interrupt but presumably
other hardware has the same "feature" and the IRQ subsystem ought to
understand it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-14 11:35       ` Mark Brown
@ 2013-02-14 11:55         ` Laxman Dewangan
  2013-02-14 11:57           ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Laxman Dewangan @ 2013-02-14 11:55 UTC (permalink / raw)
  To: Mark Brown; +Cc: gregkh, linux-kernel

On Thursday 14 February 2013 05:05 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Thu, Feb 14, 2013 at 04:36:08PM +0530, Laxman Dewangan wrote:
>> On Wednesday 13 February 2013 07:50 PM, Mark Brown wrote:
>>>>   	for (i = 0; i < d->chip->num_regs; i++) {
>>>> +		if (!d->chip->mask_base)
>>>> +			goto skip_mask_reg_update;
>>> Why is this inside the loop?
> You appear to have ignored this question.

I have accepted this to put out of loop.
Originally  thought that Inside loop, there is two register update one 
is mask and other is wakup. If I ignore the loop for mask_base= 0 then 
probably wake_ register will not get updated and hence it is inside the 
loop to update the wake register. If there is wake_base is 0 then this 
register update will be ignored anyhow.



>
>>> I'd also expect us to return an error if a caller tries to enable or
>>> disable an interrupt, or possibly to give different ops to the IRQ
>>> subsystem, rather than just silently claim we did what we were asked.
> if I remove the mask_buf at all then how do we tell the int_sts
> register is corresponding to which gpio handler?
> This doesn't sound like something that should be open coded in
> individual interrupt controller drivers, obviously it's a bit rubbish
> that there's no way to enable or disable the interrupt but presumably
> other hardware has the same "feature" and the IRQ subsystem ought to
> understand it.
>

To support such case, can we assume that mask is always enabled 
(interrupt enabled) so that it can be use in irq_thread to mask the 
interrupt status. So during initialization, if there is no mask_base 
register then all mask_buf is such that it enabled interrupt.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-14 11:55         ` Laxman Dewangan
@ 2013-02-14 11:57           ` Mark Brown
  2013-02-14 12:16             ` Laxman Dewangan
  0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2013-02-14 11:57 UTC (permalink / raw)
  To: Laxman Dewangan; +Cc: gregkh, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Thu, Feb 14, 2013 at 05:25:13PM +0530, Laxman Dewangan wrote:
> On Thursday 14 February 2013 05:05 PM, Mark Brown wrote:

> >This doesn't sound like something that should be open coded in
> >individual interrupt controller drivers, obviously it's a bit rubbish
> >that there's no way to enable or disable the interrupt but presumably
> >other hardware has the same "feature" and the IRQ subsystem ought to
> >understand it.

> To support such case, can we assume that mask is always enabled
> (interrupt enabled) so that it can be use in irq_thread to mask the
> interrupt status. So during initialization, if there is no mask_base
> register then all mask_buf is such that it enabled interrupt.

...and have any attempt to mask the interrupt return an error?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] regmap: irq: do not write mask register if it is not supported
  2013-02-14 11:57           ` Mark Brown
@ 2013-02-14 12:16             ` Laxman Dewangan
  0 siblings, 0 replies; 11+ messages in thread
From: Laxman Dewangan @ 2013-02-14 12:16 UTC (permalink / raw)
  To: Mark Brown; +Cc: gregkh, linux-kernel

On Thursday 14 February 2013 05:27 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Thu, Feb 14, 2013 at 05:25:13PM +0530, Laxman Dewangan wrote:
>> On Thursday 14 February 2013 05:05 PM, Mark Brown wrote:
>>> This doesn't sound like something that should be open coded in
>>> individual interrupt controller drivers, obviously it's a bit rubbish
>>> that there's no way to enable or disable the interrupt but presumably
>>> other hardware has the same "feature" and the IRQ subsystem ought to
>>> understand it.
>> To support such case, can we assume that mask is always enabled
>> (interrupt enabled) so that it can be use in irq_thread to mask the
>> interrupt status. So during initialization, if there is no mask_base
>> register then all mask_buf is such that it enabled interrupt.
> ...and have any attempt to mask the interrupt return an error?
>

Yes, we can return error as -EINVAL.

I just looked kernel/irq/chip.c, enable_irq()/disable_irq()  do not 
check for return value but it does not matter as per regmap-irq 
implementation.

void irq_enable(struct irq_desc *desc)
{
         irq_state_clr_disabled(desc);
         if (desc->irq_data.chip->irq_enable)
desc->irq_data.chip->irq_enable(&desc->irq_data);
         else
desc->irq_data.chip->irq_unmask(&desc->irq_data);
         irq_state_clr_masked(desc);
}


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-02-14 12:16 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-13 13:14 [PATCH 1/2] regmap: irq: Add support for interrupt type Laxman Dewangan
2013-02-13 13:14 ` [PATCH 2/2] regmap: irq: do not write mask register if it is not supported Laxman Dewangan
2013-02-13 14:20   ` Mark Brown
2013-02-14 11:06     ` Laxman Dewangan
2013-02-14 11:35       ` Mark Brown
2013-02-14 11:55         ` Laxman Dewangan
2013-02-14 11:57           ` Mark Brown
2013-02-14 12:16             ` Laxman Dewangan
2013-02-13 13:54 ` [PATCH 1/2] regmap: irq: Add support for interrupt type Mark Brown
2013-02-14 11:01   ` Laxman Dewangan
2013-02-14 11:06     ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).