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

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

[Jean Delvare]

Hi Felipe,

On Thu,  3 Jan 2008 11:59:56 -0500, Felipe Balbi wrote:
> Based on David Brownell's patch for tps65010, this patch
> starts converting isp1301_omap.c to new-style i2c driver.
> 
> Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> ---
>  drivers/i2c/chips/isp1301_omap.c |   60 +++++++++++++++++++------------------
>  1 files changed, 31 insertions(+), 29 deletions(-)
> 

Next time, please send this patch to the i2c mailing list instead of
LKML if you want to get some attention.

I'm fine with this patch except for:

> diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
> index b767603..37e1403 100644
> --- a/drivers/i2c/chips/isp1301_omap.c
> +++ b/drivers/i2c/chips/isp1301_omap.c
> (...)
> @@ -1499,12 +1500,13 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
>  	isp->timer.data = (unsigned long) isp;
>  
>  	isp->irq = -1;
> -	isp->client.addr = address;
> -	i2c_set_clientdata(&isp->client, isp);
> -	isp->client.adapter = bus;
> -	isp->client.driver = &isp1301_driver;
> -	strlcpy(isp->client.name, DRIVER_NAME, I2C_NAME_SIZE);
> -	i2c = &isp->client;
> +	isp->irq_type = 0;

The structure is initialized by kzalloc() so no need to explicitly set
this field to 0.

> +	isp->c.addr = address;
> +	i2c_set_clientdata(&isp->c, isp);
> +	isp->c.adapter = bus;
> +	isp->c.driver = &isp1301_driver;
> +	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
> +	isp->client = i2c = &isp->c;
>  
>  	/* if this is a true probe, verify the chip ... */
>  	if (kind < 0) {

I'll change it myself, no need to resend.

-- 
Jean Delvare

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

[Felipe Balbi]

On Jan 21, 2008 7:15 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Felipe,
>
> On Thu,  3 Jan 2008 11:59:56 -0500, Felipe Balbi wrote:
> > Based on David Brownell's patch for tps65010, this patch
> > starts converting isp1301_omap.c to new-style i2c driver.
> >
> > Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> > ---
> >  drivers/i2c/chips/isp1301_omap.c |   60 +++++++++++++++++++------------------
> >  1 files changed, 31 insertions(+), 29 deletions(-)
> >
>
> Next time, please send this patch to the i2c mailing list instead of
> LKML if you want to get some attention.

Ok, i'll do it.
Sorry for missing i2c mailing list.

-- 
Best Regards,

Felipe Balbi
felipebalbi@users.sourceforge.net

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

[Jean Delvare]

Hi Felipe,

On Thu,  3 Jan 2008 11:59:57 -0500, Felipe Balbi wrote:
> Based on David Brownell's patch for tps65010, this patch
> finish conversting isp1301_omap.c to new-style i2c driver.
> 
> Signed-off-by: Felipe Balbi <me@felipebalbi.com>
> ---
>  arch/arm/mach-omap1/board-h2.c   |    6 ++-
>  arch/arm/mach-omap1/board-h3.c   |    6 ++-
>  arch/arm/mach-omap2/board-h4.c   |   12 +++
>  drivers/i2c/chips/isp1301_omap.c |  149 +++++++++++++-------------------------
>  4 files changed, 73 insertions(+), 100 deletions(-)
> 
> diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
> index 1306812..0307f50 100644
> --- a/arch/arm/mach-omap1/board-h2.c
> +++ b/arch/arm/mach-omap1/board-h2.c
> @@ -350,8 +350,12 @@ static struct i2c_board_info __initdata h2_i2c_board_info[] = {
>  		.type		= "tps65010",
>  		.irq		= OMAP_GPIO_IRQ(58),
>  	},
> +	{
> +		I2C_BOARD_INFO("isp1301_omap", 0x2d),
> +		.type		= "isp1301_omap",
> +		.irq		= OMAP_GPIO_IRQ(2),
> +	},
>  	/* TODO when driver support is ready:
> -	 *  - isp1301 OTG transceiver
>  	 *  - optional ov9640 camera sensor at 0x30
>  	 *  - pcf9754 for aGPS control
>  	 *  - ... etc
> diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
> index 4f84ae2..71e285a 100644
> --- a/arch/arm/mach-omap1/board-h3.c
> +++ b/arch/arm/mach-omap1/board-h3.c
> @@ -463,8 +463,12 @@ static struct i2c_board_info __initdata h3_i2c_board_info[] = {
>  		.type		= "tps65013",
>  		/* .irq		= OMAP_GPIO_IRQ(??), */
>  	},
> +	{
> +		I2C_BOARD_INFO("isp1301_omap", 0x2d),
> +		.type		= "isp1301_omap",
> +		.irq		= OMAP_GPIO_IRQ(14),
> +	},
>  	/* TODO when driver support is ready:
> -	 *  - isp1301 OTG transceiver
>  	 *  - optional ov9640 camera sensor at 0x30
>  	 *  - ...
>  	 */
> diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
> index f125f43..33ff80b 100644
> --- a/arch/arm/mach-omap2/board-h4.c
> +++ b/arch/arm/mach-omap2/board-h4.c
> @@ -301,6 +301,15 @@ static struct omap_board_config_kernel h4_config[] = {
>  	{ OMAP_TAG_LCD,		&h4_lcd_config },
>  };
>  
> +static struct i2c_board_info __initdata h4_i2c_board_info[] = {
> +	{
> +		I2C_BOARD_INFO("isp1301_omap", 0x2d),
> +		.type		= "isp1301_omap",
> +		.irq		= OMAP_GPIO_IRQ(125),
> +	},
> +};
> +
> +
>  static void __init omap_h4_init(void)
>  {
>  	/*
> @@ -321,6 +330,9 @@ static void __init omap_h4_init(void)
>  	}
>  #endif
>  
> +	i2c_register_board_info(1, h4_i2c_board_info,
> +			ARRAY_SIZE(h4_i2c_board_info));
> +
>  	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
>  	omap_board_config = h4_config;
>  	omap_board_config_size = ARRAY_SIZE(h4_config);
> diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
> index 37e1403..c7a7c48 100644
> --- a/drivers/i2c/chips/isp1301_omap.c
> +++ b/drivers/i2c/chips/isp1301_omap.c
> @@ -31,16 +31,12 @@
>  #include <linux/usb/otg.h>
>  #include <linux/i2c.h>
>  #include <linux/workqueue.h>
> -
> -#include <asm/irq.h>
>  #include <asm/arch/usb.h>
>  
> -
>  #ifndef	DEBUG
>  #undef	VERBOSE
>  #endif
>  
> -
>  #define	DRIVER_VERSION	"24 August 2004"
>  #define	DRIVER_NAME	(isp1301_driver.driver.name)
>  
> @@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
>  struct isp1301 {
>  	struct otg_transceiver	otg;
>  	struct i2c_client	*client;
> -	struct i2c_client	c;
>  	void			(*i2c_release)(struct device *dev);
>  
> -	int			irq;
> -	int			irq_type;
> -
>  	u32			last_otg_ctrl;
>  	unsigned		working:1;
>  
> @@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
>  /*-------------------------------------------------------------------------*/
>  
>  /* only two addresses possible */
> -#define	ISP_BASE		0x2c
> -static unsigned short normal_i2c[] = {
> -	ISP_BASE, ISP_BASE + 1,
> -	I2C_CLIENT_END };
> -
> -I2C_CLIENT_INSMOD;
> -
>  static struct i2c_driver isp1301_driver;
>  
>  /* smbus apis are used for portability */
> @@ -1196,9 +1181,11 @@ static void isp1301_timer(unsigned long _isp)
>  
>  static void isp1301_release(struct device *dev)
>  {
> -	struct isp1301	*isp;
> +	struct i2c_client	*client;
> +	struct isp1301		*isp;
>  
> -	isp = container_of(dev, struct isp1301, c.dev);
> +	client = container_of(dev, struct i2c_client, dev);
> +	isp = i2c_get_clientdata(client);

This seems to be a quite complex way to do:

	isp = i2c_get_drvdata(dev);

Or am I missing something?

>  
>  	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
>  	if (isp->i2c_release)
> @@ -1208,30 +1195,27 @@ static void isp1301_release(struct device *dev)
>  
>  static struct isp1301 *the_transceiver;
>  
> -static int isp1301_detach_client(struct i2c_client *i2c)
> +static int __exit isp1301_remove(struct i2c_client *client)
>  {
> -	struct isp1301	*isp;
> -
> -	isp = container_of(i2c, struct isp1301, c);
> +	struct isp1301	*isp = i2c_get_clientdata(client);
>  
>  	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
>  	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
> -	free_irq(isp->irq, isp);
> +
> +	if (client->irq > 0)
> +		free_irq(client->irq, isp);
>  #ifdef	CONFIG_USB_OTG
>  	otg_unbind(isp);
>  #endif
> -	if (machine_is_omap_h2())
> -		omap_free_gpio(2);
> -

Why?

>  	isp->timer.data = 0;
>  	set_bit(WORK_STOP, &isp->todo);
>  	del_timer_sync(&isp->timer);
>  	flush_scheduled_work();
>  
> -	put_device(&i2c->dev);
> +	put_device(&client->dev);
>  	the_transceiver = 0;
>  
> -	return i2c_detach_client(i2c);
> +	return 0;
>  }
>  
>  /*-------------------------------------------------------------------------*/
> @@ -1301,9 +1285,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
>  
>  	power_up(isp);
>  
> -	if (machine_is_omap_h2())
> -		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
> -

Where has this code gone? Why is it no longer needed?

(Did you test you patch on OMAP H2?)

>  	dev_info(&isp->client->dev, "A-Host sessions ok\n");
>  	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
>  		INTR_ID_GND);
> @@ -1481,11 +1462,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
>  /*-------------------------------------------------------------------------*/
>  
>  /* no error returns, they'd just make bus scanning stop */
> -static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
> +static int __init isp1301_probe(struct i2c_client *client)
>  {
>  	int			status;
>  	struct isp1301		*isp;
> -	struct i2c_client	*i2c;
>  
>  	if (the_transceiver)
>  		return 0;
> @@ -1498,46 +1478,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
>  	init_timer(&isp->timer);
>  	isp->timer.function = isp1301_timer;
>  	isp->timer.data = (unsigned long) isp;
> -
> -	isp->irq = -1;
> -	isp->irq_type = 0;
> -	isp->c.addr = address;
> -	i2c_set_clientdata(&isp->c, isp);
> -	isp->c.adapter = bus;
> -	isp->c.driver = &isp1301_driver;
> -	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
> -	isp->client = i2c = &isp->c;
> +	isp->client = client;
>  
>  	/* if this is a true probe, verify the chip ... */
> -	if (kind < 0) {
> -		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> -		if (status != I2C_VENDOR_ID_PHILIPS) {
> -			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
> -				address, status);
> -			goto fail1;
> -		}
> -		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> -		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> -			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
> -				address, status);
> -			goto fail1;
> -		}
> +	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> +	if (status != I2C_VENDOR_ID_PHILIPS) {
> +		dev_dbg(&client->dev, "not philips id: %d\n",
> +				status);

Fits on a single line.

> +		goto fail1;
> +	}
> +	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> +	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> +		dev_dbg(&client->dev, "not isp1301, %d\n",
> +				status);

Same here.

> +		goto fail1;
>  	}
>  
> -	status = i2c_attach_client(i2c);
>  	if (status < 0) {
> -		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
> -				DRIVER_NAME, address, status);
> +		dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
> +				DRIVER_NAME, status);

It doesn't make sense to remove the call to i2c_attach_client() but
preserve the status check and debug message!

>  fail1:
>  		kfree(isp);
>  		return 0;
>  	}
> -	isp->i2c_release = i2c->dev.release;
> -	i2c->dev.release = isp1301_release;
> +	isp->i2c_release = client->dev.release;
> +	client->dev.release = isp1301_release;
>  
>  	/* initial development used chiprev 2.00 */
> -	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
> -	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
> +	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
> +	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
>  		status >> 8, status & 0xff);
>  
>  	/* make like power-on reset */
> @@ -1558,40 +1527,25 @@ fail1:
>  #ifdef	CONFIG_USB_OTG
>  	status = otg_bind(isp);
>  	if (status < 0) {
> -		dev_dbg(&i2c->dev, "can't bind OTG\n");
> +		dev_dbg(&client->dev, "can't bind OTG\n");
>  		goto fail2;
>  	}
>  #endif
>  
> -	if (machine_is_omap_h2()) {
> -		/* full speed signaling by default */
> -		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
> -			MC1_SPEED_REG);
> -		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
> -			MC2_SPD_SUSP_CTRL);
> -
> -		/* IRQ wired at M14 */
> -		omap_cfg_reg(M14_1510_GPIO2);
> -		isp->irq = OMAP_GPIO_IRQ(2);
> -		if (gpio_request(2, "isp1301") == 0)
> -			gpio_direction_input(2);
> -		isp->irq_type = IRQF_TRIGGER_FALLING;
> -	}

Again, why would you remove this code?

> -
> -	isp->irq_type |= IRQF_SAMPLE_RANDOM;
> -	status = request_irq(isp->irq, isp1301_irq,
> -			isp->irq_type, DRIVER_NAME, isp);
> +	status = request_irq(client->irq, isp1301_irq,
> +			IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
> +			DRIVER_NAME, isp);

When freeing the irq you first test that client->irq > 0, but when
requesting it you do not? It's inconsistent.

Also, the original code was passing different IRQF flags depending on
the platform, your changes force the same mode for everyone. This
doesn't look correct.

