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

[Felipe Balbi]

Hi,

On Sat, Mar 15, 2008 at 1:59 PM, Jean Delvare <khali at linux-fr.org> wrote:
> Hi Felipe,
>
>
>
>  On Fri, 7 Mar 2008 23:00:40 +0200, Felipe Balbi wrote:
>  > Hi,
>  >
>  > On Fri, Mar 7, 2008 at 10:20 PM, David Brownell <david-b at pacbell.net> wrote:
>  > > On Tuesday 26 February 2008, Felipe Balbi wrote:
>  > >  > >
>  > >  > >  Passing irq_flags???  That sounds like nonsense.  Please explain ...
>  > >  >
>  > >  > request_irq was using different irq flags depending on the board, so
>  > >  > instead of keeping if (machine_is_*) in the driver just because of irq
>  > >  > flags we could bring it from a platform_data.
>  > >
>  > >  Just use the resource type flags ... IORESOURCE_IRQ can
>  > >  be combined with
>  >
>  > but then isp1301 should become a platform_driver, shouldn't it?
>  > I'd need a struct platform_device on the board file and get the device
>  > on probe just to get an irq flag?
>  >
>  > Maybe I'm missing something but I'm understanding something like:
>  >
>  > --- a/arch/arm/mach-omap1/board-h2.c
>  > +++ b/arch/arm/mach-omap1/board-h2.c
>  > @@ -140,6 +140,17 @@ static struct platform_device h2_nor_device = {
>  >         .resource       = &h2_nor_resource,
>  >  };
>  >
>  > +static struct resource h2_isp1301_resource = {
>  > +       .flags          = IORESOURCE_IRQ | IORESOURCE_IRQ_HIGHEDGE,
>  > +};
>  > +
>  > +static struct platform_device h2_nor_device = {
>  > +       .name           = "isp1301_omap",
>  > +       .id             = 0,
>  > +       .num_resources  = 1,
>  > +       .resource       = &h2_isp1301_resource,
>  > +};
>  > +
>  >  static struct mtd_partition h2_nand_partitions[] = {
>  >  #if 0
>  >         /* REVISIT:  enable these partitions if you make NAND BOOT
>  > diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
>  > index 4bb931c..d0a5825 100644
>  > --- a/drivers/i2c/chips/isp1301_omap.c
>  > +++ b/drivers/i2c/chips/isp1301_omap.c
>  > @@ -1453,6 +1453,27 @@ isp1301_start_hnp(struct otg_transceiver *dev)
>  >
>  >  /*-------------------------------------------------------------------------*/
>  >
>  > +struct platform_driver isp1301_plat_driver = {
>  > +       .driver = {
>  > +               .name   = DRIVER_NAME,
>  > +               .bus    = &platform_bus_type,
>  > +               .owner  = THIS_MODULE,
>  > +       },
>  > +};
>  > +
>  > +static int __init isp1301_plat_probe(struct platform_device *pdev)
>  > +{
>  > +       struct resource *resource;
>  > +
>  > +       resource = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  > +       if (!resource)
>  > +               return -ENODEV;
>  > +
>  > +       the_transceiver->irq_type = resource->flags;
>  > +}
>  > +
>  > +/*-------------------------------------------------------------------------*/
>  > +
>  >  /* no error returns, they'd just make bus scanning stop */
>  >  static int __init isp1301_probe(struct i2c_client *client)
>  >  {
>  >
>  > (of course the code above doesn't work, it's just to clear my mind :-p)
>  >
>  > Anyways, wouldn't be easier to put a platform_data that can ship
>  > withing struct i2c_board_info?
>
>  The above looks all wrong to me. The ISP1301 is an I²C device, it gets
>  an i2c driver, not a platform driver.

That's what I was trying to say.

>  If you need to pass any extra parameter from the platform code to the
>  isp1301 driver, you must define a custom structure and pass it as
>  i2c_board_info->platform_data. In your case, it would look like:
>
>  struct isp1301_platform_data {
>         int irq_type;
>  }

Yes, I was doing exactly this.

>  David, please correct me if I'm wrong.
>
>  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.

Agreed here, the code will also look cleaner if we:

request_irq(client->irq, irq_handler, client->irq_flags,
                   CLIENT_DRIVER_NAME, client);


-- 
Best Regards,

Felipe Balbi
felipebalbi at users.sourceforge.net