* i2c-eg20t: regression since i2c_add_numbered_adapter change @ 2012-08-22 6:30 Alexander Stein 2012-08-22 7:29 ` Feng Tang 0 siblings, 1 reply; 13+ messages in thread From: Alexander Stein @ 2012-08-22 6:30 UTC (permalink / raw) To: linux-i2c Cc: linux-kernel, Jean Delvare (PC drivers, core), Ben Dooks (embedded platforms), Feng Tang, Tomoya MORINAGA Hello, I just noticed the 3.4 linux kernel fails to sucessfully probe the i2c-eg20t driver. I returns with EBUSY error. It worked on the 3.0 kernel. To my view it is caused the commit 07e8a51ff68353e01d795cceafbac9f54c49132b ( i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number). The reason it actually fails is that the i2c-isch driver is registered beforehand which gets bus number 0. But this one is the bus number the eg20t driver wants to register. A possibility is that if i2c_add_numbered_adapter failed with EBUSY just use i2c_add_adapter to get at least the driver working, but with a non-fixed bus number. Opinions? Best regards, Alexander ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-22 6:30 i2c-eg20t: regression since i2c_add_numbered_adapter change Alexander Stein @ 2012-08-22 7:29 ` Feng Tang 2012-08-22 7:57 ` Alexander Stein 0 siblings, 1 reply; 13+ messages in thread From: Feng Tang @ 2012-08-22 7:29 UTC (permalink / raw) To: Alexander Stein Cc: linux-i2c, linux-kernel, Jean Delvare (PC drivers, core), Ben Dooks (embedded platforms), Tomoya MORINAGA On Wed, 22 Aug 2012 08:30:35 +0200 Alexander Stein <alexander.stein@systec-electronic.com> wrote: > Hello, > > I just noticed the 3.4 linux kernel fails to sucessfully probe the i2c-eg20t > driver. I returns with EBUSY error. It worked on the 3.0 kernel. To my view it > is caused the commit 07e8a51ff68353e01d795cceafbac9f54c49132b ( i2c-eg20t: use > i2c_add_numbered_adapter to get a fixed bus number). > The reason it actually fails is that the i2c-isch driver is registered > beforehand which gets bus number 0. But this one is the bus number the eg20t > driver wants to register. Make sense. > A possibility is that if i2c_add_numbered_adapter failed with EBUSY just use > i2c_add_adapter to get at least the driver working, but with a non-fixed bus > number. Opinions? Or can we give it a fixed offset, like let the i2c_eg20t controller bus number start with 4? I don't expect there will be more than 4 other i2c controllers on EG20T compatible platforms. Thanks, Feng ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-22 7:29 ` Feng Tang @ 2012-08-22 7:57 ` Alexander Stein 2012-08-22 8:04 ` Feng Tang 0 siblings, 1 reply; 13+ messages in thread From: Alexander Stein @ 2012-08-22 7:57 UTC (permalink / raw) To: Feng Tang Cc: linux-i2c, linux-kernel, Jean Delvare (PC drivers, core), Ben Dooks (embedded platforms), Tomoya MORINAGA Hello, On Wednesday 22 August 2012 15:29:18, Feng Tang wrote: > On Wed, 22 Aug 2012 08:30:35 +0200 > Alexander Stein <alexander.stein@systec-electronic.com> wrote: > > > Hello, > > > > I just noticed the 3.4 linux kernel fails to sucessfully probe the i2c-eg20t > > driver. I returns with EBUSY error. It worked on the 3.0 kernel. To my view it > > is caused the commit 07e8a51ff68353e01d795cceafbac9f54c49132b ( i2c-eg20t: use > > i2c_add_numbered_adapter to get a fixed bus number). > > The reason it actually fails is that the i2c-isch driver is registered > > beforehand which gets bus number 0. But this one is the bus number the eg20t > > driver wants to register. > > Make sense. > > > A possibility is that if i2c_add_numbered_adapter failed with EBUSY just use > > i2c_add_adapter to get at least the driver working, but with a non-fixed bus > > number. Opinions? > > Or can we give it a fixed offset, like let the i2c_eg20t controller bus number > start with 4? I don't expect there will be more than 4 other i2c controllers > on EG20T compatible platforms. Why use a fixed one? Give the driver (and maybe every i2c bus driver) a parameter which sets the base bus number it should use. E.g. i2c-eg20t.base-bus-num=2 so it will register the bus numbers starting from 2. If this parameter is unset. It would use the first free one, thus simply using i2c_add_adapter. Regards, Alexander ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-22 7:57 ` Alexander Stein @ 2012-08-22 8:04 ` Feng Tang 2012-08-22 9:17 ` Alexander Stein 0 siblings, 1 reply; 13+ messages in thread From: Feng Tang @ 2012-08-22 8:04 UTC (permalink / raw) To: Alexander Stein Cc: linux-i2c, linux-kernel, Jean Delvare (PC drivers, core), Ben Dooks (embedded platforms), Tomoya MORINAGA Hi Alexander, On Wed, 22 Aug 2012 09:57:07 +0200 Alexander Stein <alexander.stein@systec-electronic.com> wrote: > Hello, > > On Wednesday 22 August 2012 15:29:18, Feng Tang wrote: > > On Wed, 22 Aug 2012 08:30:35 +0200 > > Alexander Stein <alexander.stein@systec-electronic.com> wrote: > > > > > Hello, > > > > > > I just noticed the 3.4 linux kernel fails to sucessfully probe the i2c-eg20t > > > driver. I returns with EBUSY error. It worked on the 3.0 kernel. To my view it > > > is caused the commit 07e8a51ff68353e01d795cceafbac9f54c49132b ( i2c-eg20t: use > > > i2c_add_numbered_adapter to get a fixed bus number). > > > The reason it actually fails is that the i2c-isch driver is registered > > > beforehand which gets bus number 0. But this one is the bus number the eg20t > > > driver wants to register. > > > > Make sense. > > > > > A possibility is that if i2c_add_numbered_adapter failed with EBUSY just use > > > i2c_add_adapter to get at least the driver working, but with a non-fixed bus > > > number. Opinions? > > > > Or can we give it a fixed offset, like let the i2c_eg20t controller bus number > > start with 4? I don't expect there will be more than 4 other i2c controllers > > on EG20T compatible platforms. > > Why use a fixed one? Give the driver (and maybe every i2c bus driver) a parameter which sets the base bus number it should use. > E.g. i2c-eg20t.base-bus-num=2 so it will register the bus numbers starting from 2. If this parameter is unset. It would use the first free one, thus simply using i2c_add_adapter. The reason we need a fixed number is it is easier for platform code which needs to register dozens of i2c devices to different controllers with i2c_register_board_info, and they need provide a bus number for each i2c device, this _binding_ info is not detectable but have to be fixed. Yes, your module parameter "base-bus-num" sounds like a plan too, at the cost of some extra setting in the kernel cmdline. And I'm fine with all solutions as long as I can get a _fixed_ bus number so that I can easily write the platform config code (I guess this is same for device tree, and even ACPI 5.0) Thanks, Feng ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-22 8:04 ` Feng Tang @ 2012-08-22 9:17 ` Alexander Stein 2012-08-23 8:28 ` Feng Tang 0 siblings, 1 reply; 13+ messages in thread From: Alexander Stein @ 2012-08-22 9:17 UTC (permalink / raw) To: Feng Tang Cc: linux-i2c, linux-kernel, Jean Delvare (PC drivers, core), Ben Dooks (embedded platforms), Tomoya MORINAGA Hello, Am Mittwoch, 22. August 2012, 16:04:39 schrieb Feng Tang: > > Why use a fixed one? Give the driver (and maybe every i2c bus driver) a parameter which sets the base bus number it should use. > > E.g. i2c-eg20t.base-bus-num=2 so it will register the bus numbers starting from 2. If this parameter is unset. It would use the first free one, thus simply using i2c_add_adapter. > > The reason we need a fixed number is it is easier for platform code > which needs to register dozens of i2c devices to different controllers > with i2c_register_board_info, and they need provide a bus number for > each i2c device, this _binding_ info is not detectable but have to > be fixed. Yes, I'm aware of that. With "Why use a fixed one?" I meant why hard-code it into the driver. I should be changeable. Because this is/was not possible in general to use i2c_register_board_info, so we used an echo to /sys/bus/i2c/devices/i2c-0/new_device or /sys/bus/i2c/devices/i2c-1/new_device. > Yes, your module parameter "base-bus-num" sounds like a plan too, > at the cost of some extra setting in the kernel cmdline. And I'm fine > with all solutions as long as I can get a _fixed_ bus number so that > I can easily write the platform config code (I guess this is same > for device tree, and even ACPI 5.0) i2c_add_numbered_adapter only works reliably if only using a single driver. If you are using several, especially if one uses dynamic bus numbers, you're kinda screwed. So, not very driver can add busses starting at bus number 0. I guess using an Intel Atom (i2c-isch) with an eg20t is not that uncommon, so you'll end up using 2 drivers where each one should have a fixed number. Some mechanism like udev for renaming ethernet or block devices is needed here. Regards, Alexander ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-22 9:17 ` Alexander Stein @ 2012-08-23 8:28 ` Feng Tang 2012-08-29 18:40 ` Jean Delvare 0 siblings, 1 reply; 13+ messages in thread From: Feng Tang @ 2012-08-23 8:28 UTC (permalink / raw) To: Alexander Stein Cc: linux-i2c, linux-kernel, Jean Delvare (PC drivers, core), Ben Dooks (embedded platforms), Tomoya MORINAGA Hi, On Wed, 22 Aug 2012 11:17:51 +0200 Alexander Stein <alexander.stein@systec-electronic.com> wrote: > Hello, > > Am Mittwoch, 22. August 2012, 16:04:39 schrieb Feng Tang: > > > Why use a fixed one? Give the driver (and maybe every i2c bus driver) a parameter which sets the base bus number it should use. > > > E.g. i2c-eg20t.base-bus-num=2 so it will register the bus numbers starting from 2. If this parameter is unset. It would use the first free one, thus simply using i2c_add_adapter. > > > > The reason we need a fixed number is it is easier for platform code > > which needs to register dozens of i2c devices to different controllers > > with i2c_register_board_info, and they need provide a bus number for > > each i2c device, this _binding_ info is not detectable but have to > > be fixed. > > Yes, I'm aware of that. With "Why use a fixed one?" I meant why hard-code it into the driver. I should be changeable. > Because this is/was not possible in general to use i2c_register_board_info, so we used an echo to /sys/bus/i2c/devices/i2c-0/new_device or /sys/bus/i2c/devices/i2c-1/new_device. Yeah, our EG20T kernel used to use the same "echo /sys/bus/i2c/devices/i2c-x/new_device" way, but we found out it is not convenient for: 1. needs extra user space script, why not make it just work in kernel after boot? We have several i2c devices like touchscreen/radio which we wants them just work without depending user space action. 2. The i2c bus number is not fixed, which make the user space script even harder, as that number depends whether we compile into kernel all the i2c controllers (eg20t and isch) and whether these driver are compiled as modules and their loading order. Thus we come out with this fixed bus number registering. As I said before, either your module parameter "base_bus_num" solution or my fixed bus number offset are ok to me. Will you cook a patch? Thanks, Feng ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-23 8:28 ` Feng Tang @ 2012-08-29 18:40 ` Jean Delvare 2012-08-30 7:49 ` Alexander Stein 2012-08-30 9:10 ` Feng Tang 0 siblings, 2 replies; 13+ messages in thread From: Jean Delvare @ 2012-08-29 18:40 UTC (permalink / raw) To: Feng Tang Cc: Alexander Stein, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA Hi guys, Sorry for joining the discussion a little late, I was on vacation. On Thu, 23 Aug 2012 16:28:52 +0800, Feng Tang wrote: > On Wed, 22 Aug 2012 11:17:51 +0200 > Alexander Stein <alexander.stein@systec-electronic.com> wrote: > > Am Mittwoch, 22. August 2012, 16:04:39 schrieb Feng Tang: > > > > Why use a fixed one? Give the driver (and maybe every i2c bus driver) a parameter which sets the base bus number it should use. > > > > E.g. i2c-eg20t.base-bus-num=2 so it will register the bus numbers starting from 2. If this parameter is unset. It would use the first free one, thus simply using i2c_add_adapter. Looks like what media and sound drivers are/were doing to assign fixed numbers to their devices. But my understanding is that this is a legacy thing and nobody should need to use that any longer. Adding this to all or even some i2c bus drivers looks like the wrong example to follow. If your system has more than one device supported by the driver, it doesn't even reliably guarantee fixed I2C bus numbers (especially if some can be hot-plugged.) > > > The reason we need a fixed number is it is easier for platform code > > > which needs to register dozens of i2c devices to different controllers > > > with i2c_register_board_info, and they need provide a bus number for > > > each i2c device, this _binding_ info is not detectable but have to > > > be fixed. Whenever you call i2c_register_board_info(), every I2C bus number referenced in the I2C device list passed as a parameter is reserved for static I2C bus numbers, dynamic I2C bus numbers will never overlap. So in the quoted example, if i2c-isch is able to dynamically pick I2C bus number 0 while i2c-eg20t want it statically, it means that either no device was declared on bus 0 with i2c_register_board_info(), or i2c_register_board_info() was called too late in the game. Note that there was an assumption at the time the code was written, that there was no need or reason to reserve a static I2C bus number if no slave device was declared on said I2C bus. I never much liked it but it never caused problems so far. This means that either: * you call i2c_register_board_info() to register your slave I2C devices and all the affected I2C bus drivers call i2c_add_numbered_adapter(); or * you don't call i2c_register_board_info() and all I2C bus drivers call i2c_add_adapter(). You can't mix, i.e. if you don't register any slave device on a bus but the bus driver still calls i2c_add_numbered_adapter(), it may fail. If this is a problem now on some systems, it should be easy enough to work around by adding a specific function to reserve an I2C bus number for static allocation, even without declaring any slave device on it. This function would be called at the same time i2c_register_board_info() typically is. > > Yes, I'm aware of that. With "Why use a fixed one?" I meant why hard-code it into the driver. I should be changeable. > > Because this is/was not possible in general to use i2c_register_board_info, so we used an echo to /sys/bus/i2c/devices/i2c-0/new_device or /sys/bus/i2c/devices/i2c-1/new_device. Please elaborate on "this is/was not possible in general to use i2c_register_board_info." You are supposed to call it as part of your platform setup, so if it is not possible for you, whatever the problem is needs to be addressed. > Yeah, our EG20T kernel used to use the same "echo /sys/bus/i2c/devices/i2c-x/new_device" > way, but we found out it is not convenient for: > 1. needs extra user space script, why not make it just work in kernel after > boot? We have several i2c devices like touchscreen/radio which we wants them > just work without depending user space action. It should indeed be handled all in kernel space, using i2c_register_board_info(). > 2. The i2c bus number is not fixed, which make the user space script even > harder, as that number depends whether we compile into kernel all the i2c > controllers (eg20t and isch) and whether these driver are compiled as > modules and their loading order. You can always look-up the right I2C bus number based on its name, assuming your driver properly names them. There is some code doing that at: http://www.lm-sensors.org/browser/i2c-tools/trunk/tools/i2cbusses.c#L297 Ideally this code should move to libi2c and/or i2cdetect should offer an interface to it, so it can easily be called from custom tools and scripts. > Thus we come out with this fixed bus number registering. > > As I said before, either your module parameter "base_bus_num" solution > or my fixed bus number offset are ok to me. Will you cook a patch? I think the real problem you have here is that your platform setup code doesn't (or is not able to) allocate the bus IDs globally. It really should. If you have an embedded system using both i2c-eg20t and i2c-isch, the platform setup code should decide upfront who gets what I2C bus IDs, otherwise it's impossible to declare slave devices on these I2C buses. And these bus drivers should have a way to look-up that decision and ask for the proper bus numbers. At the moment it seems that i2c-ge20t assumes it always gets i2c-0 (and sometimes i2c-1 too) for itself. This is wrong. The actual bus numbers should come from a device tree of some sort. If you look at other i2c bus drivers for other embedded platforms (for example i2c-pxa), you'll see they do exactly that, i.e. the I2C bus number is part of device platform data (often the platform device ID but it could be anything else), it's not hard-coded in the driver. It may be a little more difficult to get right for a PCI driver like i2c-eg20t, but you'll have to find a way. Relying on boot parameters to get things to work is just too fragile. -- Jean Delvare ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-29 18:40 ` Jean Delvare @ 2012-08-30 7:49 ` Alexander Stein 2012-08-30 9:19 ` Feng Tang 2012-09-08 13:18 ` Jean Delvare 2012-08-30 9:10 ` Feng Tang 1 sibling, 2 replies; 13+ messages in thread From: Alexander Stein @ 2012-08-30 7:49 UTC (permalink / raw) To: Jean Delvare Cc: Feng Tang, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA Hello Jean, On Wednesday 29 August 2012 20:40:31, Jean Delvare wrote: > Looks like what media and sound drivers are/were doing to assign fixed > numbers to their devices. But my understanding is that this is a legacy > thing and nobody should need to use that any longer. Adding this to all > or even some i2c bus drivers looks like the wrong example to follow. If > your system has more than one device supported by the driver, it > doesn't even reliably guarantee fixed I2C bus numbers (especially if > some can be hot-plugged.) I'm aware that this is more than less a workaround to the current problem which arose from the change to i2c_add_numbered_adapter. The sitation before that where i2c-eg20t just used i2c_add_adapter wasn't perfect either. > > > > The reason we need a fixed number is it is easier for platform code > > > > which needs to register dozens of i2c devices to different controllers > > > > with i2c_register_board_info, and they need provide a bus number for > > > > each i2c device, this _binding_ info is not detectable but have to > > > > be fixed. > > Whenever you call i2c_register_board_info(), every I2C bus number > referenced in the I2C device list passed as a parameter is reserved for > static I2C bus numbers, dynamic I2C bus numbers will never overlap. > > So in the quoted example, if i2c-isch is able to dynamically pick I2C > bus number 0 while i2c-eg20t want it statically, it means that either > no device was declared on bus 0 with i2c_register_board_info(), or > i2c_register_board_info() was called too late in the game. In my case i2c_register_board_info is never called due to a non-existant platform setup (see below). > Note that there was an assumption at the time the code was written, > that there was no need or reason to reserve a static I2C bus number if > no slave device was declared on said I2C bus. I never much liked it but > it never caused problems so far. This means that either: > * you call i2c_register_board_info() to register your slave I2C devices > and all the affected I2C bus drivers call i2c_add_numbered_adapter(); > or > * you don't call i2c_register_board_info() and all I2C bus drivers call > i2c_add_adapter(). > You can't mix, i.e. if you don't register any slave device on a bus but > the bus driver still calls i2c_add_numbered_adapter(), it may fail. > > If this is a problem now on some systems, it should be easy enough to > work around by adding a specific function to reserve an I2C bus number > for static allocation, even without declaring any slave device on it. > This function would be called at the same time > i2c_register_board_info() typically is. IMO the i2c_register_board_info only works in quite static setups. Especially with I2C-Busses attached to hotplugable PCI devices this way doesn't work reliable any more. The device come and go dynamically so you can't assume fixed mapping. > > > Yes, I'm aware of that. With "Why use a fixed one?" I meant why hard-code it into the driver. I should be changeable. > > > Because this is/was not possible in general to use i2c_register_board_info, so we used an echo to /sys/bus/i2c/devices/i2c-0/new_device or /sys/bus/i2c/devices/i2c-1/new_device. > > Please elaborate on "this is/was not possible in general to use > i2c_register_board_info." You are supposed to call it as part of your > platform setup, so if it is not possible for you, whatever the problem > is needs to be addressed. The problem here is that AFAIK on x86 there is no platform setup to e.g. register platform devices. So you depend on the order the drivers and devices are bound. In 3.0.x we get a static numbering due to this order. As there is no platform setup we register our I2C devices with /sys/bus/i2c/devices/i2c-[01]/new_device as some of our sensors can't be auto-detected. > > Yeah, our EG20T kernel used to use the same "echo /sys/bus/i2c/devices/i2c-x/new_device" > > way, but we found out it is not convenient for: > > 1. needs extra user space script, why not make it just work in kernel after > > boot? We have several i2c devices like touchscreen/radio which we wants them > > just work without depending user space action. > > It should indeed be handled all in kernel space, using > i2c_register_board_info(). I know this mechanism for ARM, but how can this be done on x86 architecture? But even with this, how does this address the random occurance of hotplugable bus masters? > > 2. The i2c bus number is not fixed, which make the user space script even > > harder, as that number depends whether we compile into kernel all the i2c > > controllers (eg20t and isch) and whether these driver are compiled as > > modules and their loading order. > > You can always look-up the right I2C bus number based on its name, > assuming your driver properly names them. There is some code doing that > at: > http://www.lm-sensors.org/browser/i2c-tools/trunk/tools/i2cbusses.c#L297 > > Ideally this code should move to libi2c and/or i2cdetect should offer > an interface to it, so it can easily be called from custom tools and > scripts. You either need this get the current (maybe non-fixed) bus number and attach the devices by new_device or there is some in-kernel or udev related mechanism which returns the current bus number from some unique deivce path or description. Best regards, Alexander ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-30 7:49 ` Alexander Stein @ 2012-08-30 9:19 ` Feng Tang 2012-08-30 11:08 ` Alexander Stein 2012-09-08 13:18 ` Jean Delvare 1 sibling, 1 reply; 13+ messages in thread From: Feng Tang @ 2012-08-30 9:19 UTC (permalink / raw) To: Alexander Stein Cc: Jean Delvare, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA Hi Alexander, On Thu, 30 Aug 2012 09:49:52 +0200 Alexander Stein <alexander.stein@systec-electronic.com> wrote: > > > > Whenever you call i2c_register_board_info(), every I2C bus number > > referenced in the I2C device list passed as a parameter is reserved for > > static I2C bus numbers, dynamic I2C bus numbers will never overlap. > > > > So in the quoted example, if i2c-isch is able to dynamically pick I2C > > bus number 0 while i2c-eg20t want it statically, it means that either > > no device was declared on bus 0 with i2c_register_board_info(), or > > i2c_register_board_info() was called too late in the game. > > In my case i2c_register_board_info is never called due to a non-existant platform setup (see below). I think we can have that, and we did have that for a non-product platform, it's easy to add a platform driver which can use dmidecode info to identify itself. > > > Note that there was an assumption at the time the code was written, > > that there was no need or reason to reserve a static I2C bus number if > > no slave device was declared on said I2C bus. I never much liked it but > > it never caused problems so far. This means that either: > > * you call i2c_register_board_info() to register your slave I2C devices > > and all the affected I2C bus drivers call i2c_add_numbered_adapter(); > > or > > * you don't call i2c_register_board_info() and all I2C bus drivers call > > i2c_add_adapter(). > > You can't mix, i.e. if you don't register any slave device on a bus but > > the bus driver still calls i2c_add_numbered_adapter(), it may fail. > > > > If this is a problem now on some systems, it should be easy enough to > > work around by adding a specific function to reserve an I2C bus number > > for static allocation, even without declaring any slave device on it. > > This function would be called at the same time > > i2c_register_board_info() typically is. > > IMO the i2c_register_board_info only works in quite static setups. Especially with I2C-Busses attached to hotplugable PCI devices this way doesn't work reliable any more. > The device come and go dynamically so you can't assume fixed mapping. Can you specify the hotplugable? 1. A hotplugable i2c bus controller (say i2c_eg20t) with all fixed i2c devices connecting to it 2. i2c bus controller is fixed, while the i2c devices will be dynamically connected to it. 3. Both the bus controller and devices are dynamically hotplugged Thanks, Feng ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-30 9:19 ` Feng Tang @ 2012-08-30 11:08 ` Alexander Stein 2012-08-31 2:16 ` Feng Tang 0 siblings, 1 reply; 13+ messages in thread From: Alexander Stein @ 2012-08-30 11:08 UTC (permalink / raw) To: Feng Tang Cc: Jean Delvare, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA On Thursday 30 August 2012 17:19:15, Feng Tang wrote: > > IMO the i2c_register_board_info only works in quite static setups. Especially with I2C-Busses attached to hotplugable PCI devices this way doesn't work reliable any more. > > The device come and go dynamically so you can't assume fixed mapping. > > Can you specify the hotplugable? > 1. A hotplugable i2c bus controller (say i2c_eg20t) with all fixed i2c > devices connecting to it > 2. i2c bus controller is fixed, while the i2c devices will be dynamically > connected to it. > 3. Both the bus controller and devices are dynamically hotplugged I had scenario 1 in mind, but with more than 1 bus controller (say 2x i2c_eg20t). How can you set a fixed numbering if there are more controllers, each with maybe more than 1 bus? Anyway, how can you provide a static bus numbering if there are more than one driver or more than one device per driver if the devices are hotplugable? Best regards, Alexander ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-30 11:08 ` Alexander Stein @ 2012-08-31 2:16 ` Feng Tang 0 siblings, 0 replies; 13+ messages in thread From: Feng Tang @ 2012-08-31 2:16 UTC (permalink / raw) To: Alexander Stein Cc: Jean Delvare, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA Hi Alexander, On Thu, 30 Aug 2012 13:08:09 +0200 Alexander Stein <alexander.stein@systec-electronic.com> wrote: > On Thursday 30 August 2012 17:19:15, Feng Tang wrote: > > > IMO the i2c_register_board_info only works in quite static setups. Especially with I2C-Busses attached to hotplugable PCI devices this way doesn't work reliable any more. > > > The device come and go dynamically so you can't assume fixed mapping. > > > > Can you specify the hotplugable? > > 1. A hotplugable i2c bus controller (say i2c_eg20t) with all fixed i2c > > devices connecting to it > > 2. i2c bus controller is fixed, while the i2c devices will be dynamically > > connected to it. > > 3. Both the bus controller and devices are dynamically hotplugged > > I had scenario 1 in mind, but with more than 1 bus controller (say 2x i2c_eg20t). How can you set a fixed numbering if there are more controllers, each with maybe more than 1 bus? > Anyway, how can you provide a static bus numbering if there are more than one driver or more than one device per driver if the devices are hotplugable? There is no problem for one pci driver to support multiple devices with same HW, if you check i2c-eg20t.c you can check the pch_pcidev_id[], there is a ML7213 platform which has 2 i2c controllers already. With current in tree driver, they will get fixed bus number 0 and 1. For the scenario 1, if you have really have another hotplugable pci eg20t controller, it surely will have its unique PCI id, and then we can use the "driver_data" of struct pci_device_id to point to a platform info structure like struct i2c_eg20t_platform_info { u16 bus_base_num; u16 total_hw_num; } And for EG20T compatible platform, I don't think it will have too many fancy hotplugable different type of i2c controller other than i2c_eg20t and i2c_sch. Thanks, Feng ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-30 7:49 ` Alexander Stein 2012-08-30 9:19 ` Feng Tang @ 2012-09-08 13:18 ` Jean Delvare 1 sibling, 0 replies; 13+ messages in thread From: Jean Delvare @ 2012-09-08 13:18 UTC (permalink / raw) To: Alexander Stein Cc: Feng Tang, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA Hi Alexander, Sorry for the late reply again. On Thu, 30 Aug 2012 09:49:52 +0200, Alexander Stein wrote: > On Wednesday 29 August 2012 20:40:31, Jean Delvare wrote: > > Note that there was an assumption at the time the code was written, > > that there was no need or reason to reserve a static I2C bus number if > > no slave device was declared on said I2C bus. I never much liked it but > > it never caused problems so far. This means that either: > > * you call i2c_register_board_info() to register your slave I2C devices > > and all the affected I2C bus drivers call i2c_add_numbered_adapter(); > > or > > * you don't call i2c_register_board_info() and all I2C bus drivers call > > i2c_add_adapter(). > > You can't mix, i.e. if you don't register any slave device on a bus but > > the bus driver still calls i2c_add_numbered_adapter(), it may fail. > > > > If this is a problem now on some systems, it should be easy enough to > > work around by adding a specific function to reserve an I2C bus number > > for static allocation, even without declaring any slave device on it. > > This function would be called at the same time > > i2c_register_board_info() typically is. > > IMO the i2c_register_board_info only works in quite static setups. Especially with I2C-Busses attached to hotplugable PCI devices this way doesn't work reliable any more. > The device come and go dynamically so you can't assume fixed mapping. This is correct, i2c_register_board_info() was designed with static setups in mind and isn't suitable for dynamic setups. But there are other ways for dynamic setup, described in Documentation/i2c/instantiating-devices. There is another approach which isn't fully documented yet, partly because I am not so proud of it. Look at drivers/i2c/busses/i2c-i801.c, it has a lot of platform-specific code instantiating slave i2c devices. As the code is in the bus driver, we don't have to know the bus number in advance, which is quite convenient for PC-style machines. I'm not sure if we want to generalize this, as this is a little hard to maintain, but short of a better idea, feel free to do the same for the time being. > > (...) > > Please elaborate on "this is/was not possible in general to use > > i2c_register_board_info." You are supposed to call it as part of your > > platform setup, so if it is not possible for you, whatever the problem > > is needs to be addressed. > > The problem here is that AFAIK on x86 there is no platform setup to e.g. register platform devices. So you depend on the order the drivers and devices are bound. There originally wasn't, as x86 wasn't considered an embedded platform. But I can see arch/x86/platform is growing now, so apparently it is well supported now. > In 3.0.x we get a static numbering due to this order. As there is no platform setup we register our I2C devices with /sys/bus/i2c/devices/i2c-[01]/new_device as some of our sensors can't be auto-detected. Please try dropping your platform code somewhere in arch/x86/platform and see where you can go with that approach. You mean have to tweak the i2c bus drivers themselves to be able to provide the bus numbers. > > (...) > > It should indeed be handled all in kernel space, using > > i2c_register_board_info(). > > I know this mechanism for ARM, but how can this be done on x86 architecture? Just the same, I'd say, but I admit I never tried, as I never had an embedded x86 system to play with. > But even with this, how does this address the random occurance of hotplugable bus masters? For hot-plugable bus masters, obviously it doesn't work. You can go the i2c-i801 way for now. But if you think i2c_register_board_info() should be extended to support less static setups, I have no problem with this. For example, i2c_register_board_info_dynamic() could be introduced, based on i2c_register_board_info() but you'd pass a "bus match" callback function instead of a bus number. The callback could check for example the adapter name to tell if this is the bus on which the slave devices should be instantiated, or it could check for a specific platform and be inactive for others. Whatever you need, just ask for it, or even better, code it yourself, test it, and if it works and seems reasonable as a general solution, send it over. This is an evolving area, this is no surprise that changes are needed. > > (...) > > You can always look-up the right I2C bus number based on its name, > > assuming your driver properly names them. There is some code doing that > > at: > > http://www.lm-sensors.org/browser/i2c-tools/trunk/tools/i2cbusses.c#L297 > > > > Ideally this code should move to libi2c and/or i2cdetect should offer > > an interface to it, so it can easily be called from custom tools and > > scripts. > > You either need this get the current (maybe non-fixed) bus number and attach the devices by new_device or there is some in-kernel or udev related mechanism which returns the current bus number from some unique deivce path or description. You can do a lot of things with carefully crafted udev rules, yes. -- Jean Delvare ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: i2c-eg20t: regression since i2c_add_numbered_adapter change 2012-08-29 18:40 ` Jean Delvare 2012-08-30 7:49 ` Alexander Stein @ 2012-08-30 9:10 ` Feng Tang 1 sibling, 0 replies; 13+ messages in thread From: Feng Tang @ 2012-08-30 9:10 UTC (permalink / raw) To: Jean Delvare Cc: Alexander Stein, linux-i2c, linux-kernel, Ben Dooks (embedded platforms), Tomoya MORINAGA, artem.bityutskiy Hi Jean, Thanks for the explanation and bring up some history background of those i2c core APIs! It was several month ago that I maintained a Tizen kernel for Intel EG20T based platforms when I cooked the patch, so loop in current maintainer Artem. On Wed, 29 Aug 2012 20:40:31 +0200 Jean Delvare <khali@linux-fr.org> wrote: > Hi guys, > > Sorry for joining the discussion a little late, I was on vacation. > > On Thu, 23 Aug 2012 16:28:52 +0800, Feng Tang wrote: > > On Wed, 22 Aug 2012 11:17:51 +0200 > > Alexander Stein <alexander.stein@systec-electronic.com> wrote: > > > Am Mittwoch, 22. August 2012, 16:04:39 schrieb Feng Tang: > > > > > Why use a fixed one? Give the driver (and maybe every i2c bus driver) a parameter which sets the base bus number it should use. > > > > > E.g. i2c-eg20t.base-bus-num=2 so it will register the bus numbers starting from 2. If this parameter is unset. It would use the first free one, thus simply using i2c_add_adapter. > > Looks like what media and sound drivers are/were doing to assign fixed > numbers to their devices. But my understanding is that this is a legacy > thing and nobody should need to use that any longer. Adding this to all > or even some i2c bus drivers looks like the wrong example to follow. If > your system has more than one device supported by the driver, it > doesn't even reliably guarantee fixed I2C bus numbers (especially if > some can be hot-plugged.) > > > > > The reason we need a fixed number is it is easier for platform code > > > > which needs to register dozens of i2c devices to different controllers > > > > with i2c_register_board_info, and they need provide a bus number for > > > > each i2c device, this _binding_ info is not detectable but have to > > > > be fixed. > > Whenever you call i2c_register_board_info(), every I2C bus number > referenced in the I2C device list passed as a parameter is reserved for > static I2C bus numbers, dynamic I2C bus numbers will never overlap. > > So in the quoted example, if i2c-isch is able to dynamically pick I2C > bus number 0 while i2c-eg20t want it statically, it means that either > no device was declared on bus 0 with i2c_register_board_info(), or > i2c_register_board_info() was called too late in the game. Exactly, in our own code base, I did write a platform driver in drivers/platform/x86, which calls the i2c_register_board_info in a subsys_initcall, which protect us from seeing this issue reported by Alexander and work just fine. But since that platform is not a product one, we didn't push it into mainline. > > Note that there was an assumption at the time the code was written, > that there was no need or reason to reserve a static I2C bus number if > no slave device was declared on said I2C bus. I never much liked it but > it never caused problems so far. This means that either: > * you call i2c_register_board_info() to register your slave I2C devices > and all the affected I2C bus drivers call i2c_add_numbered_adapter(); > or > * you don't call i2c_register_board_info() and all I2C bus drivers call > i2c_add_adapter(). > You can't mix, i.e. if you don't register any slave device on a bus but > the bus driver still calls i2c_add_numbered_adapter(), it may fail. > > If this is a problem now on some systems, it should be easy enough to > work around by adding a specific function to reserve an I2C bus number > for static allocation, even without declaring any slave device on it. > This function would be called at the same time > i2c_register_board_info() typically is. > > Yeah, our EG20T kernel used to use the same "echo /sys/bus/i2c/devices/i2c-x/new_device" > > way, but we found out it is not convenient for: > > 1. needs extra user space script, why not make it just work in kernel after > > boot? We have several i2c devices like touchscreen/radio which we wants them > > just work without depending user space action. > > It should indeed be handled all in kernel space, using > i2c_register_board_info(). Can't agree more, this is what we are doing now. > > > 2. The i2c bus number is not fixed, which make the user space script even > > harder, as that number depends whether we compile into kernel all the i2c > > controllers (eg20t and isch) and whether these driver are compiled as > > modules and their loading order. > > You can always look-up the right I2C bus number based on its name, > assuming your driver properly names them. There is some code doing that > at: > http://www.lm-sensors.org/browser/i2c-tools/trunk/tools/i2cbusses.c#L297 > > Ideally this code should move to libi2c and/or i2cdetect should offer > an interface to it, so it can easily be called from custom tools and > scripts. > > > Thus we come out with this fixed bus number registering. > > > > As I said before, either your module parameter "base_bus_num" solution > > or my fixed bus number offset are ok to me. Will you cook a patch? > > I think the real problem you have here is that your platform setup code > doesn't (or is not able to) allocate the bus IDs globally. It really > should. If you have an embedded system using both i2c-eg20t and > i2c-isch, the platform setup code should decide upfront who gets what > I2C bus IDs, otherwise it's impossible to declare slave devices on > these I2C buses. And these bus drivers should have a way to look-up > that decision and ask for the proper bus numbers. > > At the moment it seems that i2c-ge20t assumes it always gets i2c-0 (and > sometimes i2c-1 too) for itself. This is wrong. The actual bus numbers > should come from a device tree of some sort. If you look at other i2c > bus drivers for other embedded platforms (for example i2c-pxa), you'll > see they do exactly that, i.e. the I2C bus number is part of device > platform data (often the platform device ID but it could be anything > else), it's not hard-coded in the driver. It may be a little more > difficult to get right for a PCI driver like i2c-eg20t, but you'll have > to find a way. Relying on boot parameters to get things to work is just > too fragile. Yes, ideally we need some method like the device tree or the ACPI 5.0 which has the global info of how many i2c buses and devices there are in the platform and how they get connected. But for current existing EG20T platforms, I prefer to use your statically i2c_register_board_info + i2c_add_numbered_adapter solution. Thanks, Feng ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-09-08 13:19 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-22 6:30 i2c-eg20t: regression since i2c_add_numbered_adapter change Alexander Stein 2012-08-22 7:29 ` Feng Tang 2012-08-22 7:57 ` Alexander Stein 2012-08-22 8:04 ` Feng Tang 2012-08-22 9:17 ` Alexander Stein 2012-08-23 8:28 ` Feng Tang 2012-08-29 18:40 ` Jean Delvare 2012-08-30 7:49 ` Alexander Stein 2012-08-30 9:19 ` Feng Tang 2012-08-30 11:08 ` Alexander Stein 2012-08-31 2:16 ` Feng Tang 2012-09-08 13:18 ` Jean Delvare 2012-08-30 9:10 ` Feng Tang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).