>  	if (status < 0) {
> -		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
> -				isp->irq, status);
> +		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
> +				client->irq, status);
>  #ifdef	CONFIG_USB_OTG
>  fail2:
>  #endif
> -		i2c_detach_client(i2c);
> +		i2c_detach_client(client);
>  		goto fail1;
>  	}
>  
> -	isp->otg.dev = &isp->client->dev;
> +	isp->otg.dev = &client->dev;
>  	isp->otg.label = DRIVER_NAME;
>  
>  	isp->otg.set_host = isp1301_set_host,
> @@ -1608,43 +1562,42 @@ fail2:
>  	update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
>  	update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
>  #endif
> -

Noise, please revert.

>  	dump_regs(isp, __FUNCTION__);
>  
>  #ifdef	VERBOSE
>  	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
> -	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
> +	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
>  #endif
>  
>  	status = otg_set_transceiver(&isp->otg);
>  	if (status < 0)
> -		dev_err(&i2c->dev, "can't register transceiver, %d\n",
> +		dev_err(&client->dev, "can't register transceiver, %d\n",
>  			status);
>  
>  	return 0;
>  }
>  
> -static int isp1301_scan_bus(struct i2c_adapter *bus)
> -{
> -	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
> -			| I2C_FUNC_SMBUS_READ_WORD_DATA))
> -		return -EINVAL;
> -	return i2c_probe(bus, &addr_data, isp1301_probe);
> -}
> -
>  static struct i2c_driver isp1301_driver = {
>  	.driver = {
>  		.name	= "isp1301_omap",
>  	},
> -	.attach_adapter	= isp1301_scan_bus,
> -	.detach_client	= isp1301_detach_client,
> +	.probe	= isp1301_probe,
> +	.remove	= __exit_p(isp1301_remove),
>  };
>  
>  /*-------------------------------------------------------------------------*/
>  
>  static int __init isp_init(void)
>  {
> -	return i2c_add_driver(&isp1301_driver);
> +	int	status = -ENODEV;
> +
> +	printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);

What's the point of printing a driver version that nobody bothers
updating?

Most i2c chip drivers keep quiet when loaded, they print a message when
a device is actually found, as this driver is doing.

> +
> +	status = i2c_add_driver(&isp1301_driver);
> +	if (status)
> +		printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);

This is extremely unlikely to happen, and if it did, i2c-core would
already log a more informative error message, and insmod/modprobe as
well. So you can just call i2c_add_driver() and be done with it (as
the driver was originally doing.)

> +
> +	return status;
>  }
>  module_init(isp_init);
>  


-- 
Jean Delvare

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

[Felipe Balbi]

Hi,

On Jan 22, 2008 2:01 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Felipe,
>
>
> On Thu,  3 Jan 2008 11:59:57 -0500, Felipe Balbi wrote:
> > Based on David Brownell's patch for tps65010, this patch
> > finish conversting isp1301_omap.c to new-style i2c driver.
> >
> > Signed-off-by: Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org>
> > ---
> >  arch/arm/mach-omap1/board-h2.c   |    6 ++-
> >  arch/arm/mach-omap1/board-h3.c   |    6 ++-
> >  arch/arm/mach-omap2/board-h4.c   |   12 +++
> >  drivers/i2c/chips/isp1301_omap.c |  149 +++++++++++++-------------------------
> >  4 files changed, 73 insertions(+), 100 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
> > index 1306812..0307f50 100644
> > --- a/arch/arm/mach-omap1/board-h2.c
> > +++ b/arch/arm/mach-omap1/board-h2.c
> > @@ -350,8 +350,12 @@ static struct i2c_board_info __initdata h2_i2c_board_info[] = {
> >               .type           = "tps65010",
> >               .irq            = OMAP_GPIO_IRQ(58),
> >       },
> > +     {
> > +             I2C_BOARD_INFO("isp1301_omap", 0x2d),
> > +             .type           = "isp1301_omap",
> > +             .irq            = OMAP_GPIO_IRQ(2),
> > +     },
> >       /* TODO when driver support is ready:
> > -      *  - isp1301 OTG transceiver
> >        *  - optional ov9640 camera sensor at 0x30
> >        *  - pcf9754 for aGPS control
> >        *  - ... etc
> > diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
> > index 4f84ae2..71e285a 100644
> > --- a/arch/arm/mach-omap1/board-h3.c
> > +++ b/arch/arm/mach-omap1/board-h3.c
> > @@ -463,8 +463,12 @@ static struct i2c_board_info __initdata h3_i2c_board_info[] = {
> >               .type           = "tps65013",
> >               /* .irq         = OMAP_GPIO_IRQ(??), */
> >       },
> > +     {
> > +             I2C_BOARD_INFO("isp1301_omap", 0x2d),
> > +             .type           = "isp1301_omap",
> > +             .irq            = OMAP_GPIO_IRQ(14),
> > +     },
> >       /* TODO when driver support is ready:
> > -      *  - isp1301 OTG transceiver
> >        *  - optional ov9640 camera sensor at 0x30
> >        *  - ...
> >        */
> > diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
> > index f125f43..33ff80b 100644
> > --- a/arch/arm/mach-omap2/board-h4.c
> > +++ b/arch/arm/mach-omap2/board-h4.c
> > @@ -301,6 +301,15 @@ static struct omap_board_config_kernel h4_config[] = {
> >       { OMAP_TAG_LCD,         &h4_lcd_config },
> >  };
> >
> > +static struct i2c_board_info __initdata h4_i2c_board_info[] = {
> > +     {
> > +             I2C_BOARD_INFO("isp1301_omap", 0x2d),
> > +             .type           = "isp1301_omap",
> > +             .irq            = OMAP_GPIO_IRQ(125),
> > +     },
> > +};
> > +
> > +
> >  static void __init omap_h4_init(void)
> >  {
> >       /*
> > @@ -321,6 +330,9 @@ static void __init omap_h4_init(void)
> >       }
> >  #endif
> >
> > +     i2c_register_board_info(1, h4_i2c_board_info,
> > +                     ARRAY_SIZE(h4_i2c_board_info));
> > +
> >       platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
> >       omap_board_config = h4_config;
> >       omap_board_config_size = ARRAY_SIZE(h4_config);
> > diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
> > index 37e1403..c7a7c48 100644
> > --- a/drivers/i2c/chips/isp1301_omap.c
> > +++ b/drivers/i2c/chips/isp1301_omap.c
> > @@ -31,16 +31,12 @@
> >  #include <linux/usb/otg.h>
> >  #include <linux/i2c.h>
> >  #include <linux/workqueue.h>
> > -
> > -#include <asm/irq.h>
> >  #include <asm/arch/usb.h>
> >
> > -
> >  #ifndef      DEBUG
> >  #undef       VERBOSE
> >  #endif
> >
> > -
> >  #define      DRIVER_VERSION  "24 August 2004"
> >  #define      DRIVER_NAME     (isp1301_driver.driver.name)
> >
> > @@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
> >  struct isp1301 {
> >       struct otg_transceiver  otg;
> >       struct i2c_client       *client;
> > -     struct i2c_client       c;
> >       void                    (*i2c_release)(struct device *dev);
> >
> > -     int                     irq;
> > -     int                     irq_type;
> > -
> >       u32                     last_otg_ctrl;
> >       unsigned                working:1;
> >
> > @@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
> >  /*-------------------------------------------------------------------------*/
> >
> >  /* only two addresses possible */
> > -#define      ISP_BASE                0x2c
> > -static unsigned short normal_i2c[] = {
> > -     ISP_BASE, ISP_BASE + 1,
> > -     I2C_CLIENT_END };
> > -
> > -I2C_CLIENT_INSMOD;
> > -
> >  static struct i2c_driver isp1301_driver;
> >
> >  /* smbus apis are used for portability */
> > @@ -1196,9 +1181,11 @@ static void isp1301_timer(unsigned long _isp)
> >
> >  static void isp1301_release(struct device *dev)
> >  {
> > -     struct isp1301  *isp;
> > +     struct i2c_client       *client;
> > +     struct isp1301          *isp;
> >
> > -     isp = container_of(dev, struct isp1301, c.dev);
> > +     client = container_of(dev, struct i2c_client, dev);
> > +     isp = i2c_get_clientdata(client);
>
> This seems to be a quite complex way to do:
>
>         isp = i2c_get_drvdata(dev);
>
> Or am I missing something?

My bad, thanks for getting this.

>
> >
> >       /* ugly -- i2c hijacks our memory hook to wait_for_completion() */
> >       if (isp->i2c_release)
> > @@ -1208,30 +1195,27 @@ static void isp1301_release(struct device *dev)
> >
> >  static struct isp1301 *the_transceiver;
> >
> > -static int isp1301_detach_client(struct i2c_client *i2c)
> > +static int __exit isp1301_remove(struct i2c_client *client)
> >  {
> > -     struct isp1301  *isp;
> > -
> > -     isp = container_of(i2c, struct isp1301, c);
> > +     struct isp1301  *isp = i2c_get_clientdata(client);
> >
> >       isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
> >       isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
> > -     free_irq(isp->irq, isp);
> > +
> > +     if (client->irq > 0)
> > +             free_irq(client->irq, isp);
> >  #ifdef       CONFIG_USB_OTG
> >       otg_unbind(isp);
> >  #endif
> > -     if (machine_is_omap_h2())
> > -             omap_free_gpio(2);
> > -
>
> Why?

Board specific code should go to board files. I'll try to find a
better way of doing this. Maybe using a private_data.
Ditto to all cases below.

>
> >       isp->timer.data = 0;
> >       set_bit(WORK_STOP, &isp->todo);
> >       del_timer_sync(&isp->timer);
> >       flush_scheduled_work();
> >
> > -     put_device(&i2c->dev);
> > +     put_device(&client->dev);
> >       the_transceiver = 0;
> >
> > -     return i2c_detach_client(i2c);
> > +     return 0;
> >  }
> >
> >  /*-------------------------------------------------------------------------*/
> > @@ -1301,9 +1285,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
> >
> >       power_up(isp);
> >
> > -     if (machine_is_omap_h2())
> > -             isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
> > -
>
> Where has this code gone? Why is it no longer needed?
>
> (Did you test you patch on OMAP H2?)

Can't remember anymore, but before applying these patches isp1301 was
crashing the kernel on H2 and H3.

>
>
> >       dev_info(&isp->client->dev, "A-Host sessions ok\n");
> >       isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
> >               INTR_ID_GND);
> > @@ -1481,11 +1462,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
> >  /*-------------------------------------------------------------------------*/
> >
> >  /* no error returns, they'd just make bus scanning stop */
> > -static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
> > +static int __init isp1301_probe(struct i2c_client *client)
> >  {
> >       int                     status;
> >       struct isp1301          *isp;
> > -     struct i2c_client       *i2c;
> >
> >       if (the_transceiver)
> >               return 0;
> > @@ -1498,46 +1478,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
> >       init_timer(&isp->timer);
> >       isp->timer.function = isp1301_timer;
> >       isp->timer.data = (unsigned long) isp;
> > -
> > -     isp->irq = -1;
> > -     isp->irq_type = 0;
> > -     isp->c.addr = address;
> > -     i2c_set_clientdata(&isp->c, isp);
> > -     isp->c.adapter = bus;
> > -     isp->c.driver = &isp1301_driver;
> > -     strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
> > -     isp->client = i2c = &isp->c;
> > +     isp->client = client;
> >
> >       /* if this is a true probe, verify the chip ... */
> > -     if (kind < 0) {
> > -             status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> > -             if (status != I2C_VENDOR_ID_PHILIPS) {
> > -                     dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
> > -                             address, status);
> > -                     goto fail1;
> > -             }
> > -             status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> > -             if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> > -                     dev_dbg(&bus->dev, "%d not isp1301, %d\n",
> > -                             address, status);
> > -                     goto fail1;
> > -             }
> > +     status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
> > +     if (status != I2C_VENDOR_ID_PHILIPS) {
> > +             dev_dbg(&client->dev, "not philips id: %d\n",
> > +                             status);
>
> Fits on a single line.

Good catch. Changing today.

>
> > +             goto fail1;
> > +     }
> > +     status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
> > +     if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
> > +             dev_dbg(&client->dev, "not isp1301, %d\n",
> > +                             status);
>
> Same here.
>
> > +             goto fail1;
> >       }
> >
> > -     status = i2c_attach_client(i2c);
> >       if (status < 0) {
> > -             dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
> > -                             DRIVER_NAME, address, status);
> > +             dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
> > +                             DRIVER_NAME, status);
>
> It doesn't make sense to remove the call to i2c_attach_client() but
> preserve the status check and debug message!

Hmm... sorry for that.
Changing.

>
>
> >  fail1:
> >               kfree(isp);
> >               return 0;
> >       }
> > -     isp->i2c_release = i2c->dev.release;
> > -     i2c->dev.release = isp1301_release;
> > +     isp->i2c_release = client->dev.release;
> > +     client->dev.release = isp1301_release;
> >
> >       /* initial development used chiprev 2.00 */
> > -     status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
> > -     dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
> > +     status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
> > +     dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
> >               status >> 8, status & 0xff);
> >
> >       /* make like power-on reset */
> > @@ -1558,40 +1527,25 @@ fail1:
> >  #ifdef       CONFIG_USB_OTG
> >       status = otg_bind(isp);
> >       if (status < 0) {
> > -             dev_dbg(&i2c->dev, "can't bind OTG\n");
> > +             dev_dbg(&client->dev, "can't bind OTG\n");
> >               goto fail2;
> >       }
> >  #endif
> >
> > -     if (machine_is_omap_h2()) {
> > -             /* full speed signaling by default */
> > -             isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
> > -                     MC1_SPEED_REG);
> > -             isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
> > -                     MC2_SPD_SUSP_CTRL);
> > -
> > -             /* IRQ wired at M14 */
> > -             omap_cfg_reg(M14_1510_GPIO2);
> > -             isp->irq = OMAP_GPIO_IRQ(2);
> > -             if (gpio_request(2, "isp1301") == 0)
> > -                     gpio_direction_input(2);
> > -             isp->irq_type = IRQF_TRIGGER_FALLING;
> > -     }
>
> Again, why would you remove this code?
>
> > -
> > -     isp->irq_type |= IRQF_SAMPLE_RANDOM;
> > -     status = request_irq(isp->irq, isp1301_irq,
> > -                     isp->irq_type, DRIVER_NAME, isp);
> > +     status = request_irq(client->irq, isp1301_irq,
> > +                     IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
> > +                     DRIVER_NAME, isp);
>
> When freeing the irq you first test that client->irq > 0, but when
> requesting it you do not? It's inconsistent.

