I2C fixes and -next from Ben Dooks
cover I2C fixes and -next from Ben Dooks
This is a set of my own fixes and development work which includes fixes for the architecture I maintain, and items that will be merged at the next merge window.
Signature · 6 lines
Ben _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 1/8 I2C: S3C2410: Check ACK on byte transmission
We should check for the reception of an ACK after transmissint each data byte. The address send has been correctly checking this, but the data write byte state should have also been checking for these failures.
As part of the same fix, we remove the ACK checking from the receive path where it should not have been checking for an ACK which our hardware was sending.
drivers/i2c/busses/i2c-s3c2410.c +9 -11
@@ -324,6 +324,15 @@ static int i2s_s3c_irq_nextbyte(struct s*/ retry_write: +if (!(i2c->msg->flags &I2C_M_IGNORE_NAK )) { +if (iicstat &S3C2410_IICSTAT_LASTBIT ) { +dev_dbg (i2c->dev,"WRITE: No Ack\n" ); + +s3c24xx_i2c_stop (i2c, -ECONNREFUSED ); + goto out_ack; + } + } +if (!is_msgend (i2c)) { byte = i2c->msg->buf[i2c->msg_ptr++];writeb (byte, i2c->regs +S3C2410_IICDS );
@@ -377,17 +386,6 @@ static int i2s_s3c_irq_nextbyte(struct s* going to do any more read/write */ -if (!(i2c->msg->flags &I2C_M_IGNORE_NAK ) && - !(is_msglast (i2c) &&is_lastmsg (i2c))) { - -if (iicstat &S3C2410_IICSTAT_LASTBIT ) { -dev_dbg (i2c->dev,"READ: No Ack\n" ); - -s3c24xx_i2c_stop (i2c, -ECONNREFUSED ); - goto out_ack; - } - } - byte =readb (i2c->regs +S3C2410_IICDS ); i2c->msg->buf[i2c->msg_ptr++] = byte;
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 1/8] I2C: S3C2410: Check ACK on byte transmission
Hi Ben,
Quoted 1 line · Ben Dooks
We should check for the reception of an ACK after transmissint each
Spelling: transmitting.
Quoted 16 lines
data byte. The address send has been correctly checking this, but the data write byte state should have also been checking for these failures.
As part of the same fix, we remove the ACK checking from the receive path where it should not have been checking for an ACK which our hardware was sending.
Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
===================================================================drivers/i2c/busses/i2c-s3c2410.c +0
-0@@ -324,6 +324,15 @@ static int i2s_s3c_irq_nextbyte(struct s*/
drivers/i2c/busses/i2c-s3c2410.c static int i2s_s3c_irq_nextbyte(struct s · line 335
Quoted 10 lines
drivers/i2c/busses/i2c-s3c2410.c +9
-0retry_write: +if (!(i2c->msg->flags &I2C_M_IGNORE_NAK )) { +if (iicstat &S3C2410_IICSTAT_LASTBIT ) { +dev_dbg (i2c->dev,"WRITE: No Ack\n" ); + +s3c24xx_i2c_stop (i2c, -ECONNREFUSED ); + goto out_ack; + } + } +
Do you really want to check this again when you jump to retry_write? I doubt it. My guess is that you want to add the code before the retry_write label, rather than after it.
Quoted 22 lines
drivers/i2c/busses/i2c-s3c2410.c +0
-11if (!is_msgend (i2c)) { byte = i2c->msg->buf[i2c->msg_ptr++];writeb (byte, i2c->regs +S3C2410_IICDS );@@ -377,17 +386,6 @@ static int i2s_s3c_irq_nextbyte(struct s* going to do any more read/write */ -if (!(i2c->msg->flags &I2C_M_IGNORE_NAK ) && - !(is_msglast (i2c) &&is_lastmsg (i2c))) { - -if (iicstat &S3C2410_IICSTAT_LASTBIT ) { -dev_dbg (i2c->dev,"READ: No Ack\n" ); - -s3c24xx_i2c_stop (i2c, -ECONNREFUSED ); - goto out_ack; - } - } - byte =readb (i2c->regs +S3C2410_IICDS ); i2c->msg->buf[i2c->msg_ptr++] = byte;
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 1/8] I2C: S3C2410: Check ACK on byte transmission
Quoted 6 lines · Jean Delvare
Hi Ben,
Quoted 1 line · Ben Dooks
We should check for the reception of an ACK after transmissint each
Spelling: transmitting.
fixed.
Quoted 5 lines
Quoted 5 lines
drivers/i2c/busses/i2c-s3c2410.c +5
-0+if (!(i2c->msg->flags &I2C_M_IGNORE_NAK )) { +if (iicstat &S3C2410_IICSTAT_LASTBIT ) { +dev_dbg (i2c->dev,"WRITE: No Ack\n" ); + +s3c24xx_i2c_stop (i2c, -ECONNREFUSED );
Quoted 8 lines
drivers/i2c/busses/i2c-s3c2410.c static int i2s_s3c_irq_nextbyte(struct s · line 335
Quoted 4 lines
drivers/i2c/busses/i2c-s3c2410.c +4
-0+ goto out_ack; + } + } +Do you really want to check this again when you jump to
retry_write? I doubt it. My guess is that you want to add the code before theretry_writelabel, rather than after it.
fixed, not really a bug, but more efficient to not try and run this through again after we've already checked the i2c master status in the interrupt.
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 2/8 I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.
In reply to Ben DooksAdd a MODULE_ALIAS() statement for the i2c-s3c2410 controller to ensure that it can be autoloaded on the S3C2440 systems that we support.
drivers/i2c/busses/i2c-s3c2410.c +1 -0
@@ -947,3 +947,4 @@ MODULE_DESCRIPTION("S3C24XX I2C Bus drivMODULE_AUTHOR ("Ben Dooks, <ben-Y5A6D6n0/[email protected]>" );MODULE_LICENSE ("GPL" );MODULE_ALIAS ("platform:s3c2410-i2c" ); +MODULE_ALIAS ("platform:s3c2440-i2c" );
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Jean Delvare
Acked-byRe: [patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.
Hi Ben,
Quoted 11 lines · Ben Dooks
Add a
MODULE_ALIAS()statement for the i2c-s3c2410 controller to ensure that it can be autoloaded on the S3C2440 systems that we support.Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
===================================================================
drivers/i2c/busses/i2c-s3c2410.c MODULE_DESCRIPTION("S3C24XX I2C Bus driv · line 950
Quoted 4 lines
drivers/i2c/busses/i2c-s3c2410.c +1
-0@@ -947,3 +947,4 @@ MODULE_DESCRIPTION("S3C24XX I2C Bus drivMODULE_AUTHOR ("Ben Dooks, <ben-Y5A6D6n0/[email protected]>" );MODULE_LICENSE ("GPL" );MODULE_ALIAS ("platform:s3c2410-i2c" ); +MODULE_ALIAS ("platform:s3c2440-i2c" );
With the current driver code, that's correct.
That being said, I think the approach is wrong. You shouldn't register two different platform drivers just to be able to differentiate between device types. You have platform_data for that, it's cleaner and cheaper.
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.
Quoted 21 lines · Jean Delvare
Hi Ben,
Quoted 11 lines · Ben Dooks
Add a
MODULE_ALIAS()statement for the i2c-s3c2410 controller to ensure that it can be autoloaded on the S3C2440 systems that we support.Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt1/drivers/i2c/busses/i2c-s3c2410.c
===================================================================drivers/i2c/busses/i2c-s3c2410.c MODULE_DESCRIPTION("S3C24XX I2C Bus driv · line 950
Quoted 4 lines
drivers/i2c/busses/i2c-s3c2410.c +1
-0@@ -947,3 +947,4 @@ MODULE_DESCRIPTION("S3C24XX I2C Bus drivMODULE_AUTHOR ("Ben Dooks, <ben-Y5A6D6n0/[email protected]>" );MODULE_LICENSE ("GPL" );MODULE_ALIAS ("platform:s3c2410-i2c" ); +MODULE_ALIAS ("platform:s3c2440-i2c" );With the current driver code, that's correct.
Quoted 5 lines
Acked-by Jean Delvare <[email protected]>That being said, I think the approach is wrong. You shouldn't register two different platform drivers just to be able to differentiate between device types. You have
platform_datafor that, it's cleaner and cheaper.
Actually, there are down sides to doing it via platform data, and a few upsides to using the driver model. I do agree that >1 driver does mean that you end up allocating more space to the drivers, but I hope that I can convince you that this isn't without merit.
The following are downsides:
1) using platform data means that not only do all the boards that have i2c busses on (or other peripherals using this method) need to carry[1] and register platform data even if they do not need to.
2) We currently change the name of the platform device in the cpu specific architecture initialisation, which would mean either moving this to each machine or have some form of sharing between the arch code and the board init.
3) Some boards can have more than one cpu type on them, which makes life even worse for the above.
The advantage of changing the name of the platform device means that the type of device is shown in the /sys/devices/platform heirachy without any need for code to add a new attribute to show it within the device itself.
[1] Even if this is __initdata, it means the kernel has to carry it to load it even if it gets dumped at startup, and we have a number of boards that are available.
Signature · 10 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year. _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): 20080601215102.GJ7334-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]
Re: [patch 2/8] I2C: S3C2410: Add MODULE_ALIAS() for s3c2440 device.
Quoted 24 lines · Ben Dooks
Quoted 3 lines · Jean Delvare
That being said, I think the approach is wrong. You shouldn't register two different platform drivers just to be able to differentiate between device types. You have
platform_datafor that, it's cleaner and cheaper.Actually, there are down sides to doing it via platform data, and a few upsides to using the driver model. I do agree that >1 driver does mean that you end up allocating more space to the drivers, but I hope that I can convince you that this isn't without merit.
The following are downsides:
1) using platform data means that not only do all the boards that have i2c busses on (or other peripherals using this method) need to carry[1] and register platform data even if they do not need to.
2) We currently change the name of the platform device in the cpu specific architecture initialisation, which would mean either moving this to each machine or have some form of sharing between the arch code and the board init.
3) Some boards can have more than one cpu type on them, which makes life even worse for the above.
Quoted 9 lines
The advantage of changing the name of the platform device means that the type of device is shown in the
/sys/devices/platformheirachy without any need for code to add a new attribute to show it within the device itself.[1] Even if this is
__initdata, it means the kernel has to carry it to load it even if it gets dumped at startup, and we have a number of boards that are available.
Out of 6 platform driver in the kernel tree that do this, 5 were written by you. So apparently not many developers share your opinion that it's the best way to do it.
Anyway, I'm not going to argue any longer, it's your driver and your platform and you do pretty much what you want with it.
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 3/8 I2C: S3C2410: Pass the I2C bus number via drivers platform data
In reply to Ben DooksAllow the platform data to specify the bus bumber that the new I2C bus will be given. This is to allow the use of the board registration mechanism to specify the new style of I2C device registration which allows boards to provide a list of attached devices.
Note, as discussed on the mailing list, we have dropped backwards compatibility of adding an dynamic bus number as it should not affect most boards to have the bus pinned to 0 if they have either not specified platform data for driver. Any board supplying platform data will automatically have the bus_num field set to 0, and anyone who needs the driver on a different bus number can supply platform data to set bus_num.
drivers/i2c/busses/i2c-s3c2410.c +12 -1
@@ -751,9 +751,12 @@ static int s3c24xx_i2c_init(struct s3c24static int s3c24xx_i2c_probe (struct platform_device *pdev) {struct s3c24xx_i2c *i2c = &s3c24xx_i2c; +struct s3c2410_platform_i2c *pdata;struct resource *res;int ret; + pdata =s3c24xx_i2c_get_platformdata (&pdev->dev); +/* find the clock and enable it */ i2c->dev = &pdev->dev;
@@ -831,7 +834,15 @@ static int s3c24xx_i2c_probe(struct platdev_dbg (&pdev->dev,"irq resource %p (%lu)\n" , res, (unsigned long )res->start); - ret =i2c_add_adapter (&i2c->adap); +/* Note, previous versions of the driver used i2c_add_adapter() +* to add an bus at any number. We now pass the bus number via +* the platform data, so if unset it will now default to always +* being bus 0. +*/ + + i2c->adap.nr = pdata->bus_num; + ret =i2c_add_numbered_adapter (&i2c->adap); +if (ret <0 ) {dev_err (&pdev->dev,"failed to add bus to i2c core\n" ); goto err_irq;
include/asm-arm/plat-s3c/iic.h +1 -0
@@ -21,6 +21,7 @@*/ struct s3c2410_platform_i2c { +int bus_num;/* bus number to use */ unsigned int flags;unsigned int slave_addr;/* slave address for controller */ unsigned long bus_freq;/* standard bus frequency */
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Jean Delvare
Acked-byRe: [patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data
Hi Ben,
Quoted 39 lines · Ben Dooks
Allow the platform data to specify the bus bumber that the new I2C bus will be given. This is to allow the use of the board registration mechanism to specify the new style of I2C device registration which allows boards to provide a list of attached devices.
Note, as discussed on the mailing list, we have dropped backwards compatibility of adding an dynamic bus number as it should not affect most boards to have the bus pinned to 0 if they have either not specified platform data for driver. Any board supplying platform data will automatically have the
bus_numfield set to 0, and anyone who needs the driver on a different bus number can supply platform data to setbus_num.Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
===================================================================drivers/i2c/busses/i2c-s3c2410.c +3
-1@@ -751,9 +751,12 @@ static int s3c24xx_i2c_init(struct s3c24static int s3c24xx_i2c_probe (struct platform_device *pdev) {struct s3c24xx_i2c *i2c = &s3c24xx_i2c; +struct s3c2410_platform_i2c *pdata;struct resource *res;int ret; + pdata =s3c24xx_i2c_get_platformdata (&pdev->dev); +/* find the clock and enable it */ i2c->dev = &pdev->dev;@@ -831,7 +834,15 @@ static int s3c24xx_i2c_probe(struct platdev_dbg (&pdev->dev,"irq resource %p (%lu)\n" , res, (unsigned long )res->start); - ret =i2c_add_adapter (&i2c->adap);
drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_probe(struct plat · line 846
Quoted 10 lines
drivers/i2c/busses/i2c-s3c2410.c +9
-0+/* Note, previous versions of the driver used i2c_add_adapter() +* to add an bus at any number. We now pass the bus number via +* the platform data, so if unset it will now default to always +* being bus 0. +*/ + + i2c->adap.nr = pdata->bus_num; + ret =i2c_add_numbered_adapter (&i2c->adap); +if (ret <0 ) {
Minor stylistic comment: in general you shouldn't have blank lines between a function call and the code which checks the values returned by the function in question.
Quoted 8 lines
drivers/i2c/busses/i2c-s3c2410.c +0
-0dev_err (&pdev->dev,"failed to add bus to i2c core\n" ); goto err_irq;Index:
linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
===================================================================
include/asm-arm/plat-s3c/iic.h line 27
Quoted 7 lines
include/asm-arm/plat-s3c/iic.h +1
-0@@ -21,6 +21,7 @@*/ struct s3c2410_platform_i2c { +int bus_num;/* bus number to use */ unsigned int flags;unsigned int slave_addr;/* slave address for controller */ unsigned long bus_freq;/* standard bus frequency */
Other than that, looks good to me.
Do you plan to send this patch (and the next ones in the series) to Linus yourself, or should I take them in my tree?
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data
Quoted 41 lines · Jean Delvare
Hi Ben,
Quoted 38 lines · Ben Dooks
Allow the platform data to specify the bus bumber that the new I2C bus will be given. This is to allow the use of the board registration mechanism to specify the new style of I2C device registration which allows boards to provide a list of attached devices.
Note, as discussed on the mailing list, we have dropped backwards compatibility of adding an dynamic bus number as it should not affect most boards to have the bus pinned to 0 if they have either not specified platform data for driver. Any board supplying platform data will automatically have the
bus_numfield set to 0, and anyone who needs the driver on a different bus number can supply platform data to setbus_num.Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
===================================================================drivers/i2c/busses/i2c-s3c2410.c +3
-0@@ -751,9 +751,12 @@ static int s3c24xx_i2c_init(struct s3c24static int s3c24xx_i2c_probe (struct platform_device *pdev) {struct s3c24xx_i2c *i2c = &s3c24xx_i2c; +struct s3c2410_platform_i2c *pdata;struct resource *res;int ret; + pdata =s3c24xx_i2c_get_platformdata (&pdev->dev); +/* find the clock and enable it */ i2c->dev = &pdev->dev;@@ -831,7 +834,15 @@ static int s3c24xx_i2c_probe(struct platdev_dbg (&pdev->dev,"irq resource %p (%lu)\n" , res, (unsigned long )res->start);
Quoted 15 lines
drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_probe(struct plat · line 846
Quoted 11 lines
drivers/i2c/busses/i2c-s3c2410.c +9
-1- ret =i2c_add_adapter (&i2c->adap); +/* Note, previous versions of the driver used i2c_add_adapter() +* to add an bus at any number. We now pass the bus number via +* the platform data, so if unset it will now default to always +* being bus 0. +*/ + + i2c->adap.nr = pdata->bus_num; + ret =i2c_add_numbered_adapter (&i2c->adap); +if (ret <0 ) {Minor stylistic comment: in general you shouldn't have blank lines between a function call and the code which checks the values returned by the function in question.
Ok, will change this.
Quoted 18 lines
Quoted 8 lines
drivers/i2c/busses/i2c-s3c2410.c +0
-0dev_err (&pdev->dev,"failed to add bus to i2c core\n" ); goto err_irq;Index:
linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
===================================================================include/asm-arm/plat-s3c/iic.h line 27
Quoted 7 lines
include/asm-arm/plat-s3c/iic.h +1
-0@@ -21,6 +21,7 @@*/ struct s3c2410_platform_i2c { +int bus_num;/* bus number to use */ unsigned int flags;unsigned int slave_addr;/* slave address for controller */ unsigned long bus_freq;/* standard bus frequency */ Other than that, looks good to me.
Quoted 4 lines
Acked-by Jean Delvare <[email protected]>Do you plan to send this patch (and the next ones in the series) to Linus yourself, or should I take them in my tree?
No, firstly these are enhancements for the next kernel, and aren't ready for merging yet. Secondly they form a part of a larger series that needs to go to Russell King to avoid any horrible merge problems as some of these machines require other updates.
I was considering sending the first two patches as fixes as soon as possible myself as a first test to see how it all works.
Signature · 10 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year. _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): 20080601215335.GK7334-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]
Re: [patch 3/8] I2C: S3C2410: Pass the I2C bus number via drivers platform data
Hi Ben,
Quoted 8 lines · Ben Dooks
Quoted 2 lines · Jean Delvare
Do you plan to send this patch (and the next ones in the series) to Linus yourself, or should I take them in my tree?
No, firstly these are enhancements for the next kernel, and aren't ready for merging yet. Secondly they form a part of a larger series that needs to go to Russell King to avoid any horrible merge problems as some of these machines require other updates.
OK, I'll leave these patches to you then.
Quoted 2 lines
I was considering sending the first two patches as fixes as soon as possible myself as a first test to see how it all works.
Fine with me as well.
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 4/8 I2C: S3C2410: Add a option of reducing the bus busy waiting time.
In reply to Ben DooksAdd a single-master option to drasitcally reduce the time spent waiting to check the bus is free and ready for another transfer.
drivers/i2c/busses/i2c-s3c2410.c +8 -1
@@ -472,8 +472,15 @@ static irqreturn_t s3c24xx_i2c_irq(int istatic int s3c24xx_i2c_set_master (struct s3c24xx_i2c *i2c) { +struct s3c2410_platform_i2c *pdata;unsigned long iicstat; -int timeout =400 ; +int timeout; + +/* if we are a single master on this bus, reduce the delay awaiting +* for bus traffic to a much lower value. */ + + pdata =s3c24xx_i2c_get_platformdata (i2c->dev); + timeout = (pdata->flags &S3C_IICFLG_SINGLE_MASTER ) ?10 :400 ;while (timeout-- >0 ) { iicstat =readl (i2c->regs +S3C2410_IICSTAT );
include/asm-arm/plat-s3c/iic.h +2 -1
@@ -13,7 +13,8 @@#ifndef __ASM_ARCH_IIC_H#define __ASM_ARCH_IIC_H __FILE__ -#define S3C_IICFLG_FILTER (1<<0)/* enable s3c2440 filter */ +#define S3C_IICFLG_FILTER (1<<0)/* enable s3c2440 filter */ +#define S3C_IICFLG_SINGLE_MASTER (1<<1)/* s3c24xx is only bus master. */ /* Notes: * 1) All frequencies are expressed in Hz
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 4/8] I2C: S3C2410: Add a option of reducing the bus busy waiting time.
Hi Ben,
Quoted 2 lines · Ben Dooks
Add a single-master option to drasitcally reduce the time spent waiting to check the bus is free and ready for another transfer.
I don't understand. If you have a single-master bus, then the bus can simply never be busy when you attempt to start a transaction. So, the timeout setting shouldn't change anything, as you will get the bus immediately anyway. Isn't it the case?
As a side note, this timeout loop is pretty broken anyway, as it depends on the value of HZ. It should be adjusted to be time-based rather than count-based.
Quoted 39 lines
Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt2/drivers/i2c/busses/i2c-s3c2410.c
===================================================================drivers/i2c/busses/i2c-s3c2410.c +8
-1@@ -472,8 +472,15 @@ static irqreturn_t s3c24xx_i2c_irq(int istatic int s3c24xx_i2c_set_master (struct s3c24xx_i2c *i2c) { +struct s3c2410_platform_i2c *pdata;unsigned long iicstat; -int timeout =400 ; +int timeout; + +/* if we are a single master on this bus, reduce the delay awaiting +* for bus traffic to a much lower value. */ + + pdata =s3c24xx_i2c_get_platformdata (i2c->dev); + timeout = (pdata->flags &S3C_IICFLG_SINGLE_MASTER ) ?10 :400 ;while (timeout-- >0 ) { iicstat =readl (i2c->regs +S3C2410_IICSTAT );Index:
linux-2.6.26-rc4-quilt2/include/asm-arm/plat-s3c/iic.h
===================================================================include/asm-arm/plat-s3c/iic.h +2
-1@@ -13,7 +13,8 @@#ifndef __ASM_ARCH_IIC_H#define __ASM_ARCH_IIC_H __FILE__ -#define S3C_IICFLG_FILTER (1<<0)/* enable s3c2440 filter */ +#define S3C_IICFLG_FILTER (1<<0)/* enable s3c2440 filter */ +#define S3C_IICFLG_SINGLE_MASTER (1<<1)/* s3c24xx is only bus master. */ /* Notes: * 1) All frequencies are expressed in Hz
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 5/8 OSIRIS: Add i2c device list to Simtec Osiris
In reply to Ben DooksAdd an i2c board information initialisers to the board to define which devices are present.
arch/arm/mach-s3c2440/mach-osiris.c +18 -1
@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2440/mach-osiris.c * -* Copyright (c) 2005 Simtec Electronics +* Copyright (c) 2005,2008 Simtec Electronics * http://armlinux.simtec.co.uk/ * Ben Dooks <ben-Y5A6D6n0/[email protected]> *
@@ -19,6 +19,7 @@#include <linux/sysdev.h> #include <linux/serial_core.h> #include <linux/clk.h> +#include <linux/i2c.h> #include <asm/mach/arch.h> #include <asm/mach/map.h>
@@ -322,6 +323,19 @@ static struct sys_device osiris_pm_sysde .cls = &osiris_pm_sysclass, }; +/* I2C devices fitted. */ + +static struct i2c_board_info osiris_i2c_info[]__initdata = { + { + .type ="tps65011" , + .addr =0x48 , + .irq =IRQ_EINT20 , + }, { + .type ="eeprom" , + .addr =0x50 , + } +}; +/* Standard Osiris devices */ static struct platform_device *osiris_devices[]__initdata = {
@@ -388,6 +402,9 @@ static void __init osiris_init(void)sysdev_class_register (&osiris_pm_sysclass);sysdev_register (&osiris_pm_sysdev); +i2c_register_board_info (0 , osiris_i2c_info, +ARRAY_SIZE (osiris_i2c_info)); +platform_add_devices (osiris_devices,ARRAY_SIZE (osiris_devices)); };
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris
Hi Ben,
Quoted 24 lines · Ben Dooks
Add an i2c board information initialisers to the board to define which devices are present.
Signed-off-by Ben Dooks <[email protected]>arch/arm/mach-s3c2440/mach-osiris.c +2
-1@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2440/mach-osiris.c * -* Copyright (c) 2005 Simtec Electronics +* Copyright (c) 2005,2008 Simtec Electronics * http://armlinux.simtec.co.uk/ * Ben Dooks <ben-Y5A6D6n0/[email protected]> * @@ -19,6 +19,7 @@#include <linux/sysdev.h> #include <linux/serial_core.h> #include <linux/clk.h> +#include <linux/i2c.h> #include <asm/mach/arch.h> #include <asm/mach/map.h>
arch/arm/mach-s3c2440/mach-osiris.c static struct sys_device osiris_pm_sysde · line 331
Quoted 9 lines
arch/arm/mach-s3c2440/mach-osiris.c +6
-0@@ -322,6 +323,19 @@ static struct sys_device osiris_pm_sysde .cls = &osiris_pm_sysclass, }; +/* I2C devices fitted. */ + +static struct i2c_board_info osiris_i2c_info[]__initdata = { + { + .type ="tps65011" , + .addr =0x48 ,
Please use the I2C_BOARD_INFO helper in all your device declarations.
arch/arm/mach-s3c2440/mach-osiris.c static struct sys_device osiris_pm_sysde · line 336
Quoted 5 lines
arch/arm/mach-s3c2440/mach-osiris.c +5
-0+ .irq = IRQ_EINT20, + }, { + .type = "eeprom", + .addr = 0x50, + }
The "eeprom" driver is not a new-style i2c driver, so it won't be able to bind to device. Having it declared here even prevents the legacy binding to happen, because the address will be busy. So you should either not list the eeprom here and let the legacy eeprom driver pick it later, or add a new-style mode to the eeprom driver - but then you probably want to use David Brownell and Wolfram Sang's at24 driver instead, it's much better.
Quoted 7 lines
arch/arm/mach-s3c2440/mach-osiris.c +2
-0+}; +/* Standard Osiris devices */ static struct platform_device *osiris_devices[]__initdata = {
arch/arm/mach-s3c2440/mach-osiris.c static void __init osiris_init(void) · line 410
Quoted 9 lines
arch/arm/mach-s3c2440/mach-osiris.c +3
-0@@ -388,6 +402,9 @@ static void __init osiris_init(void)sysdev_class_register (&osiris_pm_sysclass);sysdev_register (&osiris_pm_sysdev); +i2c_register_board_info (0 , osiris_i2c_info, +ARRAY_SIZE (osiris_i2c_info)); +platform_add_devices (osiris_devices,ARRAY_SIZE (osiris_devices)); };
Same comments apply to the next 3 patches of the series.
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris
Quoted 39 lines · Jean Delvare
Hi Ben,
Quoted 24 lines · Ben Dooks
Add an i2c board information initialisers to the board to define which devices are present.
Signed-off-by Ben Dooks <[email protected]>arch/arm/mach-s3c2440/mach-osiris.c +2
-1@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2440/mach-osiris.c * -* Copyright (c) 2005 Simtec Electronics +* Copyright (c) 2005,2008 Simtec Electronics * http://armlinux.simtec.co.uk/ * Ben Dooks <ben-Y5A6D6n0/[email protected]> * @@ -19,6 +19,7 @@#include <linux/sysdev.h> #include <linux/serial_core.h> #include <linux/clk.h> +#include <linux/i2c.h> #include <asm/mach/arch.h> #include <asm/mach/map.h> arch/arm/mach-s3c2440/mach-osiris.c static struct sys_device osiris_pm_sysde · line 331
Quoted 9 lines
arch/arm/mach-s3c2440/mach-osiris.c +6
-0@@ -322,6 +323,19 @@ static struct sys_device osiris_pm_sysde .cls = &osiris_pm_sysclass, }; +/* I2C devices fitted. */ + +static struct i2c_board_info osiris_i2c_info[]__initdata = { + { + .type ="tps65011" , + .addr =0x48 ,Please use the
I2C_BOARD_INFOhelper in all your device declarations.
Quoted 13 lines
arch/arm/mach-s3c2440/mach-osiris.c static struct sys_device osiris_pm_sysde · line 336
Quoted 5 lines
arch/arm/mach-s3c2440/mach-osiris.c +5
-0+ .irq = IRQ_EINT20, + }, { + .type = "eeprom", + .addr = 0x50, + }The "eeprom" driver is not a new-style i2c driver, so it won't be able to bind to device. Having it declared here even prevents the legacy binding to happen, because the address will be busy. So you should either not list the eeprom here and let the legacy eeprom driver pick it later, or add a new-style mode to the eeprom driver - but then you probably want to use David Brownell and Wolfram Sang's at24 driver instead, it's much better.
Thanks, I'll change this to at94. Is this driver going to be included on the next kernel window?
Signature · 10 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year. _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): 20080601213509.GH7334-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]
Re: [patch 5/8] OSIRIS: Add i2c device list to Simtec Osiris
Quoted 11 lines · Ben Dooks
Quoted 7 lines · Jean Delvare
The "eeprom" driver is not a new-style i2c driver, so it won't be able to bind to device. Having it declared here even prevents the legacy binding to happen, because the address will be busy. So you should either not list the eeprom here and let the legacy eeprom driver pick it later, or add a new-style mode to the eeprom driver - but then you probably want to use David Brownell and Wolfram Sang's at24 driver instead, it's much better.
Thanks, I'll change this to at94. Is this driver going to be included on the next kernel window?
at24, not at94 ;) I hope so. I've reviewed the driver and am now waiting for Wolfram to send an updated version. The code looked good so if I receive something soon enough, there's really no reason why it wouldn't make it in kernel 2.6.27.
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 6/8 BAST: Add i2c device list on Simtec Bast
In reply to Ben DooksAdd i2c boardinfo for the connected i2c devices on the Simtec Bast.
arch/arm/mach-s3c2410/mach-bast.c +22 -1
@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2410/mach-bast.c * -* Copyright (c) 2003-2005 Simtec Electronics +* Copyright (c) 2003-2005,2008 Simtec Electronics * Ben Dooks <ben-Y5A6D6n0/[email protected]> * * http://www.simtec.co.uk/products/EB2410ITX/
@@ -21,6 +21,7 @@#include <linux/platform_device.h> #include <linux/dm9000.h> #include <linux/ata_platform.h> +#include <linux/i2c.h> #include <net/ax88796.h>
@@ -534,6 +535,23 @@ static struct s3c2410fb_mach_info __init .default_display = 1, }; +/* I2C devices fitted. */ + +static struct i2c_board_info bast_i2c_devs[]__initdata = { + { + .type ="tlv320aic23" , + .addr =0x1a , + }, { + .type ="eeprom" , + .addr =0x50 , + }, { + .type ="simtec-pmu" , + .addr =0x6b , + }, { + .type ="ch7013" , + .addr =0x75 , + }, +};/* Standard BAST devices */
@@ -593,6 +611,9 @@ static void __init bast_init(void)s3c24xx_fb_set_platdata (&bast_fb_info);platform_add_devices (bast_devices,ARRAY_SIZE (bast_devices)); +i2c_register_board_info (0 , bast_i2c_devs, +ARRAY_SIZE (bast_i2c_devs)); +nor_simtec_init (); }
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 7/8 VR1000: Add i2c device list to Thorcom VR1000
In reply to Ben DooksAdd i2c board intialisers to specify the I2C devices attached on the Thorcom VR1000.
arch/arm/mach-s3c2410/mach-vr1000.c +23 -1
@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2410/mach-vr1000.c * -* Copyright (c) 2003-2005 Simtec Electronics +* Copyright (c) 2003-2005,2008 Simtec Electronics * Ben Dooks <ben-Y5A6D6n0/[email protected]> * * Machine support for Thorcom VR1000 board. Designed for Thorcom by
@@ -19,6 +19,7 @@#include <linux/timer.h> #include <linux/init.h> #include <linux/dm9000.h> +#include <linux/i2c.h> #include <linux/serial.h> #include <linux/tty.h>
@@ -315,6 +316,24 @@ static struct platform_device vr1000_led }, }; +/* I2C devices. */ + +static struct i2c_board_info vr1000_i2c_info[]__initdata = { + { + .type ="tlv320aic23" , + .addr =0x1a , + }, { + .type ="tmp101" , + .addr =0x48 , + }, { + .type ="eeprom" , + .addr =0x50 , + }, { + .type ="m41st87" , + .addr =0x68 , + }, +}; +/* devices for this board */ static struct platform_device *vr1000_devices[]__initdata = {
@@ -373,6 +392,9 @@ static void __init vr1000_init(void) {platform_add_devices (vr1000_devices,ARRAY_SIZE (vr1000_devices)); +i2c_register_board_info (0 , vr1000_i2c_info, +ARRAY_SIZE (vr1000_i2c_info)); +nor_simtec_init (); }
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 7/8] VR1000: Add i2c device list to Thorcom VR1000
Quoted 28 lines · Ben Dooks
Add i2c board intialisers to specify the I2C devices attached on the Thorcom VR1000.
Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c
===================================================================arch/arm/mach-s3c2410/mach-vr1000.c +2
-1@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2410/mach-vr1000.c * -* Copyright (c) 2003-2005 Simtec Electronics +* Copyright (c) 2003-2005,2008 Simtec Electronics * Ben Dooks <ben-Y5A6D6n0/[email protected]> * * Machine support for Thorcom VR1000 board. Designed for Thorcom by @@ -19,6 +19,7 @@#include <linux/timer.h> #include <linux/init.h> #include <linux/dm9000.h> +#include <linux/i2c.h> #include <linux/serial.h> #include <linux/tty.h> @@ -315,6 +316,24 @@ static struct platform_device vr1000_led }, };
arch/arm/mach-s3c2410/mach-vr1000.c static struct platform_device vr1000_led · line 327
Quoted 10 lines
arch/arm/mach-s3c2410/mach-vr1000.c +9
-0+/* I2C devices. */ + +static struct i2c_board_info vr1000_i2c_info[]__initdata = { + { + .type ="tlv320aic23" , + .addr =0x1a , + }, { + .type ="tmp101" , + .addr =0x48 ,
Same problem as "eeprom"... The lm75 driver doesn't yet support the "tmp101" as a new-style device in mainline, so this won't work and will prevent the lm75 driver from attaching as a legacy driver too. So this device declaration should be delayed or commented out until the lm75 driver is ready.
Quoted 23 lines
arch/arm/mach-s3c2410/mach-vr1000.c +12
-0+ }, { + .type = "eeprom", + .addr = 0x50, + }, { + .type = "m41st87", + .addr = 0x68, + }, +}; +/* devices for this board */ static struct platform_device *vr1000_devices[] __initdata = {@@ -373,6 +392,9 @@ static void __init vr1000_init(void) {platform_add_devices (vr1000_devices,ARRAY_SIZE (vr1000_devices)); +i2c_register_board_info (0 , vr1000_i2c_info, +ARRAY_SIZE (vr1000_i2c_info)); +nor_simtec_init (); }
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [patch 7/8] VR1000: Add i2c device list to Thorcom VR1000
Quoted 30 lines · Jean Delvare
Quoted 29 lines · Ben Dooks
Add i2c board intialisers to specify the I2C devices attached on the Thorcom VR1000.
Signed-off-by Ben Dooks <[email protected]>Index:
linux-2.6.26-rc4-quilt1/arch/arm/mach-s3c2410/mach-vr1000.c
===================================================================arch/arm/mach-s3c2410/mach-vr1000.c +2
-1@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2410/mach-vr1000.c * -* Copyright (c) 2003-2005 Simtec Electronics +* Copyright (c) 2003-2005,2008 Simtec Electronics * Ben Dooks <ben-Y5A6D6n0/[email protected]> * * Machine support for Thorcom VR1000 board. Designed for Thorcom by @@ -19,6 +19,7 @@#include <linux/timer.h> #include <linux/init.h> #include <linux/dm9000.h> +#include <linux/i2c.h> #include <linux/serial.h> #include <linux/tty.h> @@ -315,6 +316,24 @@ static struct platform_device vr1000_led }, };
Quoted 15 lines
arch/arm/mach-s3c2410/mach-vr1000.c static struct platform_device vr1000_led · line 327
Quoted 9 lines
arch/arm/mach-s3c2410/mach-vr1000.c +9
-0+/* I2C devices. */ + +static struct i2c_board_info vr1000_i2c_info[]__initdata = { + { + .type ="tlv320aic23" , + .addr =0x1a , + }, { + .type ="tmp101" , + .addr =0x48 ,Same problem as "eeprom"... The lm75 driver doesn't yet support the "tmp101" as a new-style device in mainline, so this won't work and will prevent the lm75 driver from attaching as a legacy driver too. So this device declaration should be delayed or commented out until the lm75 driver is ready.
I'll think about that one, although it does take a force to load the lm75 on current kernels due to it not being 'truly' lm75 compatible. Is there any chance of the new driver being merged during the next change window?
PS, I've done a quick test of the new lm75 driver and it seems to work fine for me and would like to see a solution where we don't need to force probe the lm75 module.
Signature · 10 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year. _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
A message that is not in the archive (sent off-list or not yet received): 20080601213855.GI7334-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]
Re: [patch 7/8] VR1000: Add i2c device list to Thorcom VR1000
Quoted 18 lines · Ben Dooks
Quoted 16 lines · Jean Delvare
arch/arm/mach-s3c2410/mach-vr1000.c static struct platform_device vr1000_led · line 327
Quoted 9 lines · Ben Dooks
arch/arm/mach-s3c2410/mach-vr1000.c +9
-0+/* I2C devices. */ + +static struct i2c_board_info vr1000_i2c_info[]__initdata = { + { + .type ="tlv320aic23" , + .addr =0x1a , + }, { + .type ="tmp101" , + .addr =0x48 ,Same problem as "eeprom"... The lm75 driver doesn't yet support the "tmp101" as a new-style device in mainline, so this won't work and will prevent the lm75 driver from attaching as a legacy driver too. So this device declaration should be delayed or commented out until the lm75 driver is ready.
Quoted 2 lines
I'll think about that one, although it does take a force to load the lm75 on current kernels due to it not being 'truly' lm75 compatible.
That's still better than not being able to load the driver at all, which will happen with your device declaration above without the updated lm75 driver.
Quoted 2 lines
Is there
any chance of the new driver being merged during the next change window?
Depends on Mark M. Hoffman (Cc'd), not me. I've reviewed and acked the lm75 patches, but it's up to Mark to merge them.
Quoted 3 lines
PS, I've done a quick test of the new lm75 driver and it seems to work fine for me and would like to see a solution where we don't need to force probe the lm75 module.
Of course, the updated driver is the way to go in your case.
Signature · 6 lines
Jean Delvare _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
PATCH 8/8 ANUBIS: Add i2c device list to Simtec Anubis
In reply to Ben DooksAdd i2c board info initialiser to setup the list of I2C devices present on an Simtec Anubis.
arch/arm/mach-s3c2440/mach-anubis.c +18 -1
@@ -1,6 +1,6 @@/* linux/arch/arm/mach-s3c2440/mach-anubis.c * -* Copyright (c) 2003-2005 Simtec Electronics +* Copyright (c) 2003-2005,2008 Simtec Electronics * http://armlinux.simtec.co.uk/ * Ben Dooks <ben-Y5A6D6n0/[email protected]> *
@@ -18,6 +18,7 @@#include <linux/serial_core.h> #include <linux/platform_device.h> #include <linux/ata_platform.h> +#include <linux/i2c.h> #include <linux/sm501.h> #include <linux/sm501-regs.h>
@@ -421,6 +422,19 @@ static struct clk *anubis_clocks[] = { &s3c24xx_uclk, }; +/* I2C devices. */ + +static struct i2c_board_info anubis_i2c_info[]__initdata = { + { + .type ="tps65011" , + .addr =0x48 , + .irq =IRQ_EINT20 , + }, { + .type ="eeprom" , + .addr =0x50 , + } +}; +static void __init anubis_map_io (void ) {/* initialise the clocks */
@@ -460,6 +474,9 @@ static void __init anubis_map_io(void)static void __init anubis_init (void ) {platform_add_devices (anubis_devices,ARRAY_SIZE (anubis_devices)); + +i2c_register_board_info (0 , anubis_i2c_info, +ARRAY_SIZE (anubis_i2c_info)); }
Signature · 8 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes' _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c