Skip to contentSkip to conversation

i2c: change the id to let the i2c device work

PATCH New Bo Shen 3 participants 12 messages Oct 12, 2012

Subsystems (4)
lore
Share & tools
lore

Bo Shen

author
#0 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

As the old method will use platform device id, change the id to let the i2c device work

---
 arch/arm/mach-at91/at91sam9260_devices.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-at91/at91sam9260_devices.c +1 -1
index 5c5966a..aad76b7 100644
@@ -471,7 +471,7 @@ static struct i2c_gpio_platform_data pdata = {
 
 static struct platform_device at91sam9260_twi_device = {
 	.name			= "i2c-gpio",
-	.id			= -1,
+	.id			= 0,
 	.dev.platform_data	= &pdata,
 };
 
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 2 lines · Bo Shen

As the old method will use platform device id, change the id to let the i2c device work

What are "the old method" and new method? You're not explaining why this is needed...

arch/arm/mach-at91/at91sam9260_devices.c static struct i2c_gpio_platform_data pdata = { · line 475

Quoted 5 lines

static struct platform_device at91sam9260_twi_device = {

arch/arm/mach-at91/at91sam9260_devices.c +1 -1
 	.name			= "i2c-gpio",
-	.id			= -1,
+	.id			= 0,
 	.dev.platform_data	= &pdata,

This looks like a regression, if there's only one of a given type of device in the system it should have no numeric ID in the display name so having -1 is appropriate.

author
#2 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Hi Mark,

Quoted 6 lines · Mark Brown
Quoted 2 lines · Bo Shen

As the old method will use platform device id, change the id to let the i2c device work

What are "the old method" and new method? You're not explaining why this is needed...

Maybe use the 'legacy method' will be better (I am not sure). Now, the Linux kernel is transferring to device tree which doesn't use platform device id. So, change the id to let the legacy code work.

May you understand.

Quoted 11 lines

arch/arm/mach-at91/at91sam9260_devices.c static struct i2c_gpio_platform_data pdata = { · line 475

Quoted 6 lines
  static struct platform_device at91sam9260_twi_device = {
  	.name			= "i2c-gpio",
arch/arm/mach-at91/at91sam9260_devices.c +1 -1
-	.id			= -1,
+	.id			= 0,
  	.dev.platform_data	= &pdata,

This looks like a regression, if there's only one of a given type of device in the system it should have no numeric ID in the display name so having -1 is appropriate.

#3 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 6 lines · Bo Shen
Quoted 4 lines · Mark Brown
Quoted 2 lines · Bo Shen

As the old method will use platform device id, change the id to let the i2c device work

Quoted 8 lines
Quoted 2 lines

What are "the old method" and new method? You're not explaining why this is needed...

Maybe use the 'legacy method' will be better (I am not sure). Now, the Linux kernel is transferring to device tree which doesn't use platform device id. So, change the id to let the legacy code work.

May you understand.

No, this still makes no sense to me. This is clearly a non-DT device registration, what does DT have to do with anything here? What is the practical problem you are seeing in your system and how does this help with it?

author
#4 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 10 lines · Mark Brown
Quoted 8 lines · Bo Shen
Quoted 7 lines · Mark Brown
Quoted 2 lines · Bo Shen

As the old method will use platform device id, change the id to let the i2c device work

What are "the old method" and new method? You're not explaining why this is needed...

Quoted 8 lines
Quoted 5 lines

Maybe use the 'legacy method' will be better (I am not sure). Now, the Linux kernel is transferring to device tree which doesn't use platform device id. So, change the id to let the legacy code work.

May you understand.

No, this still makes no sense to me. This is clearly a non-DT device registration, what does DT have to do with anything here? What is the

Yes, this is non-DT device registration.
As the Linux kernel code is transferring to DT which doesn't use platform device id. however here, modified unconsciously. So correct this error.

Quoted 2 lines

practical problem you are seeing in your system and how does this help with it?

In non-DT kernel, we use platform device id to distinguish between each device.

For example,
if register i2c device on i2c bus 0, using:
i2c_register_board_info(0, ...)
if register i2c device on i2c bus 1, using:
i2c_register_board_info(1, ...)

So, in this case, if the id = -1, i2c core will dynamically assign a bus id to this bus when running, the dynamically assigned bus id is unknown when writing the code. So, when we use i2c_register_board_info(int busnum, ...) to register device on busnum. we don't know which value to be use.

So, this patch will fix this issue.

BRs,
Bo Shen

#5 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 9 lines · Bo Shen
Quoted 2 lines · Mark Brown

No, this still makes no sense to me. This is clearly a non-DT device registration, what does DT have to do with anything here? What is the

Yes, this is non-DT device registration.
As the Linux kernel code is transferring to DT which doesn't use platform device id. however here, modified unconsciously. So correct this error.

What error and what does DT have to do with any of this? DT has no impact on non-DT systems.

Quoted 11 lines
Quoted 2 lines

practical problem you are seeing in your system and how does this help with it?

In non-DT kernel, we use platform device id to distinguish between each device.

So, in this case, if the id = -1, i2c core will dynamically assign a bus id to this bus when running, the dynamically assigned bus id is unknown when writing the code. So, when we use i2c_register_board_info(int busnum, ...) to register device on busnum. we don't know which value to be use.

The I2C bus number assigned to the controller should be independant of the platform device ID used to register the device; a better fix if this is an issue would be to update the i2c-gpio driver to allow a fixed bus number to be specified in platform data.

author
#6 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 6 lines · Mark Brown
Quoted 4 lines · Bo Shen
Quoted 2 lines · Mark Brown

No, this still makes no sense to me. This is clearly a non-DT device registration, what does DT have to do with anything here? What is the

Quoted 7 lines
Quoted 4 lines

Yes, this is non-DT device registration.
As the Linux kernel code is transferring to DT which doesn't use platform device id. however here, modified unconsciously. So correct this error.

What error and what does DT have to do with any of this? DT has no impact on non-DT systems.

Yes. This has nothing to do with DT.

Quoted 12 lines
Quoted 11 lines
Quoted 2 lines

practical problem you are seeing in your system and how does this help with it?

In non-DT kernel, we use platform device id to distinguish between each device.

So, in this case, if the id = -1, i2c core will dynamically assign a bus id to this bus when running, the dynamically assigned bus id is unknown when writing the code. So, when we use i2c_register_board_info(int busnum, ...) to register device on busnum. we don't know which value to be use.

Quoted 4 lines

The I2C bus number assigned to the controller should be independant of the platform device ID used to register the device; a better fix if this is an issue would be to update the i2c-gpio driver to allow a fixed bus number to be specified in platform data.

Hi Haavard Skinnemoen,
Do you have any suggestions about this?
Thanks.

BRs,
Bo Shen

#7 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

In reply to Mark Brown
Quoted 11 lines · Mark Brown
Quoted 5 lines · Bo Shen

So, in this case, if the id = -1, i2c core will dynamically assign a bus id to this bus when running, the dynamically assigned bus id is unknown when writing the code. So, when we use i2c_register_board_info(int busnum, ...) to register device on busnum. we don't know which value to be use.

The I2C bus number assigned to the controller should be independant of the platform device ID used to register the device; a better fix if this is an issue would be to update the i2c-gpio driver to allow a fixed bus number to be specified in platform data.

i2c-gpio does support this already.

Signature · 1 line
Jean Delvare
author
#8 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Hi Jean Delvare,

Quoted 8 lines · Jean Delvare
Quoted 7 lines · Mark Brown
Quoted 5 lines · Bo Shen

So, in this case, if the id = -1, i2c core will dynamically assign a bus id to this bus when running, the dynamically assigned bus id is unknown when writing the code. So, when we use i2c_register_board_info(int busnum, ...) to register device on busnum. we don't know which value to be use.

Quoted 6 lines
Quoted 4 lines

The I2C bus number assigned to the controller should be independant of the platform device ID used to register the device; a better fix if this is an issue would be to update the i2c-gpio driver to allow a fixed bus number to be specified in platform data.

i2c-gpio does support this already.

vim I only see the i2c-gpio platform data structure as following:
--<---------------------

struct i2c_gpio_platform_data {
	unsigned int	sda_pin;
	unsigned int	scl_pin;
	int		udelay;
	int		timeout;
	unsigned int	sda_is_open_drain:1;
	unsigned int	scl_is_open_drain:1;
	unsigned int	scl_is_output_only:1;
};

-->---------------------

So, how to allow a fixed bus number to be specified in platform data?

#9 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 11 lines · Bo Shen

vim I only see the i2c-gpio platform data structure as following:
--<---------------------

struct i2c_gpio_platform_data {
	unsigned int	sda_pin;
	unsigned int	scl_pin;
	int		udelay;
	int		timeout;
	unsigned int	sda_is_open_drain:1;
	unsigned int	scl_is_open_drain:1;
	unsigned int	scl_is_output_only:1;
};
Quoted 1 line

-->---------------------

Ah sorry I misread Mark's request. i2c-gpio will turn the platform device ID into bus number, it can indeed not be forced through platform data. But I don't think any other i2c bus driver allows this either. I don't quite see the problem with setting a platform device ID even if there's only one instance of the platform device. I have many examples of this on my machine:
Fixed MDIO bus.0
coretemp.0
vesafb.0

So please just set the platform device ID to 0 (or whatever i2c adapter number you want) and your problem is solved. As you just proposed initially, actually :)

Signature · 1 line
Jean Delvare
#10 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 9 lines · Jean Delvare

Ah sorry I misread Mark's request. i2c-gpio will turn the platform device ID into bus number, it can indeed not be forced through platform data. But I don't think any other i2c bus driver allows this either. I don't quite see the problem with setting a platform device ID even if there's only one instance of the platform device. I have many examples of this on my machine:
Fixed MDIO bus.0
coretemp.0
vesafb.0

This is generally bad style; if it's required by APIs we really should be fixing the APIs to remove this sort of dependency. Aside from the ugliness it tends to be fragile.

Quoted 3 lines

So please just set the platform device ID to 0 (or whatever i2c adapter number you want) and your problem is solved. As you just proposed initially, actually :)

Though it *does* need a comprehensible commit message so people can understand what on earth the change is intended to do.

author
#11 lore raw

PATCH 1/1 i2c: change the id to let the i2c device work

Quoted 16 lines · Mark Brown
Quoted 9 lines · Jean Delvare

Ah sorry I misread Mark's request. i2c-gpio will turn the platform device ID into bus number, it can indeed not be forced through platform data. But I don't think any other i2c bus driver allows this either. I don't quite see the problem with setting a platform device ID even if there's only one instance of the platform device. I have many examples of this on my machine:
Fixed MDIO bus.0
coretemp.0
vesafb.0

This is generally bad style; if it's required by APIs we really should be fixing the APIs to remove this sort of dependency. Aside from the ugliness it tends to be fragile.

Quoted 6 lines
Quoted 3 lines

So please just set the platform device ID to 0 (or whatever i2c adapter number you want) and your problem is solved. As you just proposed initially, actually :)

Though it *does* need a comprehensible commit message so people can understand what on earth the change is intended to do.

I will update the commit message and send v2 patch. Thanks

BR,
Bo Shen