I'll fix it.

>
> Also, the original code was passing different IRQF flags depending on
> the platform, your changes force the same mode for everyone. This
> doesn't look correct.

Maybe one other thing to come from a private_data.

>
> >       if (status < 0) {
> > -             dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
> > -                             isp->irq, status);
> > +             dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
> > +                             client->irq, status);
> >  #ifdef       CONFIG_USB_OTG
> >  fail2:
> >  #endif
> > -             i2c_detach_client(i2c);
> > +             i2c_detach_client(client);
> >               goto fail1;
> >       }
> >
> > -     isp->otg.dev = &isp->client->dev;
> > +     isp->otg.dev = &client->dev;
> >       isp->otg.label = DRIVER_NAME;
> >
> >       isp->otg.set_host = isp1301_set_host,
> > @@ -1608,43 +1562,42 @@ fail2:
> >       update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
> >       update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
> >  #endif
> > -
>
> Noise, please revert.

Reverting

>
>
> >       dump_regs(isp, __FUNCTION__);
> >
> >  #ifdef       VERBOSE
> >       mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
> > -     dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
> > +     dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
> >  #endif
> >
> >       status = otg_set_transceiver(&isp->otg);
> >       if (status < 0)
> > -             dev_err(&i2c->dev, "can't register transceiver, %d\n",
> > +             dev_err(&client->dev, "can't register transceiver, %d\n",
> >                       status);
> >
> >       return 0;
> >  }
> >
> > -static int isp1301_scan_bus(struct i2c_adapter *bus)
> > -{
> > -     if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
> > -                     | I2C_FUNC_SMBUS_READ_WORD_DATA))
> > -             return -EINVAL;
> > -     return i2c_probe(bus, &addr_data, isp1301_probe);
> > -}
> > -
> >  static struct i2c_driver isp1301_driver = {
> >       .driver = {
> >               .name   = "isp1301_omap",
> >       },
> > -     .attach_adapter = isp1301_scan_bus,
> > -     .detach_client  = isp1301_detach_client,
> > +     .probe  = isp1301_probe,
> > +     .remove = __exit_p(isp1301_remove),
> >  };
> >
> >  /*-------------------------------------------------------------------------*/
> >
> >  static int __init isp_init(void)
> >  {
> > -     return i2c_add_driver(&isp1301_driver);
> > +     int     status = -ENODEV;
> > +
> > +     printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);
>
> What's the point of printing a driver version that nobody bothers
> updating?
>
> Most i2c chip drivers keep quiet when loaded, they print a message when
> a device is actually found, as this driver is doing.

Ok. Changing

>
> > +
> > +     status = i2c_add_driver(&isp1301_driver);
> > +     if (status)
> > +             printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);
>
> This is extremely unlikely to happen, and if it did, i2c-core would
> already log a more informative error message, and insmod/modprobe as
> well. So you can just call i2c_add_driver() and be done with it (as
> the driver was originally doing.)

Ok. Changing


-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Tue, 22 Jan 2008 14:13:58 +0200, Felipe Balbi wrote:
> On Jan 22, 2008 2:01 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > On Thu,  3 Jan 2008 11:59:57 -0500, Felipe Balbi wrote:
> > > -     if (machine_is_omap_h2())
> > > -             omap_free_gpio(2);
> > > -
> >
> > Why?
> 
> Board specific code should go to board files. I'll try to find a
> better way of doing this. Maybe using a private_data.
> Ditto to all cases below.

I agree that board code should ideally not be there, however you can't
just drop it and hope nobody will notice. It needs to be done in such a
way that everything still works afterwards. IMHO it is better move such
changes to a later patch so as to not make this one needlessly complex.

> > (Did you test you patch on OMAP H2?)
> 
> Can't remember anymore, but before applying these patches isp1301 was
> crashing the kernel on H2 and H3.

That's bad, this should be fixed, if possible even before applying your
patches.

Thanks,
-- 
Jean Delvare

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

[Jean Delvare]

Hi Felipe,

Any update on this patch?

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

On Sat, Feb 23, 2008 at 6:52 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Felipe,
>
>  Any update on this patch?

Hi Jean,

sorry the delay...
I'm a bit crowded on omap3 usb support but I'll try to make it work
during the next week. BTW, I don't have access to omap 1610 nor 1710.
Do you anyone who could help me testing the patch ?

>
>  --
>  Jean Delvare
>



-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sat, 23 Feb 2008 19:01:22 +0200, Felipe Balbi wrote:
> On Sat, Feb 23, 2008 at 6:52 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > Hi Felipe,
> >
> >  Any update on this patch?
> 
> Hi Jean,
> 
> sorry the delay...
> I'm a bit crowded on omap3 usb support but I'll try to make it work
> during the next week. BTW, I don't have access to omap 1610 nor 1710.
> Do you anyone who could help me testing the patch ?

No, I'm not into embedded at all, sorry. Maybe Deepak (Cc'd) can help.
Otherwise try the linux-omap mailing list.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

On Sat, Feb 23, 2008 at 7:11 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Sat, 23 Feb 2008 19:01:22 +0200, Felipe Balbi wrote:
>  > On Sat, Feb 23, 2008 at 6:52 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>  > > Hi Felipe,
>  > >
>  > >  Any update on this patch?
>  >
>  > Hi Jean,
>  >
>  > sorry the delay...
>  > I'm a bit crowded on omap3 usb support but I'll try to make it work
>  > during the next week. BTW, I don't have access to omap 1610 nor 1710.
>  > Do you anyone who could help me testing the patch ?
>
>  No, I'm not into embedded at all, sorry. Maybe Deepak (Cc'd) can help.
>  Otherwise try the linux-omap mailing list.

Maybe, Dave do you have an H2 to try my isp patch?? :-p

>
>  --
>  Jean Delvare
>



-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

> Maybe, Dave do you have an H2 to try my isp patch?? :-p

I have one boxed up, but I'm a bit short on time and table space to
do anything with it.  Unboxing it for some stuff is on the agenda
for some point, but there's a bunch of other stuff ahead of it in
the queue ...

Why don't you forward me the patch in question, and if I get time
to do that H2 stuff before you do, you'll see the results.  :)

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Sat, Feb 23, 2008 at 7:53 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > Maybe, Dave do you have an H2 to try my isp patch?? :-p
>
>  I have one boxed up, but I'm a bit short on time and table space to
>  do anything with it.  Unboxing it for some stuff is on the agenda
>  for some point, but there's a bunch of other stuff ahead of it in
>  the queue ...
>
>  Why don't you forward me the patch in question, and if I get time
>  to do that H2 stuff before you do, you'll see the results.  :)

Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?
and patch2 needs some rework, to add some platform_data to get irq and
other stuff ;-)

>
>  - Dave
>
>



-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch --]
[-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch, Size: 7495 bytes --]

From 3ff6ace182c71c9c0cd8c6a45959bdecb08eb4b5 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <me@felipebalbi.com>
Date: Tue, 1 Jan 2008 22:11:35 -0500
Subject: [PATCH 1/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 1

Based on David Brownell's patch for tps65010, this patch
starts converting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 drivers/i2c/chips/isp1301_omap.c |   60 +++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index b767603..37e1403 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -49,7 +49,8 @@ MODULE_LICENSE("GPL");
 
 struct isp1301 {
 	struct otg_transceiver	otg;
-	struct i2c_client	client;
+	struct i2c_client	*client;
+	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
 	int			irq;
@@ -153,25 +154,25 @@ static struct i2c_driver isp1301_driver;
 static inline u8
 isp1301_get_u8(struct isp1301 *isp, u8 reg)
 {
-	return i2c_smbus_read_byte_data(&isp->client, reg + 0);
+	return i2c_smbus_read_byte_data(isp->client, reg + 0);
 }
 
 static inline int
 isp1301_get_u16(struct isp1301 *isp, u8 reg)
 {
-	return i2c_smbus_read_word_data(&isp->client, reg);
+	return i2c_smbus_read_word_data(isp->client, reg);
 }
 
 static inline int
 isp1301_set_bits(struct isp1301 *isp, u8 reg, u8 bits)
 {
-	return i2c_smbus_write_byte_data(&isp->client, reg + 0, bits);
+	return i2c_smbus_write_byte_data(isp->client, reg + 0, bits);
 }
 
 static inline int
 isp1301_clear_bits(struct isp1301 *isp, u8 reg, u8 bits)
 {
-	return i2c_smbus_write_byte_data(&isp->client, reg + 1, bits);
+	return i2c_smbus_write_byte_data(isp->client, reg + 1, bits);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -355,10 +356,10 @@ isp1301_defer_work(struct isp1301 *isp, int work)
 	int status;
 
 	if (isp && !test_and_set_bit(work, &isp->todo)) {
-		(void) get_device(&isp->client.dev);
+		(void) get_device(&isp->client->dev);
 		status = schedule_work(&isp->work);
 		if (!status && !isp->working)
-			dev_vdbg(&isp->client.dev,
+			dev_vdbg(&isp->client->dev,
 				"work item %d may be lost\n", work);
 	}
 }
@@ -1113,7 +1114,7 @@ isp1301_work(struct work_struct *work)
 		/* transfer state from otg engine to isp1301 */
 		if (test_and_clear_bit(WORK_UPDATE_ISP, &isp->todo)) {
 			otg_update_isp(isp);
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 #endif
 		/* transfer state from isp1301 to otg engine */
@@ -1121,7 +1122,7 @@ isp1301_work(struct work_struct *work)
 			u8		stat = isp1301_clear_latch(isp);
 
 			isp_update_otg(isp, stat);
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 
 		if (test_and_clear_bit(WORK_HOST_RESUME, &isp->todo)) {
@@ -1156,7 +1157,7 @@ isp1301_work(struct work_struct *work)
 			}
 			host_resume(isp);
 			// mdelay(10);
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 
 		if (test_and_clear_bit(WORK_TIMER, &isp->todo)) {
@@ -1165,15 +1166,15 @@ isp1301_work(struct work_struct *work)
 			if (!stop)
 				mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
 #endif
-			put_device(&isp->client.dev);
+			put_device(&isp->client->dev);
 		}
 
 		if (isp->todo)
-			dev_vdbg(&isp->client.dev,
+			dev_vdbg(&isp->client->dev,
 				"work done, todo = 0x%lx\n",
 				isp->todo);
 		if (stop) {
-			dev_dbg(&isp->client.dev, "stop\n");
+			dev_dbg(&isp->client->dev, "stop\n");
 			break;
 		}
 	} while (isp->todo);
@@ -1197,7 +1198,7 @@ static void isp1301_release(struct device *dev)
 {
 	struct isp1301	*isp;
 
-	isp = container_of(dev, struct isp1301, client.dev);
+	isp = container_of(dev, struct isp1301, c.dev);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1211,7 +1212,7 @@ static int isp1301_detach_client(struct i2c_client *i2c)
 {
 	struct isp1301	*isp;
 
-	isp = container_of(i2c, struct isp1301, client);
+	isp = container_of(i2c, struct isp1301, c);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
@@ -1263,7 +1264,7 @@ static int isp1301_otg_enable(struct isp1301 *isp)
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
 		INTR_VBUS_VLD | INTR_SESS_VLD | INTR_ID_GND);
 
-	dev_info(&isp->client.dev, "ready for dual-role USB ...\n");
+	dev_info(&isp->client->dev, "ready for dual-role USB ...\n");
 
 	return 0;
 }
@@ -1288,7 +1289,7 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 #ifdef	CONFIG_USB_OTG
 	isp->otg.host = host;
-	dev_dbg(&isp->client.dev, "registered host\n");
+	dev_dbg(&isp->client->dev, "registered host\n");
 	host_suspend(isp);
 	if (isp->otg.gadget)
 		return isp1301_otg_enable(isp);
@@ -1303,7 +1304,7 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 	if (machine_is_omap_h2())
 		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
 
-	dev_info(&isp->client.dev, "A-Host sessions ok\n");
+	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
@@ -1321,7 +1322,7 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 	return 0;
 
 #else
-	dev_dbg(&isp->client.dev, "host sessions not allowed\n");
+	dev_dbg(&isp->client->dev, "host sessions not allowed\n");
 	return -EINVAL;
 #endif
 
@@ -1347,7 +1348,7 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 
 #ifdef	CONFIG_USB_OTG
 	isp->otg.gadget = gadget;
-	dev_dbg(&isp->client.dev, "registered gadget\n");
+	dev_dbg(&isp->client->dev, "registered gadget\n");
 	/* gadget driver may be suspended until vbus_connect () */
 	if (isp->otg.host)
 		return isp1301_otg_enable(isp);
@@ -1370,7 +1371,7 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 		INTR_SESS_VLD);
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
 		INTR_VBUS_VLD);
-	dev_info(&isp->client.dev, "B-Peripheral sessions ok\n");
+	dev_info(&isp->client->dev, "B-Peripheral sessions ok\n");
 	dump_regs(isp, __FUNCTION__);
 
 	/* If this has a Mini-AB connector, this mode is highly
@@ -1383,7 +1384,7 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 	return 0;
 
 #else
-	dev_dbg(&isp->client.dev, "peripheral sessions not allowed\n");
+	dev_dbg(&isp->client->dev, "peripheral sessions not allowed\n");
 	return -EINVAL;
 #endif
 }
@@ -1499,12 +1500,13 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	isp->timer.data = (unsigned long) isp;
 
 	isp->irq = -1;
-	isp->client.addr = address;
-	i2c_set_clientdata(&isp->client, isp);
-	isp->client.adapter = bus;
-	isp->client.driver = &isp1301_driver;
-	strlcpy(isp->client.name, DRIVER_NAME, I2C_NAME_SIZE);
-	i2c = &isp->client;
+	isp->irq_type = 0;
+	isp->c.addr = address;
+	i2c_set_clientdata(&isp->c, isp);
+	isp->c.adapter = bus;
+	isp->c.driver = &isp1301_driver;
+	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
+	isp->client = i2c = &isp->c;
 
 	/* if this is a true probe, verify the chip ... */
 	if (kind < 0) {
@@ -1589,7 +1591,7 @@ fail2:
 		goto fail1;
 	}
 
-	isp->otg.dev = &isp->client.dev;
+	isp->otg.dev = &isp->client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
-- 
1.5.4.rc1.21.g0e545-dirty


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch --]
[-- Type: text/x-patch; name=0002-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-par.patch, Size: 11022 bytes --]

From d816adfb6053267485bc9c3cb70c032a3ebb9638 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <me@felipebalbi.com>
Date: Tue, 1 Jan 2008 23:00:18 -0500
Subject: [PATCH 2/2] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

Based on David Brownell's patch for tps65010, this patch
finish conversting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 arch/arm/mach-omap1/board-h2.c   |    6 ++-
 arch/arm/mach-omap1/board-h3.c   |    6 ++-
 arch/arm/mach-omap2/board-h4.c   |   12 +++
 drivers/i2c/chips/isp1301_omap.c |  149 +++++++++++++-------------------------
 4 files changed, 73 insertions(+), 100 deletions(-)

diff --git a/arch/arm/mach-omap1/board-h2.c b/arch/arm/mach-omap1/board-h2.c
index 1306812..0307f50 100644
--- a/arch/arm/mach-omap1/board-h2.c
+++ b/arch/arm/mach-omap1/board-h2.c
@@ -350,8 +350,12 @@ static struct i2c_board_info __initdata h2_i2c_board_info[] = {
 		.type		= "tps65010",
 		.irq		= OMAP_GPIO_IRQ(58),
 	},
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(2),
+	},
 	/* TODO when driver support is ready:
-	 *  - isp1301 OTG transceiver
 	 *  - optional ov9640 camera sensor at 0x30
 	 *  - pcf9754 for aGPS control
 	 *  - ... etc
diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index 4f84ae2..71e285a 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -463,8 +463,12 @@ static struct i2c_board_info __initdata h3_i2c_board_info[] = {
 		.type		= "tps65013",
 		/* .irq		= OMAP_GPIO_IRQ(??), */
 	},
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(14),
+	},
 	/* TODO when driver support is ready:
-	 *  - isp1301 OTG transceiver
 	 *  - optional ov9640 camera sensor at 0x30
 	 *  - ...
 	 */
diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
index f125f43..33ff80b 100644
--- a/arch/arm/mach-omap2/board-h4.c
+++ b/arch/arm/mach-omap2/board-h4.c
@@ -301,6 +301,15 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct i2c_board_info __initdata h4_i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(125),
+	},
+};
+
+
 static void __init omap_h4_init(void)
 {
 	/*
@@ -321,6 +330,9 @@ static void __init omap_h4_init(void)
 	}
 #endif
 
+	i2c_register_board_info(1, h4_i2c_board_info,
+			ARRAY_SIZE(h4_i2c_board_info));
+
 	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
 	omap_board_config = h4_config;
 	omap_board_config_size = ARRAY_SIZE(h4_config);
diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 37e1403..c7a7c48 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -31,16 +31,12 @@
 #include <linux/usb/otg.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
-
-#include <asm/irq.h>
 #include <asm/arch/usb.h>
 
-
 #ifndef	DEBUG
 #undef	VERBOSE
 #endif
 
-
 #define	DRIVER_VERSION	"24 August 2004"
 #define	DRIVER_NAME	(isp1301_driver.driver.name)
 
@@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
 struct isp1301 {
 	struct otg_transceiver	otg;
 	struct i2c_client	*client;
-	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
-	int			irq;
-	int			irq_type;
-
 	u32			last_otg_ctrl;
 	unsigned		working:1;
 
@@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
 /*-------------------------------------------------------------------------*/
 
 /* only two addresses possible */
-#define	ISP_BASE		0x2c
-static unsigned short normal_i2c[] = {
-	ISP_BASE, ISP_BASE + 1,
-	I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
 static struct i2c_driver isp1301_driver;
 
 /* smbus apis are used for portability */
@@ -1196,9 +1181,11 @@ static void isp1301_timer(unsigned long _isp)
 
 static void isp1301_release(struct device *dev)
 {
-	struct isp1301	*isp;
+	struct i2c_client	*client;
+	struct isp1301		*isp;
 
-	isp = container_of(dev, struct isp1301, c.dev);
+	client = container_of(dev, struct i2c_client, dev);
+	isp = i2c_get_clientdata(client);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1208,30 +1195,27 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int isp1301_detach_client(struct i2c_client *i2c)
+static int __exit isp1301_remove(struct i2c_client *client)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(i2c, struct isp1301, c);
+	struct isp1301	*isp = i2c_get_clientdata(client);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
-	free_irq(isp->irq, isp);
+
+	if (client->irq > 0)
+		free_irq(client->irq, isp);
 #ifdef	CONFIG_USB_OTG
 	otg_unbind(isp);
 #endif
-	if (machine_is_omap_h2())
-		omap_free_gpio(2);
-
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
 	flush_scheduled_work();
 
-	put_device(&i2c->dev);
+	put_device(&client->dev);
 	the_transceiver = 0;
 
-	return i2c_detach_client(i2c);
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1301,9 +1285,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 	power_up(isp);
 
-	if (machine_is_omap_h2())
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
-
 	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
@@ -1481,11 +1462,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 /*-------------------------------------------------------------------------*/
 
 /* no error returns, they'd just make bus scanning stop */
-static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
+static int __init isp1301_probe(struct i2c_client *client)
 {
 	int			status;
 	struct isp1301		*isp;
-	struct i2c_client	*i2c;
 
 	if (the_transceiver)
 		return 0;
@@ -1498,46 +1478,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
-
-	isp->irq = -1;
-	isp->irq_type = 0;
-	isp->c.addr = address;
-	i2c_set_clientdata(&isp->c, isp);
-	isp->c.adapter = bus;
-	isp->c.driver = &isp1301_driver;
-	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
-	isp->client = i2c = &isp->c;
+	isp->client = client;
 
 	/* if this is a true probe, verify the chip ... */
-	if (kind < 0) {
-		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
-		if (status != I2C_VENDOR_ID_PHILIPS) {
-			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
-				address, status);
-			goto fail1;
-		}
-		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
-		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
-			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
-				address, status);
-			goto fail1;
-		}
+	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
+	if (status != I2C_VENDOR_ID_PHILIPS) {
+		dev_dbg(&client->dev, "not philips id: %d\n",
+				status);
+		goto fail1;
+	}
+	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
+	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
+		dev_dbg(&client->dev, "not isp1301, %d\n",
+				status);
+		goto fail1;
 	}
 
-	status = i2c_attach_client(i2c);
 	if (status < 0) {
-		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
-				DRIVER_NAME, address, status);
+		dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
+				DRIVER_NAME, status);
 fail1:
 		kfree(isp);
 		return 0;
 	}
-	isp->i2c_release = i2c->dev.release;
-	i2c->dev.release = isp1301_release;
+	isp->i2c_release = client->dev.release;
+	client->dev.release = isp1301_release;
 
 	/* initial development used chiprev 2.00 */
-	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
-	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
+	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
+	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
 		status >> 8, status & 0xff);
 
 	/* make like power-on reset */
@@ -1558,40 +1527,25 @@ fail1:
 #ifdef	CONFIG_USB_OTG
 	status = otg_bind(isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't bind OTG\n");
+		dev_dbg(&client->dev, "can't bind OTG\n");
 		goto fail2;
 	}
 #endif
 
-	if (machine_is_omap_h2()) {
-		/* full speed signaling by default */
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
-			MC1_SPEED_REG);
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
-			MC2_SPD_SUSP_CTRL);
-
-		/* IRQ wired at M14 */
-		omap_cfg_reg(M14_1510_GPIO2);
-		isp->irq = OMAP_GPIO_IRQ(2);
-		if (gpio_request(2, "isp1301") == 0)
-			gpio_direction_input(2);
-		isp->irq_type = IRQF_TRIGGER_FALLING;
-	}
-
-	isp->irq_type |= IRQF_SAMPLE_RANDOM;
-	status = request_irq(isp->irq, isp1301_irq,
-			isp->irq_type, DRIVER_NAME, isp);
+	status = request_irq(client->irq, isp1301_irq,
+			IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
+			DRIVER_NAME, isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
-				isp->irq, status);
+		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
+				client->irq, status);
 #ifdef	CONFIG_USB_OTG
 fail2:
 #endif
-		i2c_detach_client(i2c);
+		i2c_detach_client(client);
 		goto fail1;
 	}
 
-	isp->otg.dev = &isp->client->dev;
+	isp->otg.dev = &client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
@@ -1608,43 +1562,42 @@ fail2:
 	update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
 	update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
 #endif
-
 	dump_regs(isp, __FUNCTION__);
 
 #ifdef	VERBOSE
 	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
-	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
+	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
 #endif
 
 	status = otg_set_transceiver(&isp->otg);
 	if (status < 0)
-		dev_err(&i2c->dev, "can't register transceiver, %d\n",
+		dev_err(&client->dev, "can't register transceiver, %d\n",
 			status);
 
 	return 0;
 }
 
-static int isp1301_scan_bus(struct i2c_adapter *bus)
-{
-	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
-			| I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EINVAL;
-	return i2c_probe(bus, &addr_data, isp1301_probe);
-}
-
 static struct i2c_driver isp1301_driver = {
 	.driver = {
 		.name	= "isp1301_omap",
 	},
-	.attach_adapter	= isp1301_scan_bus,
-	.detach_client	= isp1301_detach_client,
+	.probe	= isp1301_probe,
+	.remove	= __exit_p(isp1301_remove),
 };
 
 /*-------------------------------------------------------------------------*/
 
 static int __init isp_init(void)
 {
-	return i2c_add_driver(&isp1301_driver);
+	int	status = -ENODEV;
+
+	printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);
+
+	status = i2c_add_driver(&isp1301_driver);
+	if (status)
+		printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);
+
+	return status;
 }
 module_init(isp_init);
 
-- 
1.5.4.rc1.21.g0e545-dirty


[-- Attachment #4: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 23 February 2008, Felipe Balbi wrote:
> Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?

But not yet in the I2C merge queue??


> and patch2 needs some rework, to add some platform_data to get irq and
> other stuff ;-)

Is patch 2 known to work on omap2 and such, or is the only question whether
the omap1 support has broken?

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 985 bytes --]

On Sat, Feb 23, 2008 at 8:10 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Saturday 23 February 2008, Felipe Balbi wrote:
>  > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?
>
>  But not yet in the I2C merge queue??

not merged, yeah

>
>
>
>  > and patch2 needs some rework, to add some platform_data to get irq and
>  > other stuff ;-)
>
>  Is patch 2 known to work on omap2 and such, or is the only question whether
>  the omap1 support has broken?

omap1 support and some comments from Jean which you can get on this thread.

BTW, the patch is not applying anymore after tony's omap merge.
I have an updated version attached here (of patch2), I'll try to do
some more work and keep you Cc:ed

PS: i just generated this patch, but it is part 2 of isp and should be
on top of patch1 from previous email ;-)

>
>  - Dave
>
>



-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --]
[-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 10313 bytes --]

From 0fe387b0e1328942a5aa8590e053aa58a261dad8 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <me@felipebalbi.com>
Date: Tue, 1 Jan 2008 23:00:18 -0500
Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

Based on David Brownell's patch for tps65010, this patch
finish conversting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 arch/arm/mach-omap1/board-h3.c   |    8 ++
 arch/arm/mach-omap2/board-h4.c   |   11 +++
 drivers/i2c/chips/isp1301_omap.c |  147 ++++++++++++-------------------------
 3 files changed, 67 insertions(+), 99 deletions(-)

diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index 6fc5168..7c1b966 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = {
 	},
 };
 
+static struct i2c_board_info __initdata h3_i2c_board_info[] = {
+       {
+               I2C_BOARD_INFO("isp1301_omap", 0x2d),
+               .type           = "isp1301_omap",
+               .irq            = OMAP_GPIO_IRQ(14),
+       },
+};
+
 #define H3_NAND_RB_GPIO_PIN	10
 
 static int nand_dev_ready(struct omap_nand_platform_data *data)
diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
index f125f43..d469c63 100644
--- a/arch/arm/mach-omap2/board-h4.c
+++ b/arch/arm/mach-omap2/board-h4.c
@@ -301,6 +301,14 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct i2c_board_info __initdata h4_i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(125),
+	},
+};
+
 static void __init omap_h4_init(void)
 {
 	/*
@@ -321,6 +329,9 @@ static void __init omap_h4_init(void)
 	}
 #endif
 
+	i2c_register_board_info(1, h4_i2c_board_info,
+			ARRAY_SIZE(h4_i2c_board_info));
+
 	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
 	omap_board_config = h4_config;
 	omap_board_config_size = ARRAY_SIZE(h4_config);
diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 92a9c01..0981620 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -31,16 +31,12 @@
 #include <linux/usb/otg.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
-
-#include <asm/irq.h>
 #include <asm/arch/usb.h>
 
-
 #ifndef	DEBUG
 #undef	VERBOSE
 #endif
 
-
 #define	DRIVER_VERSION	"24 August 2004"
 #define	DRIVER_NAME	(isp1301_driver.driver.name)
 
@@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
 struct isp1301 {
 	struct otg_transceiver	otg;
 	struct i2c_client	*client;
-	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
-	int			irq;
-	int			irq_type;
-
 	u32			last_otg_ctrl;
 	unsigned		working:1;
 
@@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
 /*-------------------------------------------------------------------------*/
 
 /* only two addresses possible */
-#define	ISP_BASE		0x2c
-static unsigned short normal_i2c[] = {
-	ISP_BASE, ISP_BASE + 1,
-	I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
 static struct i2c_driver isp1301_driver;
 
 /* smbus apis are used for portability */
@@ -1190,9 +1175,7 @@ static void isp1301_timer(unsigned long _isp)
 
 static void isp1301_release(struct device *dev)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(dev, struct isp1301, c.dev);
+	struct isp1301		*isp = dev_get_drvdata(dev);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1202,30 +1185,27 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int isp1301_detach_client(struct i2c_client *i2c)
+static int __exit isp1301_remove(struct i2c_client *client)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(i2c, struct isp1301, c);
+	struct isp1301	*isp = i2c_get_clientdata(client);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
-	free_irq(isp->irq, isp);
+
+	if (client->irq > 0)
+		free_irq(client->irq, isp);
 #ifdef	CONFIG_USB_OTG
 	otg_unbind(isp);
 #endif
-	if (machine_is_omap_h2())
-		omap_free_gpio(2);
-
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
 	flush_scheduled_work();
 
-	put_device(&i2c->dev);
+	put_device(&client->dev);
 	the_transceiver = 0;
 
-	return i2c_detach_client(i2c);
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1295,9 +1275,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 	power_up(isp);
 
-	if (machine_is_omap_h2())
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
-
 	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
@@ -1475,11 +1452,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 /*-------------------------------------------------------------------------*/
 
 /* no error returns, they'd just make bus scanning stop */
-static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
+static int __init isp1301_probe(struct i2c_client *client)
 {
 	int			status;
 	struct isp1301		*isp;
-	struct i2c_client	*i2c;
 
 	if (the_transceiver)
 		return 0;
@@ -1492,46 +1468,35 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
-
-	isp->irq = -1;
-	isp->irq_type = 0;
-	isp->c.addr = address;
-	i2c_set_clientdata(&isp->c, isp);
-	isp->c.adapter = bus;
-	isp->c.driver = &isp1301_driver;
-	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
-	isp->client = i2c = &isp->c;
+	isp->client = client;
 
 	/* if this is a true probe, verify the chip ... */
-	if (kind < 0) {
-		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
-		if (status != I2C_VENDOR_ID_PHILIPS) {
-			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
-				address, status);
-			goto fail1;
-		}
-		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
-		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
-			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
-				address, status);
-			goto fail1;
-		}
+	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
+	if (status != I2C_VENDOR_ID_PHILIPS) {
+		dev_dbg(&client->dev, "not philips id: %d\n",
+				status);
+		goto fail1;
+	}
+	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
+	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
+		dev_dbg(&client->dev, "not isp1301, %d\n",
+				status);
+		goto fail1;
 	}
 
-	status = i2c_attach_client(i2c);
 	if (status < 0) {
-		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
-				DRIVER_NAME, address, status);
+		dev_dbg(&client->dev, "can't attach %s to device, err %d\n",
+				DRIVER_NAME, status);
 fail1:
 		kfree(isp);
 		return 0;
 	}
-	isp->i2c_release = i2c->dev.release;
-	i2c->dev.release = isp1301_release;
+	isp->i2c_release = client->dev.release;
+	client->dev.release = isp1301_release;
 
 	/* initial development used chiprev 2.00 */
-	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
-	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
+	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
+	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
 		status >> 8, status & 0xff);
 
 	/* make like power-on reset */
@@ -1552,40 +1517,25 @@ fail1:
 #ifdef	CONFIG_USB_OTG
 	status = otg_bind(isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't bind OTG\n");
+		dev_dbg(&client->dev, "can't bind OTG\n");
 		goto fail2;
 	}
 #endif
 
-	if (machine_is_omap_h2()) {
-		/* full speed signaling by default */
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
-			MC1_SPEED_REG);
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
-			MC2_SPD_SUSP_CTRL);
-
-		/* IRQ wired at M14 */
-		omap_cfg_reg(M14_1510_GPIO2);
-		isp->irq = OMAP_GPIO_IRQ(2);
-		if (gpio_request(2, "isp1301") == 0)
-			gpio_direction_input(2);
-		isp->irq_type = IRQF_TRIGGER_FALLING;
-	}
-
-	isp->irq_type |= IRQF_SAMPLE_RANDOM;
-	status = request_irq(isp->irq, isp1301_irq,
-			isp->irq_type, DRIVER_NAME, isp);
+	status = request_irq(client->irq, isp1301_irq,
+			IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
+			DRIVER_NAME, isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
-				isp->irq, status);
+		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
+				client->irq, status);
 #ifdef	CONFIG_USB_OTG
 fail2:
 #endif
-		i2c_detach_client(i2c);
+		i2c_detach_client(client);
 		goto fail1;
 	}
 
-	isp->otg.dev = &isp->client->dev;
+	isp->otg.dev = &client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
@@ -1602,43 +1552,42 @@ fail2:
 	update_otg1(isp, isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE));
 	update_otg2(isp, isp1301_get_u8(isp, ISP1301_OTG_STATUS));
 #endif
-
 	dump_regs(isp, __FUNCTION__);
 
 #ifdef	VERBOSE
 	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
-	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
+	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
 #endif
 
 	status = otg_set_transceiver(&isp->otg);
 	if (status < 0)
-		dev_err(&i2c->dev, "can't register transceiver, %d\n",
+		dev_err(&client->dev, "can't register transceiver, %d\n",
 			status);
 
 	return 0;
 }
 
-static int isp1301_scan_bus(struct i2c_adapter *bus)
-{
-	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
-			| I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EINVAL;
-	return i2c_probe(bus, &addr_data, isp1301_probe);
-}
-
 static struct i2c_driver isp1301_driver = {
 	.driver = {
 		.name	= "isp1301_omap",
 	},
-	.attach_adapter	= isp1301_scan_bus,
-	.detach_client	= isp1301_detach_client,
+	.probe	= isp1301_probe,
+	.remove	= __exit_p(isp1301_remove),
 };
 
 /*-------------------------------------------------------------------------*/
 
 static int __init isp_init(void)
 {
-	return i2c_add_driver(&isp1301_driver);
+	int	status = -ENODEV;
+
+	printk(KERN_INFO "%s: version %s\n", DRIVER_NAME, DRIVER_VERSION);
+
+	status = i2c_add_driver(&isp1301_driver);
+	if (status)
+		printk(KERN_ERR "%s failed to probe\n", DRIVER_NAME);
+
+	return status;
 }
 module_init(isp_init);
 
-- 
1.5.4.34.g053d9


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

Hi Dave,

On Sat, Feb 23, 2008 at 9:00 PM, Felipe Balbi
<felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:
> On Sat, Feb 23, 2008 at 8:10 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
>  > On Saturday 23 February 2008, Felipe Balbi wrote:
>  >  > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?
>  >
>  >  But not yet in the I2C merge queue??
>
>  not merged, yeah
>
>
>  >
>  >
>  >
>  >  > and patch2 needs some rework, to add some platform_data to get irq and
>  >  > other stuff ;-)
>  >
>  >  Is patch 2 known to work on omap2 and such, or is the only question whether
>  >  the omap1 support has broken?
>
>  omap1 support and some comments from Jean which you can get on this thread.
>
>  BTW, the patch is not applying anymore after tony's omap merge.
>  I have an updated version attached here (of patch2), I'll try to do
>  some more work and keep you Cc:ed
>
>  PS: i just generated this patch, but it is part 2 of isp and should be
>  on top of patch1 from previous email ;-)

Sorry replying to myself, but I have a better with most of comments
from Jean applied.
Missing irq flags, mostly.

It's attached

-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --]
[-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 9727 bytes --]

From 9024a8844c1fdfd65a23e9e66eb6fbca497a14a2 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <me@felipebalbi.com>
Date: Tue, 1 Jan 2008 23:00:18 -0500
Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

Based on David Brownell's patch for tps65010, this patch
finish conversting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 arch/arm/mach-omap1/board-h3.c   |    8 ++
 arch/arm/mach-omap2/board-h4.c   |   11 +++
 drivers/i2c/chips/isp1301_omap.c |  134 ++++++++++---------------------------
 3 files changed, 55 insertions(+), 98 deletions(-)

diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index 6fc5168..7c1b966 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = {
 	},
 };
 
+static struct i2c_board_info __initdata h3_i2c_board_info[] = {
+       {
+               I2C_BOARD_INFO("isp1301_omap", 0x2d),
+               .type           = "isp1301_omap",
+               .irq            = OMAP_GPIO_IRQ(14),
+       },
+};
+
 #define H3_NAND_RB_GPIO_PIN	10
 
 static int nand_dev_ready(struct omap_nand_platform_data *data)
diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
index f125f43..d469c63 100644
--- a/arch/arm/mach-omap2/board-h4.c
+++ b/arch/arm/mach-omap2/board-h4.c
@@ -301,6 +301,14 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct i2c_board_info __initdata h4_i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(125),
+	},
+};
+
 static void __init omap_h4_init(void)
 {
 	/*
@@ -321,6 +329,9 @@ static void __init omap_h4_init(void)
 	}
 #endif
 
+	i2c_register_board_info(1, h4_i2c_board_info,
+			ARRAY_SIZE(h4_i2c_board_info));
+
 	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
 	omap_board_config = h4_config;
 	omap_board_config_size = ARRAY_SIZE(h4_config);
diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 92a9c01..6e84995 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -31,16 +31,12 @@
 #include <linux/usb/otg.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
-
-#include <asm/irq.h>
 #include <asm/arch/usb.h>
 
-
 #ifndef	DEBUG
 #undef	VERBOSE
 #endif
 
-
 #define	DRIVER_VERSION	"24 August 2004"
 #define	DRIVER_NAME	(isp1301_driver.driver.name)
 
@@ -50,12 +46,8 @@ MODULE_LICENSE("GPL");
 struct isp1301 {
 	struct otg_transceiver	otg;
 	struct i2c_client	*client;
-	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
-	int			irq;
-	int			irq_type;
-
 	u32			last_otg_ctrl;
 	unsigned		working:1;
 
@@ -140,13 +132,6 @@ static inline void notresponding(struct isp1301 *isp)
 /*-------------------------------------------------------------------------*/
 
 /* only two addresses possible */
-#define	ISP_BASE		0x2c
-static unsigned short normal_i2c[] = {
-	ISP_BASE, ISP_BASE + 1,
-	I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
 static struct i2c_driver isp1301_driver;
 
 /* smbus apis are used for portability */
@@ -1190,9 +1175,7 @@ static void isp1301_timer(unsigned long _isp)
 
 static void isp1301_release(struct device *dev)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(dev, struct isp1301, c.dev);
+	struct isp1301		*isp = dev_get_drvdata(dev);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1202,30 +1185,27 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int isp1301_detach_client(struct i2c_client *i2c)
+static int __exit isp1301_remove(struct i2c_client *client)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(i2c, struct isp1301, c);
+	struct isp1301	*isp = i2c_get_clientdata(client);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
-	free_irq(isp->irq, isp);
+
+	if (client->irq > 0)
+		free_irq(client->irq, isp);
 #ifdef	CONFIG_USB_OTG
 	otg_unbind(isp);
 #endif
-	if (machine_is_omap_h2())
-		omap_free_gpio(2);
-
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
 	flush_scheduled_work();
 
-	put_device(&i2c->dev);
+	put_device(&client->dev);
 	the_transceiver = 0;
 
-	return i2c_detach_client(i2c);
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1295,9 +1275,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 	power_up(isp);
 
-	if (machine_is_omap_h2())
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
-
 	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
@@ -1475,11 +1452,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 /*-------------------------------------------------------------------------*/
 
 /* no error returns, they'd just make bus scanning stop */
-static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
+static int __init isp1301_probe(struct i2c_client *client)
 {
 	int			status;
 	struct isp1301		*isp;
-	struct i2c_client	*i2c;
 
 	if (the_transceiver)
 		return 0;
@@ -1492,46 +1468,30 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
-
-	isp->irq = -1;
-	isp->irq_type = 0;
-	isp->c.addr = address;
-	i2c_set_clientdata(&isp->c, isp);
-	isp->c.adapter = bus;
-	isp->c.driver = &isp1301_driver;
-	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
-	isp->client = i2c = &isp->c;
+	isp->client = client;
 
 	/* if this is a true probe, verify the chip ... */
-	if (kind < 0) {
-		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
-		if (status != I2C_VENDOR_ID_PHILIPS) {
-			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
-				address, status);
-			goto fail1;
-		}
-		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
-		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
-			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
-				address, status);
-			goto fail1;
-		}
+	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
+	if (status != I2C_VENDOR_ID_PHILIPS) {
+		dev_dbg(&client->dev, "not philips id: %d\n", status);
+		goto fail1;
+	}
+	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
+	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
+		dev_dbg(&client->dev, "not isp1301, %d\n", status);
+		goto fail1;
 	}
 
