[i2c] [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

[Felipe Balbi]

On Sun, Mar 16, 2008 at 12:40 PM, Jean Delvare <khali at linux-fr.org> wrote:
>
> On Sun, 16 Mar 2008 12:22:10 +0200, Felipe Balbi wrote:
>  > On Sun, Mar 16, 2008 at 5:57 AM, David Brownell <david-b at pacbell.net> wrote:
>  > > On Saturday 15 March 2008, Jean Delvare wrote:
>  > >  > The above looks all wrong to me. The ISP1301 is an I²C device, it gets
>  > >  > an i2c driver, not a platform driver.
>  > >
>  > >  Right; blame that on me, for a moment I forgot and so I
>  > >  mentioned the use of the IORESOURCE_IRQ_* flags, which work
>  > >  on busses using resources -- like "platform" and "pnp").
>  > >  Not helpful here.
>  > >
>  > >
>  > >
>  > >  > That being said, if many drivers need an irq_type field in addition to
>  > >  > the irq number, we might consider adding an irq_type field to struct
>  > >  > i2c_board_info and struct i2c_client directly. It probably doesn't make
>  > >  > much sense to have irq and irq_type take different routes if they are
>  > >  > most often needed together.
>  > >
>  > >  Actually ... why not just require the board-specific setup
>  > >  code to set the right trigger mode?  Use set_irq_type().
>  > >
>  > >  That's what x86 does, and it works fine.  Drivers don't need
>  > >  to worry about trigger modes ("irq_type") at all, since the
>  > >  board setup code (on PCs: ACPI or BIOS) just stuffs the same
>  > >  registers that set_irq_type() would stuff.  Drivers can just:
>  > >
>  > >    status = request_irq(irq, handler, 0, "name", data);
>  > >
>  > >  Platforms already do the pinmux setup, making sure that the
>  > >  relevant pin is set up as an external interrupt source or
>  > >  GPIO input as appropriate.  They can just as well do the rest.
>  >
>  > Good catch dave, actually osk is doing that already :-)
>  > so I'll add set_irq_type($ISP1301_IRQ, $ISP1301_FLAGS); for all 3
>  > boards and on the driver i'll request_irq like you said above.
>
>  Very nice. I wanted to suggest this originally, but stepped back as I
>  noticed that request_irq() had a parameter for the IRQ flags. As
>  request_irq() must be called by the device driver, I concluded that we
>  really needed to carry the IRQ flags from the platform code to the
>  driver... I didn't know that there was a way to configure the IRQ
>  before requesting it. That's obviously much nicer. Would be great if we
>  could get rid of that flags parameter of request_irq(), if you ask me.

I was just thinking about the same :-)
If all the irq configuration were moved to board-files, we could get
rid of that parameter and it'd be just great ;-)

Thanks Dave and Jean for all your time during this patch. Hopefully
it's now good enough. I also sent the patch putting linux-omap and
linux-mainline in sync. If tell me that's ok, i'll send a similar
patch to linux-omap asap Cc:ing both you and Dave.

-- 
Best Regards,

Felipe Balbi
felipebalbi at users.sourceforge.net