Re: OMAP: Add TI TWL92330/Menelaus Power Management chip driver

[David Brownell <david-b@pacbell.net>]

On Thursday 12 July 2007, Andrew Morton wrote:
> On Tue, 10 Jul 2007 12:23:06 +0530
> "Trilok Soni" <soni.trilok@gmail.com> wrote:

> > +	u8			mask1, mask2;
> > +	void			(*handlers[16])(struct menelaus_chip *);
> 
> What determined the value of this hard-wired 16?

Two 8-bit registers for IRQ status, so total of 16 potential handlers.
Of course, I think several of those bits aren't actually used.


> > +/* Handles Menelaus interrupts. Does not run in interrupt context */
> > +static void menelaus_work(struct work_struct *_menelaus)
> > +{
> > +	struct menelaus_chip *menelaus =
> > +			container_of(_menelaus, struct menelaus_chip, work);
> > +	void (*handler)(struct menelaus_chip *menelaus);
> > +
> > +	while (1) {
> > +		unsigned isr;
> > +
> > +		isr = (menelaus_read_reg(MENELAUS_INT_STATUS2)
> > +				& ~menelaus->mask2) << 8;
> > +		isr |= menelaus_read_reg(MENELAUS_INT_STATUS1)
> > +				& ~menelaus->mask1;
> > +		if (!isr)
> > +			break;
> > +
> > +		while (isr) {
> > +			int irq = fls(isr) - 1;
> > +			isr &= ~(1 << irq);
> > +
> > +			mutex_lock(&menelaus->lock);
> > +			menelaus_disable_irq(irq);
> > +			menelaus_ack_irq(irq);
> > +			handler = menelaus->handlers[irq];
> > +			if (handler)
> > +				handler(menelaus);
> > +			menelaus_enable_irq(irq);
> > +			mutex_unlock(&menelaus->lock);
> > +		}
> > +	}
> > +	enable_irq(menelaus->client->irq);
> 
> This can do an enable_irq() of an already-enabled IRQ.  I don't know if
> that will cause runtime problems, but it doesn't seem desirable.

How could it do that?


> > +}
> > +
> > +/*
> > + * We cannot use I2C in interrupt context, so we just schedule work.
> > + */
> > +static irqreturn_t menelaus_irq(int irq, void *_menelaus)
> > +{
> > +	struct menelaus_chip *menelaus = _menelaus;
> > +
> > +	disable_irq_nosync(irq);
> 
> I guess the reader of this code will wonder why the nosync variant was
> used.  A comment would explain that.

Right.  ISTR that the "sync" version waits for any active
IRQ handler to complete.  That is, it's specified to cause
a self-deadlock in this call context (wait for self) ...


> In fact the reader might wonder why the heck we use a workqueue at all,
> rather than just doing the work in interrupt context as drivers normally
> do.
> 
> Perhaps that was all explained somewhere and I missed it.  If not, I'd
> suggest that this design decision should be described in a code comment
> somewhere.

Evidently the "we cannot use I2C in interrupt context" comment was
excessively subtle.  ;)

The whole I2C stack is synchronous; *EVERY* request to perform I/O
requires a can-sleep task context.  It's not like USB or SPI, where
the IRQ handler can submit I/O requests which will complete later.

Which is why every I2C driver that needs to handle an IRQ will need
to arrange some task context ... ergo the workqueue.  It's something
far more common in embedded systems than in the original "sensors"
context from which the I2C stack evolved.


> > +	int			err = 0;
> > +	struct menelaus_platform_data *menelaus_pdata =
> > +					client->dev.platform_data;
> > +
> > +	if (the_menelaus) {
> > +		dev_dbg(&client->dev, "only one %s for now\n",
> > +				DRIVER_NAME);
> > +		return -ENODEV;
> 
> I doubt if this can happen?

It's just paranoia.  If it ever *did* happen there would be
extreme confusion; it's easy to prevent, and useful to be
very explicit about this rule.