* David Brownell <david-b@xxxxxxxxxxx> [081006 11:20]:
> I can't test the pwrbutton stuff, but rtc and musb behave...
>
> With one issue: the usb transceiver glue misbehaves on
> startup if the device is connected. (Again.) I looked at
> this a bit, and I think the issue is probably caused by
> not actually using key USB registers ... IRQ trigger
> mode state transitions are at best a fragile proxy for the
> real state.
>
> (This is similar to the GPIO patch I just sent, but simpler
> except for the impact on the few drivers thinking oddly
> about IRQs. Those patches cover the key SIH modules, and
> a similar one affects the PIH in twl4030-core.)
Pushing this, looks like we should have the remaining I2C
issues sorted out soon.
Tony
>
> - Dave
>
>
> ================================
> Streamline the "power irq" code, and some of the mechanisms
> it uses:
>
> - Support IRQ masking, not just unmasking; simpler code.
> - Use the standard handle_edge_irq() handler for these edge IRQs.
> - Use generic_handle_irq() instead of a manual expansion thereof.
> - Provide a missing spinlock for the shared data.
>
> In short, use more of the standard genirq code ... more correctly.
>
> Also, update the drivers using the "power IRQ" accordingly:
>
> - Don't request IRQF_DISABLED if the handler makes I2C calls;
> and defend against lockdep forcing it on us.
>
> - Let the irq_chip handle IRQ mask/unmask; it handles mutual
> exclusion between drivers, among other things.
>
> - (Unrelated) remove useless MODULE_ALIAS in pwrbutton.
>
> The USB transceiver driver still places some dodgey games with IRQ
> enable/disable, and IRQ trigger flags. At least some of them seem
> like they'd be simplified by using USB transceiver registers ...
> notably, startup code, which doesn't seem to check state before
> it enters an irq-driven mode.
>
> For the moment, these drivers still (wrongly) try to configure IRQ
> trigger modes themselves ... again, that's something that an irq_chip
> handles (but not yet, for this one).
>
> NOTE: tested (MUSB and RTC only) along with the init/retry hack
> to make twl4030-pwrirq work around the i2c-omap timeout problems.
>
> ---
> drivers/i2c/chips/twl4030-pwrbutton.c | 33 ++++------
> drivers/i2c/chips/twl4030-pwrirq.c | 98 ++++++++++----------------------
> drivers/i2c/chips/twl4030-usb.c | 57 ++++++++----------
> drivers/rtc/rtc-twl4030.c | 8 ++
> 4 files changed, 80 insertions(+), 116 deletions(-)
>
> --- a/drivers/i2c/chips/twl4030-pwrbutton.c
> +++ b/drivers/i2c/chips/twl4030-pwrbutton.c
> @@ -49,6 +49,14 @@ static irqreturn_t powerbutton_irq(int i
> int err;
> u8 value;
>
> +#ifdef CONFIG_LOCKDEP
> + /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> + * we don't want and can't tolerate. Although it might be
> + * friendlier not to borrow this thread context...
> + */
> + local_irq_enable();
> +#endif
> +
> err = twl4030_i2c_read_u8(TWL4030_MODULE_PM_MASTER, &value,
> STS_HW_CONDITIONS);
> if (!err) {
> @@ -67,6 +75,7 @@ static int __init twl4030_pwrbutton_init
> int err = 0;
> u8 value;
>
> + /* PWRBTN == PWRON */
> if (request_irq(TWL4030_PWRIRQ_PWRBTN, powerbutton_irq, 0,
> "PwrButton", NULL) < 0) {
> printk(KERN_ERR "Unable to allocate IRQ for power button\n");
> @@ -92,22 +101,9 @@ static int __init twl4030_pwrbutton_init
> goto free_irq_and_out;
> }
>
> - err = twl4030_i2c_read_u8(TWL4030_MODULE_INT, &value, PWR_IMR1);
> - if (err) {
> - printk(KERN_WARNING "I2C error %d while reading TWL4030"
> - " INT PWR_IMR1 register\n", err);
> -
> - goto free_input_dev;
> - }
> -
> - err = twl4030_i2c_write_u8(TWL4030_MODULE_INT,
> - value & (~PWR_PWRON_IRQ), PWR_IMR1);
> - if (err) {
> - printk(KERN_WARNING "I2C error %d while writing TWL4030"
> - " INT PWR_IMR1 register\n", err);
> - goto free_input_dev;
> - }
> -
> + /* FIXME just pass IRQF_EDGE_FALLING | IRQF_EDGE_RISING
> + * to request_irq(), once MODULE_INT supports them...
> + */
> err = twl4030_i2c_read_u8(TWL4030_MODULE_INT, &value, PWR_EDR1);
> if (err) {
> printk(KERN_WARNING "I2C error %d while reading TWL4030"
> @@ -136,19 +132,16 @@ free_irq_and_out:
> out:
> return err;
> }
> +module_init(twl4030_pwrbutton_init);
>
> static void __exit twl4030_pwrbutton_exit(void)
> {
> free_irq(TWL4030_PWRIRQ_PWRBTN, NULL);
> input_unregister_device(powerbutton_dev);
> input_free_device(powerbutton_dev);
> -
> }
> -
> -module_init(twl4030_pwrbutton_init);
> module_exit(twl4030_pwrbutton_exit);
>
> -MODULE_ALIAS("i2c:twl4030-pwrbutton");
> MODULE_DESCRIPTION("Triton2 Power Button");
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Peter De Schrijver");
> --- a/drivers/i2c/chips/twl4030-pwrirq.c
> +++ b/drivers/i2c/chips/twl4030-pwrirq.c
> @@ -29,21 +29,35 @@
> #include <linux/i2c/twl4030.h>
>
>
> +static DEFINE_SPINLOCK(pwr_lock);
> static u8 twl4030_pwrirq_mask;
> -static u8 twl4030_pwrirq_pending_unmask;
>
> static struct task_struct *twl4030_pwrirq_unmask_thread;
>
> static void twl4030_pwrirq_ack(unsigned int irq) {}
>
> -static void twl4030_pwrirq_disableint(unsigned int irq) {}
> +static void twl4030_pwrirq_disableint(unsigned int irq)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pwr_lock, flags);
> + twl4030_pwrirq_mask |= 1 << (irq - TWL4030_PWR_IRQ_BASE);
> + if (twl4030_pwrirq_unmask_thread
> + && twl4030_pwrirq_unmask_thread->state != TASK_RUNNING)
> + wake_up_process(twl4030_pwrirq_unmask_thread);
> + spin_unlock_irqrestore(&pwr_lock, flags);
> +}
>
> static void twl4030_pwrirq_enableint(unsigned int irq)
> {
> - twl4030_pwrirq_pending_unmask |= 1 << (irq - TWL4030_PWR_IRQ_BASE);
> - if (twl4030_pwrirq_unmask_thread &&
> - twl4030_pwrirq_unmask_thread->state != TASK_RUNNING)
> + unsigned long flags;
> +
> + spin_lock_irqsave(&pwr_lock, flags);
> + twl4030_pwrirq_mask &= ~(1 << (irq - TWL4030_PWR_IRQ_BASE));
> + if (twl4030_pwrirq_unmask_thread
> + && twl4030_pwrirq_unmask_thread->state != TASK_RUNNING)
> wake_up_process(twl4030_pwrirq_unmask_thread);
> + spin_unlock_irqrestore(&pwr_lock, flags);
> }
>
> static struct irq_chip twl4030_pwrirq_chip = {
> @@ -53,48 +67,6 @@ static struct irq_chip twl4030_pwrirq_ch
> .unmask = twl4030_pwrirq_enableint,
> };
>
> -static void do_twl4030_pwrmodule_irq(unsigned int irq, irq_desc_t *desc)
> -{
> - struct irqaction *action;
> - const unsigned int cpu = smp_processor_id();
> -
> - desc->status |= IRQ_LEVEL;
> -
> - if (!desc->depth) {
> - kstat_cpu(cpu).irqs[irq]++;
> -
> - action = desc->action;
> - if (action) {
> - int ret;
> - int status = 0;
> - int retval = 0;
> -
> - do {
> - ret = action->handler(irq, action->dev_id);
> - if (ret == IRQ_HANDLED)
> - status |= action->flags;
> - retval |= ret;
> - action = action->next;
> - } while (action);
> -
> - if (status & IRQF_SAMPLE_RANDOM)
> - add_interrupt_randomness(irq);
> -
> - if (retval != IRQ_HANDLED)
> - printk(KERN_ERR "ISR for TWL4030 power module"
> - " irq %d can't handle interrupt\n",
> - irq);
> - } else {
> - local_irq_disable();
> - twl4030_pwrirq_mask |= 1 << (irq - TWL4030_PWR_IRQ_BASE);
> - local_irq_enable();
> - twl4030_i2c_write_u8(TWL4030_MODULE_INT,
> - twl4030_pwrirq_mask,
> - TWL4030_INT_PWR_IMR1);
> - }
> - }
> -}
> -
> static void do_twl4030_pwrirq(unsigned int irq, irq_desc_t *desc)
> {
> const unsigned int cpu = smp_processor_id();
> @@ -113,6 +85,7 @@ static void do_twl4030_pwrirq(unsigned i
> local_irq_enable();
> ret = twl4030_i2c_read_u8(TWL4030_MODULE_INT, &pwr_isr,
> TWL4030_INT_PWR_ISR1);
> + local_irq_disable();
> if (ret) {
> printk(KERN_WARNING
> "I2C error %d while reading TWL4030"
> @@ -122,13 +95,10 @@ static void do_twl4030_pwrirq(unsigned i
>
> for (module_irq = TWL4030_PWR_IRQ_BASE; pwr_isr != 0;
> module_irq++, pwr_isr >>= 1) {
> - if (pwr_isr & 1) {
> - irq_desc_t *d = irq_desc + module_irq;
> - d->handle_irq(module_irq, d);
> - }
> + if (pwr_isr & 1)
> + generic_handle_irq(module_irq);
> }
>
> - local_irq_disable();
> desc->chip->unmask(irq);
> }
> }
> @@ -138,22 +108,19 @@ static int twl4030_pwrirq_thread(void *d
> current->flags |= PF_NOFREEZE;
>
> while (!kthread_should_stop()) {
> - u8 local_unmask;
> -
> - local_irq_disable();
> - local_unmask = twl4030_pwrirq_pending_unmask;
> - twl4030_pwrirq_pending_unmask = 0;
> - local_irq_enable();
> + u8 local_mask;
>
> - twl4030_pwrirq_mask &= ~local_unmask;
> + spin_lock_irq(&pwr_lock);
> + local_mask = twl4030_pwrirq_mask;
> + spin_unlock_irq(&pwr_lock);
>
> - twl4030_i2c_write_u8(TWL4030_MODULE_INT, twl4030_pwrirq_mask,
> + twl4030_i2c_write_u8(TWL4030_MODULE_INT, local_mask,
> TWL4030_INT_PWR_IMR1);
>
> - local_irq_disable();
> - if (!twl4030_pwrirq_pending_unmask)
> + spin_lock_irq(&pwr_lock);
> + if (local_mask == twl4030_pwrirq_mask)
> set_current_state(TASK_INTERRUPTIBLE);
> - local_irq_enable();
> + spin_unlock_irq(&pwr_lock);
>
> schedule();
> }
> @@ -168,7 +135,6 @@ static int __init twl4030_pwrirq_init(vo
> int i, err;
>
> twl4030_pwrirq_mask = 0xff;
> - twl4030_pwrirq_pending_unmask = 0;
>
> /* HEY: core already did this.
> * But that's surely not why we
> @@ -201,8 +167,8 @@ msleep(10);
> }
>
> for (i = TWL4030_PWR_IRQ_BASE; i < TWL4030_PWR_IRQ_END; i++) {
> - set_irq_chip(i, &twl4030_pwrirq_chip);
> - set_irq_handler(i, do_twl4030_pwrmodule_irq);
> + set_irq_chip_and_handler(i, &twl4030_pwrirq_chip,
> + handle_edge_irq);
> set_irq_flags(i, IRQF_VALID);
> }
>
> --- a/drivers/i2c/chips/twl4030-usb.c
> +++ b/drivers/i2c/chips/twl4030-usb.c
> @@ -237,14 +237,9 @@
> #define GPIO_USB_4PIN_ULPI_2430C (3 << 0)
>
> /* In module TWL4030_MODULE_INT */
> -#define REG_PWR_ISR1 0x00
> -#define REG_PWR_IMR1 0x01
> -#define USB_PRES (1 << 2)
> #define REG_PWR_EDR1 0x05
> #define USB_PRES_FALLING (1 << 4)
> #define USB_PRES_RISING (1 << 5)
> -#define REG_PWR_SIH_CTRL 0x07
> -#define COR (1 << 2)
>
> /* bits in OTG_CTRL */
> #define OTG_XCEIV_OUTPUTS \
> @@ -274,6 +269,7 @@ struct twl4030_usb {
> unsigned vbus:1;
> int irq;
> u8 asleep;
> + bool irq_enabled;
> };
>
> /* internal define on top of container_of */
> @@ -417,6 +413,8 @@ static void usb_irq_enable(struct twl403
> {
> u8 val;
>
> + /* FIXME use set_irq_type(...) when that (soon) works */
> +
> /* edge setup */
> WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
> &val, REG_PWR_EDR1) < 0);
> @@ -429,34 +427,18 @@ static void usb_irq_enable(struct twl403
> WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT,
> val, REG_PWR_EDR1) < 0);
>
> - /* un-mask interrupt */
> - WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
> - &val, REG_PWR_IMR1) < 0);
> -
> - val &= ~USB_PRES;
> -
> - WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT,
> - val, REG_PWR_IMR1) < 0);
> + if (!twl->irq_enabled) {
> + enable_irq(twl->irq);
> + twl->irq_enabled = true;
> + }
> }
>
> static void usb_irq_disable(struct twl4030_usb *twl)
> {
> - u8 val;
> -
> - /* undo edge setup */
> - WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
> - &val, REG_PWR_EDR1) < 0);
> - val &= ~(USB_PRES_RISING | USB_PRES_FALLING);
> - WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT,
> - val, REG_PWR_EDR1) < 0);
> -
> - /* mask interrupt */
> - WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
> - &val, REG_PWR_IMR1) < 0);
> - val |= USB_PRES;
> -
> - WARN_ON(twl4030_i2c_write_u8_verify(twl, TWL4030_MODULE_INT,
> - val, REG_PWR_IMR1) < 0);
> + if (twl->irq_enabled) {
> + disable_irq(twl->irq);
> + twl->irq_enabled = false;
> + }
> }
>
> static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> @@ -565,6 +547,21 @@ static irqreturn_t twl4030_usb_irq(int i
> struct twl4030_usb *twl = _twl;
> u8 val;
>
> +#ifdef CONFIG_LOCKDEP
> + /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> + * we don't want and can't tolerate. Although it might be
> + * friendlier not to borrow this thread context...
> + */
> + local_irq_enable();
> +#endif
> +
> + /* FIXME stop accessing PWR_EDR1 ... if nothing else, we
> + * know which edges we told the IRQ to trigger on. And
> + * there seem to be OTG_specific registers and irqs that
> + * provide the right info without guessing like this:
> + * USB_INT_STS, ID_STATUS, STS_HW_CONDITIONS, etc.
> + */
> +
> /* action based on cable attach or detach */
> WARN_ON(twl4030_i2c_read_u8(TWL4030_MODULE_INT,
> &val, REG_PWR_EDR1) < 0);
> @@ -697,7 +694,7 @@ static int __init twl4030_usb_probe(stru
> /* init irq workqueue before request_irq */
> INIT_WORK(&twl->irq_work, twl4030_usb_irq_work);
>
> - usb_irq_disable(twl);
> + twl->irq_enabled = true;
> status = request_irq(twl->irq, twl4030_usb_irq, 0, "twl4030_usb", twl);
> if (status < 0) {
> dev_dbg(&pdev->dev, "can't get IRQ %d, err %d\n",
> --- a/drivers/rtc/rtc-twl4030.c
> +++ b/drivers/rtc/rtc-twl4030.c
> @@ -347,6 +347,14 @@ static irqreturn_t twl4030_rtc_interrupt
> int res;
> u8 rd_reg;
>
> +#ifdef CONFIG_LOCKDEP
> + /* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> + * we don't want and can't tolerate. Although it might be
> + * friendlier not to borrow this thread context...
> + */
> + local_irq_enable();
> +#endif
> +
> res = twl4030_rtc_read_u8(&rd_reg, REG_RTC_STATUS_REG);
> if (res)
> goto out;
> --
> 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
--
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