-	status = i2c_attach_client(i2c);
-	if (status < 0) {
-		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
-				DRIVER_NAME, address, status);
 fail1:
 		kfree(isp);
 		return 0;
 	}
-	isp->i2c_release = i2c->dev.release;
-	i2c->dev.release = isp1301_release;
+	isp->i2c_release = client->dev.release;
+	client->dev.release = isp1301_release;
 
 	/* initial development used chiprev 2.00 */
-	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
-	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
+	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
+	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
 		status >> 8, status & 0xff);
 
 	/* make like power-on reset */
@@ -1552,40 +1512,26 @@ fail1:
 #ifdef	CONFIG_USB_OTG
 	status = otg_bind(isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't bind OTG\n");
+		dev_dbg(&client->dev, "can't bind OTG\n");
 		goto fail2;
 	}
 #endif
 
-	if (machine_is_omap_h2()) {
-		/* full speed signaling by default */
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
-			MC1_SPEED_REG);
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
-			MC2_SPD_SUSP_CTRL);
-
-		/* IRQ wired at M14 */
-		omap_cfg_reg(M14_1510_GPIO2);
-		isp->irq = OMAP_GPIO_IRQ(2);
-		if (gpio_request(2, "isp1301") == 0)
-			gpio_direction_input(2);
-		isp->irq_type = IRQF_TRIGGER_FALLING;
-	}
-
-	isp->irq_type |= IRQF_SAMPLE_RANDOM;
-	status = request_irq(isp->irq, isp1301_irq,
-			isp->irq_type, DRIVER_NAME, isp);
+	if (client->irq > 0)
+		status = request_irq(client->irq, isp1301_irq,
+				IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
+				DRIVER_NAME, isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
-				isp->irq, status);
+		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
+				client->irq, status);
 #ifdef	CONFIG_USB_OTG
 fail2:
 #endif
-		i2c_detach_client(i2c);
+		i2c_detach_client(client);
 		goto fail1;
 	}
 
-	isp->otg.dev = &isp->client->dev;
+	isp->otg.dev = &client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
@@ -1607,31 +1553,23 @@ fail2:
 
 #ifdef	VERBOSE
 	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
-	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
+	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
 #endif
 
 	status = otg_set_transceiver(&isp->otg);
 	if (status < 0)
-		dev_err(&i2c->dev, "can't register transceiver, %d\n",
+		dev_err(&client->dev, "can't register transceiver, %d\n",
 			status);
 
 	return 0;
 }
 
-static int isp1301_scan_bus(struct i2c_adapter *bus)
-{
-	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
-			| I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EINVAL;
-	return i2c_probe(bus, &addr_data, isp1301_probe);
-}
-
 static struct i2c_driver isp1301_driver = {
 	.driver = {
 		.name	= "isp1301_omap",
 	},
-	.attach_adapter	= isp1301_scan_bus,
-	.detach_client	= isp1301_detach_client,
+	.probe	= isp1301_probe,
+	.remove	= __exit_p(isp1301_remove),
 };
 
 /*-------------------------------------------------------------------------*/
-- 
1.5.4.34.g053d9


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote:
> On Saturday 23 February 2008, Felipe Balbi wrote:
> > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?

Yes, except that my copy has "isp->irq_type = 0;" removed.

> But not yet in the I2C merge queue??

I was waiting for the second part. The first part doesn't make much
sense alone. I could add the first part to my i2c tree, but I can't
merge it upstream without the second part.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sat, 23 Feb 2008 21:08:04 +0200, Felipe Balbi wrote:
> Sorry replying to myself, but I have a better with most of comments
> from Jean applied.
> Missing irq flags, mostly.
> 
> It's attached

This update adds this bug:

> +	if (client->irq > 0)
> +		status = request_irq(client->irq, isp1301_irq,
> +				IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
> +				DRIVER_NAME, isp);
>  	if (status < 0) {

This test should be moved inside the "if (client->irq > 0)".

> -		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
> -				isp->irq, status);
> +		dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
> +				client->irq, status);
>  #ifdef	CONFIG_USB_OTG
>  fail2:
>  #endif
> -		i2c_detach_client(i2c);
> +		i2c_detach_client(client);

Also note (I missed it in my previous review): don't call
i2c_detach_client() for a new-syle driver!

>  		goto fail1;
>  	}

You addressed some of the issues I raised (thanks) but not all, so I
still can't include this patch into my i2c tree for now.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 23 February 2008, Jean Delvare wrote:
> On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote:
> > On Saturday 23 February 2008, Felipe Balbi wrote:
> > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?
> 
> Yes, except that my copy has "isp->irq_type = 0;" removed.

>From a bzeroed structure?  Then I won't worry about that difference...


> > But not yet in the I2C merge queue??
> 
> I was waiting for the second part. The first part doesn't make much
> sense alone. I could add the first part to my i2c tree, but I can't
> merge it upstream without the second part.

That makes no sense to me.  If the first part is OK, it's OK to
merge without the second part.

Even if you don't _plan_ to merge it soon, it'd be less confusing
to have that in your queue ... say, towards the end, below a line
that indicates a scheduled merge point.  (FWIW that's not unlike
Andrew does with MM.)

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sat, 23 Feb 2008 11:53:03 -0800, David Brownell wrote:
> On Saturday 23 February 2008, Jean Delvare wrote:
> > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote:
> > > On Saturday 23 February 2008, Felipe Balbi wrote:
> > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right Jean?
> > 
> > Yes, except that my copy has "isp->irq_type = 0;" removed.
> 
> From a bzeroed structure?  Then I won't worry about that difference...

I removed this statement exactly because the structure is kzalloc'd.
You don't have to worry as a tester. I do worry as the i2c subsystem
maintainer, because I'd like Felipe to base his second patch on the
"right" first patch, so that it applies properly to my tree.

Felipe, my version of the first patch is available here:
http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-isp1301_omap-convert-to-new-style-1.patch

Please make sure that the next update to the second part applies
properly on top of it.

> > > But not yet in the I2C merge queue??
> > 
> > I was waiting for the second part. The first part doesn't make much
> > sense alone. I could add the first part to my i2c tree, but I can't
> > merge it upstream without the second part.
> 
> That makes no sense to me.  If the first part is OK, it's OK to
> merge without the second part.

And if the second part never makes it, we end up with garbage in the
upstream kernel. Thanks but no thanks.

> Even if you don't _plan_ to merge it soon, it'd be less confusing
> to have that in your queue ... say, towards the end, below a line
> that indicates a scheduled merge point.  (FWIW that's not unlike
> Andrew does with MM.)

Indeed, the first patch in question has even been in -mm for quite some
time now, and Andrew keeps sending it to me so that I take it in my i2c
tree. So I've now included the first part in my public i2c patch queue
to make everyone happy. But then again, there is no "scheduled merge
point" for a patchset that is not yet complete.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f]

sorry im topostig, but im using gmail through gprs and it sucks. Jean,
thanks for explaining everything and applying the first part to your
i2c tree. I'll try to address the issues today or tomorrow and send
you a better patch.

Thanks again for your time ;)

On 2/24/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Sat, 23 Feb 2008 11:53:03 -0800, David Brownell wrote:
> > On Saturday 23 February 2008, Jean Delvare wrote:
> > > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote:
> > > > On Saturday 23 February 2008, Felipe Balbi wrote:
> > > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right
> Jean?
> > >
> > > Yes, except that my copy has "isp->irq_type = 0;" removed.
> >
> > From a bzeroed structure?  Then I won't worry about that difference...
>
> I removed this statement exactly because the structure is kzalloc'd.
> You don't have to worry as a tester. I do worry as the i2c subsystem
> maintainer, because I'd like Felipe to base his second patch on the
> "right" first patch, so that it applies properly to my tree.
>
> Felipe, my version of the first patch is available here:
> http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-isp1301_omap-convert-to-new-style-1.patch
>
> Please make sure that the next update to the second part applies
> properly on top of it.
>
> > > > But not yet in the I2C merge queue??
> > >
> > > I was waiting for the second part. The first part doesn't make much
> > > sense alone. I could add the first part to my i2c tree, but I can't
> > > merge it upstream without the second part.
> >
> > That makes no sense to me.  If the first part is OK, it's OK to
> > merge without the second part.
>
> And if the second part never makes it, we end up with garbage in the
> upstream kernel. Thanks but no thanks.
>
> > Even if you don't _plan_ to merge it soon, it'd be less confusing
> > to have that in your queue ... say, towards the end, below a line
> > that indicates a scheduled merge point.  (FWIW that's not unlike
> > Andrew does with MM.)
>
> Indeed, the first patch in question has even been in -mm for quite some
> time now, and Andrew keeps sending it to me so that I take it in my i2c
> tree. So I've now included the first part in my public i2c patch queue
> to make everyone happy. But then again, there is no "scheduled merge
> point" for a patchset that is not yet complete.
>
> --
> Jean Delvare
>


-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3115 bytes --]

On Sun, Feb 24, 2008 at 1:05 PM,  <felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:
> sorry im topostig, but im using gmail through gprs and it sucks. Jean,
>  thanks for explaining everything and applying the first part to your
>  i2c tree. I'll try to address the issues today or tomorrow and send
>  you a better patch.
>
>  Thanks again for your time ;)
>
>
>
>  On 2/24/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
>  > On Sat, 23 Feb 2008 11:53:03 -0800, David Brownell wrote:
>  > > On Saturday 23 February 2008, Jean Delvare wrote:
>  > > > On Sat, 23 Feb 2008 10:10:39 -0800, David Brownell wrote:
>  > > > > On Saturday 23 February 2008, Felipe Balbi wrote:
>  > > > > > Thanks Dave, the patch is attached. patch 1 is acked by Jean, right
>  > Jean?
>  > > >
>  > > > Yes, except that my copy has "isp->irq_type = 0;" removed.
>  > >
>  > > From a bzeroed structure?  Then I won't worry about that difference...
>  >
>  > I removed this statement exactly because the structure is kzalloc'd.
>  > You don't have to worry as a tester. I do worry as the i2c subsystem
>  > maintainer, because I'd like Felipe to base his second patch on the
>  > "right" first patch, so that it applies properly to my tree.
>  >
>  > Felipe, my version of the first patch is available here:
>  > http://khali.linux-fr.org/devel/linux-2.6/jdelvare-i2c/i2c-isp1301_omap-convert-to-new-style-1.patch
>  >
>  > Please make sure that the next update to the second part applies
>  > properly on top of it.
>  >
>  > > > > But not yet in the I2C merge queue??
>  > > >
>  > > > I was waiting for the second part. The first part doesn't make much
>  > > > sense alone. I could add the first part to my i2c tree, but I can't
>  > > > merge it upstream without the second part.
>  > >
>  > > That makes no sense to me.  If the first part is OK, it's OK to
>  > > merge without the second part.
>  >
>  > And if the second part never makes it, we end up with garbage in the
>  > upstream kernel. Thanks but no thanks.
>  >
>  > > Even if you don't _plan_ to merge it soon, it'd be less confusing
>  > > to have that in your queue ... say, towards the end, below a line
>  > > that indicates a scheduled merge point.  (FWIW that's not unlike
>  > > Andrew does with MM.)
>  >
>  > Indeed, the first patch in question has even been in -mm for quite some
>  > time now, and Andrew keeps sending it to me so that I take it in my i2c
>  > tree. So I've now included the first part in my public i2c patch queue
>  > to make everyone happy. But then again, there is no "scheduled merge
>  > point" for a patchset that is not yet complete.
>  >
>  > --
>  > Jean Delvare
>  >
>
>
>
>
> --
>  Best Regards,
>
>  Felipe Balbi
>  felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>

Another version here.
Jean, I'm thinking about introducing include/asm/arch/isp1301.h so I
can introduce struct isp1301_platform_data to pass irq_flags to the
driver.
What do you think ?

Dave, any comments ?

-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --]
[-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 9582 bytes --]

From 44a83059a776e607836139892ed2fd3d7fb307b2 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <me@felipebalbi.com>
Date: Tue, 1 Jan 2008 23:00:18 -0500
Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

Based on David Brownell's patch for tps65010, this patch
finish conversting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 arch/arm/mach-omap1/board-h3.c   |    8 ++
 arch/arm/mach-omap2/board-h4.c   |   11 +++
 drivers/i2c/chips/isp1301_omap.c |  135 ++++++++++---------------------------
 3 files changed, 56 insertions(+), 98 deletions(-)

diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index 6fc5168..7c1b966 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = {
 	},
 };
 
+static struct i2c_board_info __initdata h3_i2c_board_info[] = {
+       {
+               I2C_BOARD_INFO("isp1301_omap", 0x2d),
+               .type           = "isp1301_omap",
+               .irq            = OMAP_GPIO_IRQ(14),
+       },
+};
+
 #define H3_NAND_RB_GPIO_PIN	10
 
 static int nand_dev_ready(struct omap_nand_platform_data *data)
diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
index f125f43..d469c63 100644
--- a/arch/arm/mach-omap2/board-h4.c
+++ b/arch/arm/mach-omap2/board-h4.c
@@ -301,6 +301,14 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct i2c_board_info __initdata h4_i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(125),
+	},
+};
+
 static void __init omap_h4_init(void)
 {
 	/*
@@ -321,6 +329,9 @@ static void __init omap_h4_init(void)
 	}
 #endif
 
+	i2c_register_board_info(1, h4_i2c_board_info,
+			ARRAY_SIZE(h4_i2c_board_info));
+
 	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
 	omap_board_config = h4_config;
 	omap_board_config_size = ARRAY_SIZE(h4_config);
diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 92a9c01..3b20366 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -31,8 +31,6 @@
 #include <linux/usb/otg.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
-
-#include <asm/irq.h>
 #include <asm/arch/usb.h>
 
 
@@ -50,12 +48,8 @@ MODULE_LICENSE("GPL");
 struct isp1301 {
 	struct otg_transceiver	otg;
 	struct i2c_client	*client;
-	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
-	int			irq;
-	int			irq_type;
-
 	u32			last_otg_ctrl;
 	unsigned		working:1;
 
@@ -140,13 +134,6 @@ static inline void notresponding(struct isp1301 *isp)
 /*-------------------------------------------------------------------------*/
 
 /* only two addresses possible */
-#define	ISP_BASE		0x2c
-static unsigned short normal_i2c[] = {
-	ISP_BASE, ISP_BASE + 1,
-	I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
 static struct i2c_driver isp1301_driver;
 
 /* smbus apis are used for portability */
@@ -1190,9 +1177,7 @@ static void isp1301_timer(unsigned long _isp)
 
 static void isp1301_release(struct device *dev)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(dev, struct isp1301, c.dev);
+	struct isp1301		*isp = dev_get_drvdata(dev);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1202,30 +1187,27 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int isp1301_detach_client(struct i2c_client *i2c)
+static int __exit isp1301_remove(struct i2c_client *client)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(i2c, struct isp1301, c);
+	struct isp1301	*isp = i2c_get_clientdata(client);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
-	free_irq(isp->irq, isp);
+
+	if (client->irq > 0)
+		free_irq(client->irq, isp);
 #ifdef	CONFIG_USB_OTG
 	otg_unbind(isp);
 #endif
-	if (machine_is_omap_h2())
-		omap_free_gpio(2);
-
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
 	flush_scheduled_work();
 
-	put_device(&i2c->dev);
+	put_device(&client->dev);
 	the_transceiver = 0;
 
-	return i2c_detach_client(i2c);
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1295,9 +1277,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 	power_up(isp);
 
-	if (machine_is_omap_h2())
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
-
 	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
@@ -1475,11 +1454,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 /*-------------------------------------------------------------------------*/
 
 /* no error returns, they'd just make bus scanning stop */
-static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
+static int __init isp1301_probe(struct i2c_client *client)
 {
 	int			status;
 	struct isp1301		*isp;
-	struct i2c_client	*i2c;
 
 	if (the_transceiver)
 		return 0;
@@ -1492,46 +1470,29 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
-
-	isp->irq = -1;
-	isp->irq_type = 0;
-	isp->c.addr = address;
-	i2c_set_clientdata(&isp->c, isp);
-	isp->c.adapter = bus;
-	isp->c.driver = &isp1301_driver;
-	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
-	isp->client = i2c = &isp->c;
+	isp->client = client;
 
 	/* if this is a true probe, verify the chip ... */
-	if (kind < 0) {
-		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
-		if (status != I2C_VENDOR_ID_PHILIPS) {
-			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
-				address, status);
-			goto fail1;
-		}
-		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
-		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
-			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
-				address, status);
-			goto fail1;
-		}
+	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
+	if (status != I2C_VENDOR_ID_PHILIPS) {
+		dev_dbg(&client->dev, "not philips id: %d\n", status);
+		goto fail1;
+	}
+	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
+	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
+		dev_dbg(&client->dev, "not isp1301, %d\n", status);
+		goto fail1;
 	}
 
-	status = i2c_attach_client(i2c);
-	if (status < 0) {
-		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
-				DRIVER_NAME, address, status);
 fail1:
 		kfree(isp);
 		return 0;
 	}
-	isp->i2c_release = i2c->dev.release;
-	i2c->dev.release = isp1301_release;
+	isp->i2c_release = client->dev.release = isp1301_release;
 
 	/* initial development used chiprev 2.00 */
-	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
-	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
+	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
+	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
 		status >> 8, status & 0xff);
 
 	/* make like power-on reset */
@@ -1552,40 +1513,26 @@ fail1:
 #ifdef	CONFIG_USB_OTG
 	status = otg_bind(isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't bind OTG\n");
+		dev_dbg(&client->dev, "can't bind OTG\n");
 		goto fail2;
 	}
 #endif
 
-	if (machine_is_omap_h2()) {
-		/* full speed signaling by default */
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
-			MC1_SPEED_REG);
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
-			MC2_SPD_SUSP_CTRL);
-
-		/* IRQ wired at M14 */
-		omap_cfg_reg(M14_1510_GPIO2);
-		isp->irq = OMAP_GPIO_IRQ(2);
-		if (gpio_request(2, "isp1301") == 0)
-			gpio_direction_input(2);
-		isp->irq_type = IRQF_TRIGGER_FALLING;
-	}
-
-	isp->irq_type |= IRQF_SAMPLE_RANDOM;
-	status = request_irq(isp->irq, isp1301_irq,
-			isp->irq_type, DRIVER_NAME, isp);
-	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
-				isp->irq, status);
+	if (client->irq > 0) {
+		status = request_irq(client->irq, isp1301_irq,
+				IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
+				DRIVER_NAME, isp);
+		if (status < 0) {
+			dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
+					client->irq, status);
 #ifdef	CONFIG_USB_OTG
 fail2:
 #endif
-		i2c_detach_client(i2c);
-		goto fail1;
+			goto fail1;
+		}
 	}
 
-	isp->otg.dev = &isp->client->dev;
+	isp->otg.dev = &client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
@@ -1607,31 +1554,23 @@ fail2:
 
 #ifdef	VERBOSE
 	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
-	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
+	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
 #endif
 
 	status = otg_set_transceiver(&isp->otg);
 	if (status < 0)
-		dev_err(&i2c->dev, "can't register transceiver, %d\n",
+		dev_err(&client->dev, "can't register transceiver, %d\n",
 			status);
 
 	return 0;
 }
 
-static int isp1301_scan_bus(struct i2c_adapter *bus)
-{
-	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
-			| I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EINVAL;
-	return i2c_probe(bus, &addr_data, isp1301_probe);
-}
-
 static struct i2c_driver isp1301_driver = {
 	.driver = {
 		.name	= "isp1301_omap",
 	},
-	.attach_adapter	= isp1301_scan_bus,
-	.detach_client	= isp1301_detach_client,
+	.probe	= isp1301_probe,
+	.remove	= __exit_p(isp1301_remove),
 };
 
 /*-------------------------------------------------------------------------*/
-- 
1.5.4.34.g053d9


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

Hi all,

Dave, here I have two more patches (again).

The first one is actually the second part of i2c new style driver for
isp1301 and the second one,
syncs some changes from linux-omap. Now that Tony merged omap changes
to mainline we can update
this driver.

I'll also send a patch to Tony later when these patches are applied to mainline.

Thanks Dave and Jean.

ps: I'm still needing some tips about how to pass irq flags from
board-files to the driver. A struct platform_data* would be enough ?

-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff --]
[-- Type: text/x-patch; name=0001-I2C-ISP1301_OMAP-New-style-i2c-driver-updates-part.diff, Size: 9585 bytes --]

From 0bc638319135186fbc5aa40d9e8b1ae5c3aa4409 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <me@felipebalbi.com>
Date: Tue, 1 Jan 2008 23:00:18 -0500
Subject: [PATCH] I2C: ISP1301_OMAP: New-style i2c driver updates, part 2

Based on David Brownell's patch for tps65010, this patch
finish conversting isp1301_omap.c to new-style i2c driver.

Signed-off-by: Felipe Balbi <me@felipebalbi.com>
---
 arch/arm/mach-omap1/board-h3.c   |    8 ++
 arch/arm/mach-omap2/board-h4.c   |   11 +++
 drivers/i2c/chips/isp1301_omap.c |  137 +++++++++++---------------------------
 3 files changed, 57 insertions(+), 99 deletions(-)

diff --git a/arch/arm/mach-omap1/board-h3.c b/arch/arm/mach-omap1/board-h3.c
index 6fc5168..7c1b966 100644
--- a/arch/arm/mach-omap1/board-h3.c
+++ b/arch/arm/mach-omap1/board-h3.c
@@ -469,6 +469,14 @@ static struct omap_gpio_switch h3_gpio_switches[] __initdata = {
 	},
 };
 
+static struct i2c_board_info __initdata h3_i2c_board_info[] = {
+       {
+               I2C_BOARD_INFO("isp1301_omap", 0x2d),
+               .type           = "isp1301_omap",
+               .irq            = OMAP_GPIO_IRQ(14),
+       },
+};
+
 #define H3_NAND_RB_GPIO_PIN	10
 
 static int nand_dev_ready(struct omap_nand_platform_data *data)
diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c
index f125f43..d469c63 100644
--- a/arch/arm/mach-omap2/board-h4.c
+++ b/arch/arm/mach-omap2/board-h4.c
@@ -301,6 +301,14 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct i2c_board_info __initdata h4_i2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("isp1301_omap", 0x2d),
+		.type		= "isp1301_omap",
+		.irq		= OMAP_GPIO_IRQ(125),
+	},
+};
+
 static void __init omap_h4_init(void)
 {
 	/*
@@ -321,6 +329,9 @@ static void __init omap_h4_init(void)
 	}
 #endif
 
+	i2c_register_board_info(1, h4_i2c_board_info,
+			ARRAY_SIZE(h4_i2c_board_info));
+
 	platform_add_devices(h4_devices, ARRAY_SIZE(h4_devices));
 	omap_board_config = h4_config;
 	omap_board_config_size = ARRAY_SIZE(h4_config);
diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 92a9c01..12ac0b3 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -31,8 +31,6 @@
 #include <linux/usb/otg.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
-
-#include <asm/irq.h>
 #include <asm/arch/usb.h>
 
 
@@ -50,12 +48,8 @@ MODULE_LICENSE("GPL");
 struct isp1301 {
 	struct otg_transceiver	otg;
 	struct i2c_client	*client;
-	struct i2c_client	c;
 	void			(*i2c_release)(struct device *dev);
 
-	int			irq;
-	int			irq_type;
-
 	u32			last_otg_ctrl;
 	unsigned		working:1;
 
@@ -140,13 +134,6 @@ static inline void notresponding(struct isp1301 *isp)
 /*-------------------------------------------------------------------------*/
 
 /* only two addresses possible */
-#define	ISP_BASE		0x2c
-static unsigned short normal_i2c[] = {
-	ISP_BASE, ISP_BASE + 1,
-	I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
 static struct i2c_driver isp1301_driver;
 
 /* smbus apis are used for portability */
@@ -1190,9 +1177,7 @@ static void isp1301_timer(unsigned long _isp)
 
 static void isp1301_release(struct device *dev)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(dev, struct isp1301, c.dev);
+	struct isp1301		*isp = dev_get_drvdata(dev);
 
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
@@ -1202,30 +1187,27 @@ static void isp1301_release(struct device *dev)
 
 static struct isp1301 *the_transceiver;
 
-static int isp1301_detach_client(struct i2c_client *i2c)
+static int __exit isp1301_remove(struct i2c_client *client)
 {
-	struct isp1301	*isp;
-
-	isp = container_of(i2c, struct isp1301, c);
+	struct isp1301	*isp = i2c_get_clientdata(client);
 
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_FALLING, ~0);
 	isp1301_clear_bits(isp, ISP1301_INTERRUPT_RISING, ~0);
-	free_irq(isp->irq, isp);
+
+	if (client->irq > 0)
+		free_irq(client->irq, isp);
 #ifdef	CONFIG_USB_OTG
 	otg_unbind(isp);
 #endif
-	if (machine_is_omap_h2())
-		omap_free_gpio(2);
-
 	isp->timer.data = 0;
 	set_bit(WORK_STOP, &isp->todo);
 	del_timer_sync(&isp->timer);
 	flush_scheduled_work();
 
-	put_device(&i2c->dev);
+	put_device(&client->dev);
 	the_transceiver = 0;
 
-	return i2c_detach_client(i2c);
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -1295,9 +1277,6 @@ isp1301_set_host(struct otg_transceiver *otg, struct usb_bus *host)
 
 	power_up(isp);
 
-	if (machine_is_omap_h2())
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
-
 	dev_info(&isp->client->dev, "A-Host sessions ok\n");
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
 		INTR_ID_GND);
@@ -1475,11 +1454,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 /*-------------------------------------------------------------------------*/
 
 /* no error returns, they'd just make bus scanning stop */
-static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
+static int __init isp1301_probe(struct i2c_client *client)
 {
 	int			status;
 	struct isp1301		*isp;
-	struct i2c_client	*i2c;
 
 	if (the_transceiver)
 		return 0;
@@ -1492,46 +1470,29 @@ static int isp1301_probe(struct i2c_adapter *bus, int address, int kind)
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
-
-	isp->irq = -1;
-	isp->irq_type = 0;
-	isp->c.addr = address;
-	i2c_set_clientdata(&isp->c, isp);
-	isp->c.adapter = bus;
-	isp->c.driver = &isp1301_driver;
-	strlcpy(isp->c.name, DRIVER_NAME, I2C_NAME_SIZE);
-	isp->client = i2c = &isp->c;
+	isp->client = client;
 
 	/* if this is a true probe, verify the chip ... */
-	if (kind < 0) {
-		status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
-		if (status != I2C_VENDOR_ID_PHILIPS) {
-			dev_dbg(&bus->dev, "addr %d not philips id: %d\n",
-				address, status);
-			goto fail1;
-		}
-		status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
-		if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
-			dev_dbg(&bus->dev, "%d not isp1301, %d\n",
-				address, status);
-			goto fail1;
-		}
+	status = isp1301_get_u16(isp, ISP1301_VENDOR_ID);
+	if (status != I2C_VENDOR_ID_PHILIPS) {
+		dev_dbg(&client->dev, "not philips id: %d\n", status);
+		goto fail1;
+	}
+	status = isp1301_get_u16(isp, ISP1301_PRODUCT_ID);
+	if (status != I2C_PRODUCT_ID_PHILIPS_1301) {
+		dev_dbg(&client->dev, "not isp1301, %d\n", status);
+		goto fail1;
 	}
 
-	status = i2c_attach_client(i2c);
-	if (status < 0) {
-		dev_dbg(&bus->dev, "can't attach %s to device %d, err %d\n",
-				DRIVER_NAME, address, status);
 fail1:
 		kfree(isp);
 		return 0;
-	}
-	isp->i2c_release = i2c->dev.release;
-	i2c->dev.release = isp1301_release;
+
+	isp->i2c_release = client->dev.release = isp1301_release;
 
 	/* initial development used chiprev 2.00 */
-	status = i2c_smbus_read_word_data(i2c, ISP1301_BCD_DEVICE);
-	dev_info(&i2c->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
+	status = i2c_smbus_read_word_data(client, ISP1301_BCD_DEVICE);
+	dev_info(&client->dev, "chiprev %x.%02x, driver " DRIVER_VERSION "\n",
 		status >> 8, status & 0xff);
 
 	/* make like power-on reset */
@@ -1552,40 +1513,26 @@ fail1:
 #ifdef	CONFIG_USB_OTG
 	status = otg_bind(isp);
 	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't bind OTG\n");
+		dev_dbg(&client->dev, "can't bind OTG\n");
 		goto fail2;
 	}
 #endif
 
-	if (machine_is_omap_h2()) {
-		/* full speed signaling by default */
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1,
-			MC1_SPEED_REG);
-		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_2,
-			MC2_SPD_SUSP_CTRL);
-
-		/* IRQ wired at M14 */
-		omap_cfg_reg(M14_1510_GPIO2);
-		isp->irq = OMAP_GPIO_IRQ(2);
-		if (gpio_request(2, "isp1301") == 0)
-			gpio_direction_input(2);
-		isp->irq_type = IRQF_TRIGGER_FALLING;
-	}
-
-	isp->irq_type |= IRQF_SAMPLE_RANDOM;
-	status = request_irq(isp->irq, isp1301_irq,
-			isp->irq_type, DRIVER_NAME, isp);
-	if (status < 0) {
-		dev_dbg(&i2c->dev, "can't get IRQ %d, err %d\n",
-				isp->irq, status);
+	if (client->irq > 0) {
+		status = request_irq(client->irq, isp1301_irq,
+				IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_FALLING,
+				DRIVER_NAME, isp);
+		if (status < 0) {
+			dev_dbg(&client->dev, "can't get IRQ %d, err %d\n",
+					client->irq, status);
 #ifdef	CONFIG_USB_OTG
 fail2:
 #endif
-		i2c_detach_client(i2c);
-		goto fail1;
+			goto fail1;
+		}
 	}
 
-	isp->otg.dev = &isp->client->dev;
+	isp->otg.dev = &client->dev;
 	isp->otg.label = DRIVER_NAME;
 
 	isp->otg.set_host = isp1301_set_host,
@@ -1607,31 +1554,23 @@ fail2:
 
 #ifdef	VERBOSE
 	mod_timer(&isp->timer, jiffies + TIMER_JIFFIES);
-	dev_dbg(&i2c->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
+	dev_dbg(&client->dev, "scheduled timer, %d min\n", TIMER_MINUTES);
 #endif
 
 	status = otg_set_transceiver(&isp->otg);
 	if (status < 0)
-		dev_err(&i2c->dev, "can't register transceiver, %d\n",
+		dev_err(&client->dev, "can't register transceiver, %d\n",
 			status);
 
 	return 0;
 }
 
-static int isp1301_scan_bus(struct i2c_adapter *bus)
-{
-	if (!i2c_check_functionality(bus, I2C_FUNC_SMBUS_BYTE_DATA
-			| I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EINVAL;
-	return i2c_probe(bus, &addr_data, isp1301_probe);
-}
-
 static struct i2c_driver isp1301_driver = {
 	.driver = {
 		.name	= "isp1301_omap",
 	},
-	.attach_adapter	= isp1301_scan_bus,
-	.detach_client	= isp1301_detach_client,
+	.probe	= isp1301_probe,
+	.remove	= __exit_p(isp1301_remove),
 };
 
 /*-------------------------------------------------------------------------*/
-- 
1.5.4.34.g053d9


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-I2C-ISP1301-Put-linux-mainline-and-linux-omap-in-sy.diff --]
[-- Type: text/x-patch; name=0002-I2C-ISP1301-Put-linux-mainline-and-linux-omap-in-sy.diff, Size: 4209 bytes --]

From 8a558ae11b68763a3c51cc1c20197f39820e1370 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@nokia.com>
Date: Mon, 25 Feb 2008 14:30:49 +0200
Subject: [PATCH] I2C: ISP1301: Put linux-mainline and linux-omap in sync

This patch merges linux-omap's and mainline's isp1301 driver
to put them in sync.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/i2c/chips/isp1301_omap.c |   42 ++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/chips/isp1301_omap.c b/drivers/i2c/chips/isp1301_omap.c
index 12ac0b3..b5ae803 100644
--- a/drivers/i2c/chips/isp1301_omap.c
+++ b/drivers/i2c/chips/isp1301_omap.c
@@ -18,6 +18,8 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
+#undef	DEBUG
+#undef	VERBOSE
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -38,7 +40,6 @@
 #undef	VERBOSE
 #endif
 
-
 #define	DRIVER_VERSION	"24 August 2004"
 #define	DRIVER_NAME	(isp1301_driver.driver.name)
 
@@ -84,7 +85,8 @@ struct isp1301 {
 
 /*-------------------------------------------------------------------------*/
 
-#ifdef	CONFIG_MACH_OMAP_H2
+#if	defined(CONFIG_MACH_OMAP_H2) || \
+	defined(CONFIG_MACH_OMAP_H3)
 
 /* board-specific PM hooks */
 
@@ -121,15 +123,26 @@ static void enable_vbus_source(struct isp1301 *isp)
 	 */
 }
 
+#else
 
-/* products will deliver OTG messages with LEDs, GUI, etc */
-static inline void notresponding(struct isp1301 *isp)
+static void enable_vbus_draw(struct isp1301 *isp, unsigned mA)
 {
-	printk(KERN_NOTICE "OTG device not responding.\n");
+	pr_debug("%s UNIMPL\n", __FUNCTION__);
 }
 
+static void enable_vbus_source(struct isp1301 *isp)
+{
+	pr_debug("%s UNIMPL\n", __FUNCTION__);
+}
 
 #endif
+/*-------------------------------------------------------------------------*/
+
+/* products will deliver OTG messages with LEDs, GUI, etc */
+static inline void notresponding(struct isp1301 *isp)
+{
+	printk(KERN_NOTICE "OTG device not responding.\n");
+}
 
 /*-------------------------------------------------------------------------*/
 
@@ -495,6 +508,7 @@ static inline void check_state(struct isp1301 *isp, const char *tag) { }
 static void update_otg1(struct isp1301 *isp, u8 int_src)
 {
 	u32	otg_ctrl;
+	u8      int_id;
 
 	otg_ctrl = OTG_CTRL_REG
 			& OTG_CTRL_MASK
@@ -508,7 +522,10 @@ static void update_otg1(struct isp1301 *isp, u8 int_src)
 	}
 	if (int_src & INTR_VBUS_VLD)
 		otg_ctrl |= OTG_VBUSVLD;
-	if (int_src & INTR_ID_GND) {		/* default-A */
+
+	int_id = isp1301_get_u8(isp, ISP1301_INTERRUPT_SOURCE);
+
+	if (int_id & INTR_ID_GND) {		/* default-A */
 		if (isp->otg.state == OTG_STATE_B_IDLE
 				|| isp->otg.state == OTG_STATE_UNDEFINED) {
 			a_idle(isp, "init");
@@ -1063,7 +1080,7 @@ static void isp_update_otg(struct isp1301 *isp, u8 stat)
 	/* update the OTG controller state to match the isp1301; may
 	 * trigger OPRT_CHG irqs for changes going to the isp1301.
 	 */
-	update_otg1(isp, isp_stat);
+	update_otg1(isp, isp_stat); // pass the actual interrupt latch status
 	update_otg2(isp, isp_bstat);
 	check_state(isp, __FUNCTION__);
 #endif
@@ -1337,13 +1354,14 @@ isp1301_set_peripheral(struct otg_transceiver *otg, struct usb_gadget *gadget)
 	power_up(isp);
 	isp->otg.state = OTG_STATE_B_IDLE;
 
-	if (machine_is_omap_h2())
+/* REVISIT H4 too? */
+	if (machine_is_omap_h2() || machine_is_omap_h3())
 		isp1301_set_bits(isp, ISP1301_MODE_CONTROL_1, MC1_DAT_SE0);
 
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_RISING,
-		INTR_SESS_VLD);
+		INTR_SESS_VLD | INTR_VBUS_VLD);
 	isp1301_set_bits(isp, ISP1301_INTERRUPT_FALLING,
-		INTR_VBUS_VLD);
+		INTR_VBUS_VLD | INTR_SESS_VLD);
 	dev_info(&isp->client->dev, "B-Peripheral sessions ok\n");
 	dump_regs(isp, __FUNCTION__);
 
@@ -1420,6 +1438,10 @@ isp1301_start_hnp(struct otg_transceiver *dev)
 	 * So do this part as early as possible...
 	 */
 	switch (isp->otg.state) {
+	case OTG_STATE_B_PERIPHERAL:
+		isp->otg.state = OTG_STATE_B_WAIT_ACON;
+		isp1301_defer_work(isp, WORK_UPDATE_ISP);
+		break;
 	case OTG_STATE_B_HOST:
 		isp->otg.state = OTG_STATE_B_PERIPHERAL;
 		/* caller will suspend next */
-- 
1.5.4.34.g053d9


[-- Attachment #4: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Monday 25 February 2008, Felipe Balbi wrote:
> Jean, I'm thinking about introducing include/asm/arch/isp1301.h

Or maybe isp1301_omap.h to match the driver name.


> so I 
> can introduce struct isp1301_platform_data to pass irq_flags to the
> driver.

Passing irq_flags???  That sounds like nonsense.  Please explain ...


By the way, I see why there's no board-h2.c update.  One of
the patches in December stripped out most of the tps65010
i2c_board info from mainline, replacing some of it with
isp1301 stuff ... that can't possibly work in mainline.

So there are going to be some H2 and H3 regression-fix
patches upcoming...

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

On Tue, Feb 26, 2008 at 7:38 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Monday 25 February 2008, Felipe Balbi wrote:
>  > Jean, I'm thinking about introducing include/asm/arch/isp1301.h
>
>  Or maybe isp1301_omap.h to match the driver name.
>
>
>
>  > so I
>  > can introduce struct isp1301_platform_data to pass irq_flags to the
>  > driver.
>
>  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.

>
>
>  By the way, I see why there's no board-h2.c update.  One of
>  the patches in December stripped out most of the tps65010
>  i2c_board info from mainline, replacing some of it with
>  isp1301 stuff ... that can't possibly work in mainline.
>
>  So there are going to be some H2 and H3 regression-fix
>  patches upcoming...

that's true...

-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi Felipe,

On Mon, 25 Feb 2008 10:46:21 +0200, Felipe Balbi wrote:
> Jean, I'm thinking about introducing include/asm/arch/isp1301.h so I
> can introduce struct isp1301_platform_data to pass irq_flags to the
> driver.
> What do you think ?

This would rather be include/linux/i2c/isp1301_omap.h, now that we have
created this directory.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

Hi,

On Thu, Feb 28, 2008 at 11:50 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Felipe,
>
>
>  On Mon, 25 Feb 2008 10:46:21 +0200, Felipe Balbi wrote:
>  > Jean, I'm thinking about introducing include/asm/arch/isp1301.h so I
>  > can introduce struct isp1301_platform_data to pass irq_flags to the
>  > driver.
>  > What do you think ?
>
>  This would rather be include/linux/i2c/isp1301_omap.h, now that we have
>  created this directory.

Thanks Jean, I'll try to address this issue during this week and ask
someone in linux-omap ML to try the patches agains linux-mainline :-)

Have a nice day

>
>  --
>  Jean Delvare
>



-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Monday 25 February 2008, Felipe Balbi wrote:
> 
> Hi all,
> 
> Dave, here I have two more patches (again).
> 
> The first one is actually the second part of i2c new style driver for
> isp1301

I tried to apply this over the (Jean's version of) the first one,
and got rejects ...


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

Hi,

On Thu, Mar 6, 2008 at 8:28 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Monday 25 February 2008, Felipe Balbi wrote:
>  >
>  > Hi all,
>  >
>  > Dave, here I have two more patches (again).
>  >
>  > The first one is actually the second part of i2c new style driver for
>  > isp1301
>
>  I tried to apply this over the (Jean's version of) the first one,
>  and got rejects ...

Crap, I'll refresh and resend ;-)


-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

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 

#define IORESOURCE_IRQ_HIGHEDGE         (1<<0)
#define IORESOURCE_IRQ_LOWEDGE          (1<<1)
#define IORESOURCE_IRQ_HIGHLEVEL        (1<<2)
#define IORESOURCE_IRQ_LOWLEVEL         (1<<3)
#define IORESOURCE_IRQ_SHAREABLE        (1<<4)

I'd presume "highedge" means "rising", etc.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

Hi,

On Fri, Mar 7, 2008 at 10:20 PM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 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?

Sorry if I'm being pointless :-s

-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

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-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 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.

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;
}

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.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 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-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

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.

- Dave





_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Felipe Balbi]

On Sun, Mar 16, 2008 at 5:57 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 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.

Thanks

>
>  - Dave
>
>
>
>
>



-- 
Best Regards,

Felipe Balbi
felipebalbi-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

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-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 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.

Thanks David for your help :)

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 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-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c