On Tue, Aug 27, 2013 at 12:30 PM, George Cherian <george.cherian@xxxxxx> wrote:
> This patch
> - removes the irq_demux_work
> - Uses devm_request_threaded_irq
> - Call the user handler iff gpio_to_irq is done.
>
> Signed-off-by: George Cherian <george.cherian@xxxxxx>
Can you please split this up? It seems like three different patches,
and now they block each other. The threading patch is fine and
I could apply it unless this was mixed up with other stuff.
I'd like Kuninoro and/or Nikolay to have a look at this, so please
CC them on subsequent iterations.
NB: I really like that you move the irq handling to a thread, good
job.
> static const struct i2c_device_id pcf857x_id[] = {
> @@ -89,12 +89,12 @@ struct pcf857x {
> struct gpio_chip chip;
> struct i2c_client *client;
> struct mutex lock; /* protect 'out' */
> - struct work_struct work; /* irq demux work */
> struct irq_domain *irq_domain; /* for irq demux */
> spinlock_t slock; /* protect irq demux */
> unsigned out; /* software latch */
> unsigned status; /* current status */
> int irq; /* real irq number */
> + int irq_mapped; /* mapped gpio irqs */
This seems like an u32 or atleast unsigned, and state that its one
bit flag per IRQ. How many GPIO lines are there?
> -static void pcf857x_irq_demux_work(struct work_struct *work)
> +static irqreturn_t pcf857x_irq(int irq, void *data)
> {
> - struct pcf857x *gpio = container_of(work,
> - struct pcf857x,
> - work);
> + struct pcf857x *gpio = data;
> unsigned long change, i, status, flags;
>
> status = gpio->read(gpio->client);
>
> spin_lock_irqsave(&gpio->slock, flags);
> +
> + /*
> + * call the interrupt handler iff gpio is used as
> + * interrupt source, just to avoid bad irqs
> + */
>
> - change = gpio->status ^ status;
> + change = ((gpio->status ^ status) & gpio->irq_mapped);
I don't know if that is right.
If this happens you are getting what we call a "spurious IRQ"
and this shall be reported to the IRQ core by returning
IRQ_NONE and handled from there.
> @@ -226,9 +223,13 @@ static irqreturn_t pcf857x_irq_demux(int irq, void *data)
> static int pcf857x_irq_domain_map(struct irq_domain *domain, unsigned int virq,
> irq_hw_number_t hw)
> {
> + struct pcf857x *gpio = domain->host_data;
> irq_set_chip_and_handler(virq,
> &dummy_irq_chip,
> handle_level_irq);
> + set_irq_flags(virq, IRQF_VALID);
> + gpio->irq_mapped |= (1 << hw);
I'm a bit uneasy about this. It feels like its the irqdomain's
responsibility to keep track of whether an IRQ is mapped
or not.
Maybe Grant should have a look at this.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
