[PATCH 2/4] mfd: update irq handler in max8925

[PATCH 2/4] mfd: update irq handler in max8925

[Haojian Zhuang]

[PATCH 2/4] mfd: update irq handler in max8925

[Mark Brown]

On Mon, Jan 25, 2010 at 06:08:19AM -0500, Haojian Zhuang wrote:

> +static struct resource power_supply_resources[] = {
> +	{
> +		.name	= "max8925-power",
> +		.start	= MAX8925_CHG_IRQ1,
> +		.end	= MAX8925_CHG_IRQ1,
> +		.flags	= IORESOURCE_IO,

These should be IORESOURCE_IRQ.

> +		else if (value & irq_data->offs) {
> +			dev_err(chip->dev, "Noboday cares IRQ #%d. Mask it\n",
> +				chip->irq_base + i);
> +			max8925_set_bits(i2c, irq_data->mask_reg,
> +					irq_data->offs, irq_data->offs);

genirq ought to be handling this for you?

> +	for (i = 0; i < ARRAY_SIZE(max8925_irqs); i++) {
> +		irq_data = &max8925_irqs[i];
> +		/* non TSC IRQ should be serviced in max8925_irq() */
> +		if (!irq_data->tsc_irq)
> +			continue;

Shouldn't the interrupt handlers be able to avoid having to iterate
through the array?  It's unlikely to be expensive in the context of the
I/O but it looks a bit odd.

> + * clear_irq: operation on clearing status of nIRQ pin in platform
> + * clear_tsc_irq: operation on clearing status of nTIRQ pin in platform
> + */

Shouldn't these also be handled by genirq as functions of the interrupt
controller that the chip is chained from?

[PATCH 2/4] mfd: update irq handler in max8925

[Haojian Zhuang]

On Mon, Jan 25, 2010 at 6:59 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 25, 2010 at 06:08:19AM -0500, Haojian Zhuang wrote:
>
>> +static struct resource power_supply_resources[] = {
>> + ? ? {
>> + ? ? ? ? ? ? .name ? = "max8925-power",
>> + ? ? ? ? ? ? .start ?= MAX8925_CHG_IRQ1,
>> + ? ? ? ? ? ? .end ? ?= MAX8925_CHG_IRQ1,
>> + ? ? ? ? ? ? .flags ?= IORESOURCE_IO,
>
> These should be IORESOURCE_IRQ.
>
Now I changed .start and .end. I only need declare IO resources.

>> + ? ? ? ? ? ? else if (value & irq_data->offs) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(chip->dev, "Noboday cares IRQ #%d. Mask it\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? chip->irq_base + i);
>> + ? ? ? ? ? ? ? ? ? ? max8925_set_bits(i2c, irq_data->mask_reg,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_data->offs, irq_data->offs);
>
> genirq ought to be handling this for you?
>
No, genirq doesn't know this. I let the thread_fn to take this job.

>> + ? ? for (i = 0; i < ARRAY_SIZE(max8925_irqs); i++) {
>> + ? ? ? ? ? ? irq_data = &max8925_irqs[i];
>> + ? ? ? ? ? ? /* non TSC IRQ should be serviced in max8925_irq() */
>> + ? ? ? ? ? ? if (!irq_data->tsc_irq)
>> + ? ? ? ? ? ? ? ? ? ? continue;
>
> Shouldn't the interrupt handlers be able to avoid having to iterate
> through the array? ?It's unlikely to be expensive in the context of the
> I/O but it looks a bit odd.
>
There're multiple IRQ & IRQ_MASK registers. If I use bitmap at here, I
would use more code on composing it. Since some bits are not defined
in IRQ, I need to take care on it. If I use array at here, the code is
clean and simple. Although it looks a bit odd.

>> + * clear_irq: operation on clearing status of nIRQ pin in platform
>> + * clear_tsc_irq: operation on clearing status of nTIRQ pin in platform
>> + */
>
> Shouldn't these also be handled by genirq as functions of the interrupt
> controller that the chip is chained from?
>
Because PMIC IRQ is connected to the special pin of CPU. CPU needs to
clear pin status in external. genirq shouldn't take care on it. I have
to use this ugly way to clear pin status.

Thanks
Haojian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mfd-update-irq-handler-in-max8925.patch
Type: text/x-patch
Size: 29093 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100125/98120427/attachment-0001.bin>

[PATCH 2/4] mfd: update irq handler in max8925

[Haojian Zhuang]

On Mon, Jan 25, 2010 at 10:12 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Mon, Jan 25, 2010 at 6:59 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Mon, Jan 25, 2010 at 06:08:19AM -0500, Haojian Zhuang wrote:
>>
>>> +static struct resource power_supply_resources[] = {
>>> + ? ? {
>>> + ? ? ? ? ? ? .name ? = "max8925-power",
>>> + ? ? ? ? ? ? .start ?= MAX8925_CHG_IRQ1,
>>> + ? ? ? ? ? ? .end ? ?= MAX8925_CHG_IRQ1,
>>> + ? ? ? ? ? ? .flags ?= IORESOURCE_IO,
>>
>> These should be IORESOURCE_IRQ.
>>
> Now I changed .start and .end. I only need declare IO resources.
>
>>> + ? ? ? ? ? ? else if (value & irq_data->offs) {
>>> + ? ? ? ? ? ? ? ? ? ? dev_err(chip->dev, "Noboday cares IRQ #%d. Mask it\n",
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? chip->irq_base + i);
>>> + ? ? ? ? ? ? ? ? ? ? max8925_set_bits(i2c, irq_data->mask_reg,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_data->offs, irq_data->offs);
>>
>> genirq ought to be handling this for you?
>>
> No, genirq doesn't know this. I let the thread_fn to take this job.
>
>>> + ? ? for (i = 0; i < ARRAY_SIZE(max8925_irqs); i++) {
>>> + ? ? ? ? ? ? irq_data = &max8925_irqs[i];
>>> + ? ? ? ? ? ? /* non TSC IRQ should be serviced in max8925_irq() */
>>> + ? ? ? ? ? ? if (!irq_data->tsc_irq)
>>> + ? ? ? ? ? ? ? ? ? ? continue;
>>
>> Shouldn't the interrupt handlers be able to avoid having to iterate
>> through the array? ?It's unlikely to be expensive in the context of the
>> I/O but it looks a bit odd.
>>
> There're multiple IRQ & IRQ_MASK registers. If I use bitmap at here, I
> would use more code on composing it. Since some bits are not defined
> in IRQ, I need to take care on it. If I use array at here, the code is
> clean and simple. Although it looks a bit odd.
>
>>> + * clear_irq: operation on clearing status of nIRQ pin in platform
>>> + * clear_tsc_irq: operation on clearing status of nTIRQ pin in platform
>>> + */
>>
>> Shouldn't these also be handled by genirq as functions of the interrupt
>> controller that the chip is chained from?
>>
> Because PMIC IRQ is connected to the special pin of CPU. CPU needs to
> clear pin status in external. genirq shouldn't take care on it. I have
> to use this ugly way to clear pin status.
>
> Thanks
> Haojian
>

Update the patch now.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mfd-update-irq-handler-in-max8925.patch
Type: text/x-patch
Size: 29103 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100125/31cf6460/attachment-0001.bin>

[PATCH 2/4] mfd: update irq handler in max8925

[Mark Brown]

On Mon, Jan 25, 2010 at 10:12:39PM -0500, Haojian Zhuang wrote:
> On Mon, Jan 25, 2010 at 6:59 AM, Mark Brown
> > On Mon, Jan 25, 2010 at 06:08:19AM -0500, Haojian Zhuang wrote:

> >> + ? ? ? ? ? ? else if (value & irq_data->offs) {
> >> + ? ? ? ? ? ? ? ? ? ? dev_err(chip->dev, "Noboday cares IRQ #%d. Mask it\n",
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? chip->irq_base + i);
> >> + ? ? ? ? ? ? ? ? ? ? max8925_set_bits(i2c, irq_data->mask_reg,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_data->offs, irq_data->offs);

> > genirq ought to be handling this for you?

> No, genirq doesn't know this. I let the thread_fn to take this job.

This suggests that something is wrong somewhere else - genirq will
normally warn if an interrupt is reported without being handled which
looks like what this code is trying to achieve.  Looking at the patch
I expect you should just be able to drop the test you're doing
immediately before the quoted code to see if the interrupt is enabled
and call handle_nested_irq() unconditionally, genirq should generate a
warning and mask the IRQ if it's not handled which is what your code
here is doing.

> >> + * clear_irq: operation on clearing status of nIRQ pin in platform
> >> + * clear_tsc_irq: operation on clearing status of nTIRQ pin in platform
> >> + */

> > Shouldn't these also be handled by genirq as functions of the interrupt
> > controller that the chip is chained from?

> Because PMIC IRQ is connected to the special pin of CPU. CPU needs to
> clear pin status in external. genirq shouldn't take care on it. I have
> to use this ugly way to clear pin status.

I would expect this to be handled by the special pin on the CPU being an
irq_chip, or by having the existing interrupt controller handle this pin
properly.  The need to ack is going to be there for anything that ends
up hanging off this special IRQ, isn't it?

[PATCH 2/4] mfd: update irq handler in max8925

[Haojian Zhuang]

On Tue, Jan 26, 2010 at 6:28 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 25, 2010 at 10:12:39PM -0500, Haojian Zhuang wrote:
>> On Mon, Jan 25, 2010 at 6:59 AM, Mark Brown
>> > On Mon, Jan 25, 2010 at 06:08:19AM -0500, Haojian Zhuang wrote:
>
>> >> + ? ? ? ? ? ? else if (value & irq_data->offs) {
>> >> + ? ? ? ? ? ? ? ? ? ? dev_err(chip->dev, "Noboday cares IRQ #%d. Mask it\n",
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? chip->irq_base + i);
>> >> + ? ? ? ? ? ? ? ? ? ? max8925_set_bits(i2c, irq_data->mask_reg,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? irq_data->offs, irq_data->offs);
>
>> > genirq ought to be handling this for you?
>
>> No, genirq doesn't know this. I let the thread_fn to take this job.
>
> This suggests that something is wrong somewhere else - genirq will
> normally warn if an interrupt is reported without being handled which
> looks like what this code is trying to achieve. ?Looking at the patch
> I expect you should just be able to drop the test you're doing
> immediately before the quoted code to see if the interrupt is enabled
> and call handle_nested_irq() unconditionally, genirq should generate a
> warning and mask the IRQ if it's not handled which is what your code
> here is doing.
>
Actually I can't find genirq reports any warning. I only find my code
reports warning message.

>> >> + * clear_irq: operation on clearing status of nIRQ pin in platform
>> >> + * clear_tsc_irq: operation on clearing status of nTIRQ pin in platform
>> >> + */
>
>> > Shouldn't these also be handled by genirq as functions of the interrupt
>> > controller that the chip is chained from?
>
>> Because PMIC IRQ is connected to the special pin of CPU. CPU needs to
>> clear pin status in external. genirq shouldn't take care on it. I have
>> to use this ugly way to clear pin status.
>
> I would expect this to be handled by the special pin on the CPU being an
> irq_chip, or by having the existing interrupt controller handle this pin
> properly. ?The need to ack is going to be there for anything that ends
> up hanging off this special IRQ, isn't it?
>
Em. it's OK. I'll do it.

[PATCH 2/4] mfd: update irq handler in max8925

[Mark Brown]

On Tue, Jan 26, 2010 at 06:31:56AM -0500, Haojian Zhuang wrote:
> On Tue, Jan 26, 2010 at 6:28 AM, Mark Brown

> > and call handle_nested_irq() unconditionally, genirq should generate a
> > warning and mask the IRQ if it's not handled which is what your code
> > here is doing.

> Actually I can't find genirq reports any warning. I only find my code
> reports warning message.

There's a threashold before the warning starts displaying, IIRC.  In any
case, the handling of spurious IRQs isn't really an IRQ controller
driver issue - it's a generic issue that affects all interrupt
controllers.

[PATCH 2/4] mfd: update irq handler in max8925

[Haojian Zhuang]

On Tue, Jan 26, 2010 at 7:06 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jan 26, 2010 at 06:31:56AM -0500, Haojian Zhuang wrote:
>> On Tue, Jan 26, 2010 at 6:28 AM, Mark Brown
>
>> > and call handle_nested_irq() unconditionally, genirq should generate a
>> > warning and mask the IRQ if it's not handled which is what your code
>> > here is doing.
>
>> Actually I can't find genirq reports any warning. I only find my code
>> reports warning message.
>
> There's a threashold before the warning starts displaying, IIRC. ?In any
> case, the handling of spurious IRQs isn't really an IRQ controller
> driver issue - it's a generic issue that affects all interrupt
> controllers.
>

Thanks a lot.

Updated patch is attached in mail. Now the modification is in below.
1. remove clear_irq() & clear_tsc_irq().
2. remove warning message in max8925_irq().

Best Regards
Haojian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-mfd-update-irq-handler-in-max8925.patch
Type: text/x-patch
Size: 28146 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100127/645e8444/attachment-0001.bin>

[PATCH 2/4] mfd: update irq handler in max8925

[Mark Brown]

On Wed, Jan 27, 2010 at 01:02:09AM -0500, Haojian Zhuang wrote:

> Updated patch is attached in mail. Now the modification is in below.
> 1. remove clear_irq() & clear_tsc_irq().
> 2. remove warning message in max8925_irq().

This looks OK for me - for what it's worth

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>