Some drivers need to know what the status of the interrupt line is.
This is especially true for drivers that register a handler with
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and in the handler they
need to know which edge transition it was invoked for.
The irq_read_line callback in the chip allows the controller to provide
the real time status of this line. Controllers that can read the status
of an interrupt line should implement this by doing necessary
hardware reads and return the logical state of the line.
Interrupt controllers based on the slow bus architecture should conduct
the transaction in this callback. The genirq code will call the chip's
bus lock prior to calling irq_read_line. Obviously since the transaction
would be completed before returning from irq_read_line it need not do
any transactions in the bus unlock call.
Drivers need to be aware whether the interrupt controller is a slow bus
and call read_irq_line in proper context.
Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
---
include/linux/interrupt.h | 1 +
include/linux/irq.h | 2 ++
kernel/irq/manage.c | 25 +++++++++++++++++++++++++
3 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 59b72ca..0722576 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -337,6 +337,7 @@ static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long
/* IRQ wakeup (PM) control: */
extern int irq_set_irq_wake(unsigned int irq, unsigned int on);
+extern int irq_read_line(unsigned int irq);
#ifndef CONFIG_GENERIC_HARDIRQS_NO_COMPAT
/* Please do not use: Use the replacement functions instead */
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 2a375a7..52198b6 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -298,6 +298,7 @@ static inline void irqd_clr_chained_irq_inprogress(struct irq_data *d)
* @irq_retrigger: resend an IRQ to the CPU
* @irq_set_type: set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
* @irq_set_wake: enable/disable power-management wake-on of an IRQ
+ * @irq_read_line: return the current value on the irq line
* @irq_bus_lock: function to lock access to slow bus (i2c) chips
* @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips
* @irq_cpu_online: configure an interrupt source for a secondary CPU
@@ -324,6 +325,7 @@ struct irq_chip {
int (*irq_retrigger)(struct irq_data *data);
int (*irq_set_type)(struct irq_data *data, unsigned int flow_type);
int (*irq_set_wake)(struct irq_data *data, unsigned int on);
+ int (*irq_read_line)(struct irq_data *data);
void (*irq_bus_lock)(struct irq_data *data);
void (*irq_bus_sync_unlock)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 12a80fd..6667052 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -518,6 +518,31 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
}
EXPORT_SYMBOL(irq_set_irq_wake);
+/**
+ * irq_read_line - read the value on an irq line
+ * @irq: Interrupt number representing a hardware line
+ *
+ * This function may be called from IRQ context only when
+ * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
+ */
+int irq_read_line(unsigned int irq)
+{
+ struct irq_desc *desc = irq_to_desc(irq);
+ unsigned long flags;
+ int val;
+
+ if (!desc || !desc->irq_data.chip->irq_read_line)
+ return -EINVAL;
+
+ chip_bus_lock(desc);
+ raw_spin_lock_irqsave(&desc->lock, flags);
+ val = desc->irq_data.chip->irq_read_line(&desc->irq_data);
+ raw_spin_unlock_irqrestore(&desc->lock, flags);
+ chip_bus_sync_unlock(desc);
+ return val;
+}
+EXPORT_SYMBOL(irq_read_line);
+
/*
* Internal function that tells the architecture code whether a
* particular irq has been exclusively allocated or is available
--
1.7.1
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
* Abhijeet Dharmapurikar ([email protected]) wrote:
> Some drivers need to know what the status of the interrupt line is.
> This is especially true for drivers that register a handler with
> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and in the handler they
> need to know which edge transition it was invoked for.
>
> The irq_read_line callback in the chip allows the controller to provide
> the real time status of this line. Controllers that can read the status
> of an interrupt line should implement this by doing necessary
> hardware reads and return the logical state of the line.
>
> Interrupt controllers based on the slow bus architecture should conduct
> the transaction in this callback. The genirq code will call the chip's
> bus lock prior to calling irq_read_line. Obviously since the transaction
> would be completed before returning from irq_read_line it need not do
> any transactions in the bus unlock call.
>
> Drivers need to be aware whether the interrupt controller is a slow bus
> and call read_irq_line in proper context.
Hrm, this strikes me as odd. Is there any way this generic API could
handle the corner-cases without needed the caller to know this ?
[...]
> +/**
> + * irq_read_line - read the value on an irq line
> + * @irq: Interrupt number representing a hardware line
> + *
> + * This function may be called from IRQ context only when
> + * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
The comment here seems to imply "be careful when using this extremely
fragile interface", which does not give me the warm safety feeling I
would come to expect from a generic kernel API.
Any ideas on how to improve that ?
Thanks,
Mathieu
> + */
> +int irq_read_line(unsigned int irq)
> +{
> + struct irq_desc *desc = irq_to_desc(irq);
> + unsigned long flags;
> + int val;
> +
> + if (!desc || !desc->irq_data.chip->irq_read_line)
> + return -EINVAL;
> +
> + chip_bus_lock(desc);
> + raw_spin_lock_irqsave(&desc->lock, flags);
> + val = desc->irq_data.chip->irq_read_line(&desc->irq_data);
> + raw_spin_unlock_irqrestore(&desc->lock, flags);
> + chip_bus_sync_unlock(desc);
> + return val;
> +}
> +EXPORT_SYMBOL(irq_read_line);
> +
> /*
> * Internal function that tells the architecture code whether a
> * particular irq has been exclusively allocated or is available
> --
> 1.7.1
>
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
On Fri, 15 Apr 2011, Abhijeet Dharmapurikar wrote:
> Some drivers need to know what the status of the interrupt line is.
> This is especially true for drivers that register a handler with
> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and in the handler they
> need to know which edge transition it was invoked for.
What's the purpose of this? How is that going to be used?
> The irq_read_line callback in the chip allows the controller to provide
> the real time status of this line. Controllers that can read the status
> of an interrupt line should implement this by doing necessary
> hardware reads and return the logical state of the line.
>
> Interrupt controllers based on the slow bus architecture should conduct
> the transaction in this callback. The genirq code will call the chip's
> bus lock prior to calling irq_read_line. Obviously since the transaction
> would be completed before returning from irq_read_line it need not do
> any transactions in the bus unlock call.
>
> Drivers need to be aware whether the interrupt controller is a slow bus
> and call read_irq_line in proper context.
>
> Signed-off-by: Abhijeet Dharmapurikar <[email protected]>
> ---
> include/linux/interrupt.h | 1 +
> include/linux/irq.h | 2 ++
> kernel/irq/manage.c | 25 +++++++++++++++++++++++++
> 3 files changed, 28 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 59b72ca..0722576 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -337,6 +337,7 @@ static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long
>
> /* IRQ wakeup (PM) control: */
> extern int irq_set_irq_wake(unsigned int irq, unsigned int on);
> +extern int irq_read_line(unsigned int irq);
>
> #ifndef CONFIG_GENERIC_HARDIRQS_NO_COMPAT
That hunk does not apply.
> /* Please do not use: Use the replacement functions instead */
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 2a375a7..52198b6 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -298,6 +298,7 @@ static inline void irqd_clr_chained_irq_inprogress(struct irq_data *d)
> * @irq_retrigger: resend an IRQ to the CPU
> * @irq_set_type: set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
> * @irq_set_wake: enable/disable power-management wake-on of an IRQ
> + * @irq_read_line: return the current value on the irq line
> * @irq_bus_lock: function to lock access to slow bus (i2c) chips
> * @irq_bus_sync_unlock:function to sync and unlock slow bus (i2c) chips
> * @irq_cpu_online: configure an interrupt source for a secondary CPU
> @@ -324,6 +325,7 @@ struct irq_chip {
> int (*irq_retrigger)(struct irq_data *data);
> int (*irq_set_type)(struct irq_data *data, unsigned int flow_type);
> int (*irq_set_wake)(struct irq_data *data, unsigned int on);
> + int (*irq_read_line)(struct irq_data *data);
>
> void (*irq_bus_lock)(struct irq_data *data);
> void (*irq_bus_sync_unlock)(struct irq_data *data);
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 12a80fd..6667052 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -518,6 +518,31 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
> }
> EXPORT_SYMBOL(irq_set_irq_wake);
>
> +/**
> + * irq_read_line - read the value on an irq line
> + * @irq: Interrupt number representing a hardware line
> + *
> + * This function may be called from IRQ context only when
> + * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
> + */
> +int irq_read_line(unsigned int irq)
> +{
> + struct irq_desc *desc = irq_to_desc(irq);
> + unsigned long flags;
> + int val;
Please use irq_get_desc_buslock()
> + if (!desc || !desc->irq_data.chip->irq_read_line)
> + return -EINVAL;
> +
> + chip_bus_lock(desc);
> + raw_spin_lock_irqsave(&desc->lock, flags);
> + val = desc->irq_data.chip->irq_read_line(&desc->irq_data);
> + raw_spin_unlock_irqrestore(&desc->lock, flags);
> + chip_bus_sync_unlock(desc);
> + return val;
> +}
> +EXPORT_SYMBOL(irq_read_line);
EXPORT_SYMBOL_GPL() please.
Thanks,
tglx
On 04/15/2011 11:42 AM, Abhijeet Dharmapurikar wrote:
[...]
>
> +/**
> + * irq_read_line - read the value on an irq line
> + * @irq: Interrupt number representing a hardware line
> + *
> + * This function may be called from IRQ context only when
> + * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
What does it return? I can see that -EINVAL is returned if it isn't
applicable to this irq. But what if chip->irq_read_line is implemented?
> + */
> +int irq_read_line(unsigned int irq)
> +{
> + struct irq_desc *desc = irq_to_desc(irq);
> + unsigned long flags;
> + int val;
> +
> + if (!desc || !desc->irq_data.chip->irq_read_line)
> + return -EINVAL;
> +
> + chip_bus_lock(desc);
> + raw_spin_lock_irqsave(&desc->lock, flags);
> + val = desc->irq_data.chip->irq_read_line(&desc->irq_data);
> + raw_spin_unlock_irqrestore(&desc->lock, flags);
> + chip_bus_sync_unlock(desc);
> + return val;
> +}
> +EXPORT_SYMBOL(irq_read_line);
> +
> /*
> * Internal function that tells the architecture code whether a
> * particular irq has been exclusively allocated or is available
Mathieu Desnoyers wrote:
> * Abhijeet Dharmapurikar ([email protected]) wrote:
>> Drivers need to be aware whether the interrupt controller is a slow bus
>> and call read_irq_line in proper context.
>
> Hrm, this strikes me as odd. Is there any way this generic API could
> handle the corner-cases without needed the caller to know this ?
Usually, a slow bus irq controller marks its irq as nested and has its
irq handler running in thread context. In that case it will be safe to
call irq_read_line from thread context. For the usual (non slow bus) irq
controller we can always call irq_read_line within irq context.
This function is intended to be called from within the handler, to know
if a rising edge or a falling edge caused the interrupt. It can be used
in other places too, but then the driver needs to be careful about the
interrupt controller slowbus nature.
The problem happens when this function is called from interrupt context
when irq controller is slowbus. For example one has taken a spin_lock
with interrupts disabled and now it wants to know the real time status
on a pin that is controlled by a slow bus controller. The slowbus
controller could end up sleeping and things go bad.
I should explain this more in the commit text. Will do so in the next
revision of this patch.
>> + * This function may be called from IRQ context only when
>> + * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
>
> The comment here seems to imply "be careful when using this extremely
> fragile interface", which does not give me the warm safety feeling I
> would come to expect from a generic kernel API.
>
> Any ideas on how to improve that ?
Well, even the enable_irq() and disable_irq_nosync() have the same
calling restrictions. I picked this line from the comment block in
enable_irq() function. You think I should explain it more in the
upcoming patch?
--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm
Innovation Center, Inc. is a member of the Code Aurora Forum.
Thomas Gleixner wrote:
> On Fri, 15 Apr 2011, Abhijeet Dharmapurikar wrote:
>
>> Some drivers need to know what the status of the interrupt line is.
>> This is especially true for drivers that register a handler with
>> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and in the handler they
>> need to know which edge transition it was invoked for.
>
> What's the purpose of this? How is that going to be used?
The main purpose is to know in the handler if it was invoked becuase of
a rising edge or the falling edge.
Now one could say that the driver should maintain some state and toggle
it upon each invocation of this handler. That scheme quickly goes out of
sync because we might ignore interrupts while suspended.
>
>>
>> #ifndef CONFIG_GENERIC_HARDIRQS_NO_COMPAT
>
> That hunk does not apply.
Will rebase and resend it again, my bad.
>> +int irq_read_line(unsigned int irq)
>> +{
>> + struct irq_desc *desc = irq_to_desc(irq);
>> + unsigned long flags;
>> + int val;
>
> Please use irq_get_desc_buslock()
yes will fix this in the next revision of this patch.
>> +EXPORT_SYMBOL(irq_read_line);
>
> EXPORT_SYMBOL_GPL() please.
yes will do
--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm
Innovation Center, Inc. is a member of the Code Aurora Forum.
David Daney wrote:
> On 04/15/2011 11:42 AM, Abhijeet Dharmapurikar wrote:
> [...]
>>
>> +/**
>> + * irq_read_line - read the value on an irq line
>> + * @irq: Interrupt number representing a hardware line
>> + *
>> + * This function may be called from IRQ context only when
>> + * desc->chip->bus_lock and desc->chip->bus_sync_unlock are NULL !
>
> What does it return? I can see that -EINVAL is returned if it isn't
> applicable to this irq. But what if chip->irq_read_line is implemented?
It returns the logical state of that line (0 when it is low or 1 when it
is high). Will add the return type in the comment.
>
>
>
>
>> + */
>> +int irq_read_line(unsigned int irq)
>> +{
>> + struct irq_desc *desc = irq_to_desc(irq);
>> + unsigned long flags;
>> + int val;
>> +
>> + if (!desc || !desc->irq_data.chip->irq_read_line)
>> + return -EINVAL;
It returns -EINVAL is irq_read_line is not implemented.
>> +
>> + chip_bus_lock(desc);
>> + raw_spin_lock_irqsave(&desc->lock, flags);
--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm
Innovation Center, Inc. is a member of the Code Aurora Forum.
On Fri, 15 Apr 2011, Abhijeet Dharmapurikar wrote:
> Thomas Gleixner wrote:
> > On Fri, 15 Apr 2011, Abhijeet Dharmapurikar wrote:
> >
> > > Some drivers need to know what the status of the interrupt line is.
> > > This is especially true for drivers that register a handler with
> > > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and in the handler they
> > > need to know which edge transition it was invoked for.
> >
> > What's the purpose of this? How is that going to be used?
>
> The main purpose is to know in the handler if it was invoked becuase of a
> rising edge or the falling edge.
Come on. That's not an explanation. I know that already and it does
not answer my question how this is going to be used. IOW: What does
the driver do aside of knowing that it was a rising falling edge?
> Now one could say that the driver should maintain some state and toggle it
> upon each invocation of this handler. That scheme quickly goes out of sync
> because we might ignore interrupts while suspended.
Suspend is a totaly different issue. I have the impression that you
are trying to solve some basic issue at the driver level again - just
this time you add some core helper to get it solved.
Can you please show _AND_ explain the code which is going to use this?
Thanks,
tglx
Thomas Gleixner wrote:
> On Fri, 15 Apr 2011, Abhijeet Dharmapurikar wrote:
>
>> Thomas Gleixner wrote:
>>> On Fri, 15 Apr 2011, Abhijeet Dharmapurikar wrote:
>>>
>>>> Some drivers need to know what the status of the interrupt line is.
>>>> This is especially true for drivers that register a handler with
>>>> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and in the handler they
>>>> need to know which edge transition it was invoked for.
>>> What's the purpose of this? How is that going to be used?
>> The main purpose is to know in the handler if it was invoked becuase of a
>> rising edge or the falling edge.
>
> Come on. That's not an explanation. I know that already and it does
> not answer my question how this is going to be used. IOW: What does
> the driver do aside of knowing that it was a rising falling edge?
>
>> Now one could say that the driver should maintain some state and toggle it
>> upon each invocation of this handler. That scheme quickly goes out of sync
>> because we might ignore interrupts while suspended.
>
> Suspend is a totaly different issue. I have the impression that you
> are trying to solve some basic issue at the driver level again - just
> this time you add some core helper to get it solved.
>
> Can you please show _AND_ explain the code which is going to use this?
Sorry, misunderstood your question. Dont have code ready yet but here
are few examples which I need to address in the near future
The USB_ID pin is an interrupt to the pmic. This pin is used to detect
if a peripheral or a host is connected. If a peripheral is connected the
ID pin is driven low (for the host case it remains floating) and when
it is removed a pull up draws it high. We want to detect in the handler
whether it was invoked when the cable was plugged in or when cable was
plugged out. That helps the handler decide to turn on/off a regulator
which provides power to the peripheral.
A similar scheme applies to VBUS line of the USB too. If it goes high
that means a USB cable from host (or charger) is plugged in, if it goes
low USB cable was removed out. Enumeration/charging begins or stops
depending on the value read from the interrupt pin.
The battery temperature too is an edge interrupt. If it goes high it
means battery temp is either too hot or cold, charging needs to stop.
Charging can be resumed when this interrupt goes low.
There are many more such interrupts including battery insertion/removal,
sdcard insertion/removal, few keys on the phone etc which can benefit
from this api.
Note that these are not gpio lines configured as interrupts. If it were
gpio lines I can easily derive the gpio number (irq_to_gpio()) and read
the line.
I hope this clarifies my problem a bit more.
I can provide an api in my irq controller driver - lets call it
pm8xxx_read_irq_line(int irq) and ask the drivers to call it but thought
doing it via irq_chip could benefit other irq_controllers.
--
Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm
Innovation Center, Inc. is a member of the Code Aurora Forum.