| Message ID | 1471943158-21377-1-git-send-email-yadi.hu@windriver.com |
|---|---|
| State | Under Review |
| Delegated to: | Wolfram Sang |
| Headers | show |
Hi Yadi, On Tue, 23 Aug 2016 17:05:58 +0800, Yadi Hu wrote: > From: Hu Yadi <Yadi.hu@windriver.com> > > The eg20t driver uses i2c_add_numbered_adapter() to register adapter: > > pch_adap->nr = i; > ret = i2c_add_numbered_adapter(pch_adap); > > Variable i is assigned to 0, it means that i2c_eg20t is the first adapter > by default. if another adapter registers before eg20t, above code return > error for index conflict: > > i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED > i2c_eg20t: probe of 0000:05:0c.2 failed with error -16 > > So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter() > interface.since it dynamically allocates the index number. This does the exact opposite of: commit 07e8a51ff68353e01d795cceafbac9f54c49132b Author: Feng Tang <feng.tang@intel.com> Date: Thu Jan 12 15:38:02 2012 +0800 i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number So my understanding is that you are not supposed to register another i2c bus before the eg20t buses. What is the conflicting driver? Is it creating the buses with static number or not? Looking at commit 03bde7c31a360f814ca42101d60563b1b803dca1 Author: Wolfram Sang <wsa+renesas@sang-engineering.com> Date: Thu Mar 12 17:17:59 2015 +0100 i2c: busses with dynamic ids should start after fixed ids for DT it could be that you need to set some OF attribute to reserve i2c bus numbers <= 1 for static usage. Assuming you use OF. Or is it automatic, Wolfram? If not, it may make sense to add a helper function exposing __i2c_first_dynamic_bus_num to drivers (something like i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly makes sense if static i2c device definitions exist. If not, i2c_add_adapter() is just as good. So something like: if (i2c_is_dynamic_bus_num(i)) ret = i2c_add_adapter(pch_adap); else { pch_adap->nr = i; ret = i2c_add_numbered_adapter(pch_adap); } may make sense. Unless someone has a better idea. > > Signed-off-by: Hu Yadi <Yadi.hu@windriver.com> > --- > drivers/i2c/busses/i2c-eg20t.c | 3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c > index 7a51ddc..2f4c2af 100644 > --- a/drivers/i2c/busses/i2c-eg20t.c > +++ b/drivers/i2c/busses/i2c-eg20t.c > @@ -913,8 +913,7 @@ static int __devinit pch_i2c_probe(struct pci_dev *pdev, > > pch_i2c_init(&adap_info->pch_data[i]); > > - pch_adap->nr = i; > - ret = i2c_add_numbered_adapter(pch_adap); > + ret = i2c_add_adapter(pch_adap); Coding style is wrong here. Please use tabs, as ./scripts/checkpatch.pl tells you. Always check your patches with this script before you post them, thanks. > if (ret) { > pch_pci_err(pdev, "i2c_add_adapter[ch:%d] FAILED\n", i); > goto err_add_adapter;
On 2016年08月26日 23:30, Jean Delvare wrote: > Hi Yadi, > > On Tue, 23 Aug 2016 17:05:58 +0800, Yadi Hu wrote: >> From: Hu Yadi <Yadi.hu@windriver.com> >> >> The eg20t driver uses i2c_add_numbered_adapter() to register adapter: >> >> pch_adap->nr = i; >> ret = i2c_add_numbered_adapter(pch_adap); >> >> Variable i is assigned to 0, it means that i2c_eg20t is the first adapter >> by default. if another adapter registers before eg20t, above code return >> error for index conflict: >> >> i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED >> i2c_eg20t: probe of 0000:05:0c.2 failed with error -16 >> >> So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter() >> interface.since it dynamically allocates the index number. > This does the exact opposite of: > > commit 07e8a51ff68353e01d795cceafbac9f54c49132b > Author: Feng Tang <feng.tang@intel.com> > Date: Thu Jan 12 15:38:02 2012 +0800 > > i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number > > So my understanding is that you are not supposed to register another > i2c bus before the eg20t buses. What is the conflicting driver? Is it > creating the buses with static number or not? I am using one Kontron M2M, on which both i2c-eg20t and i2c-kempld are used simultaneously. since kempld always is firstly initialized and configured to register dynamically, it always occupied the first bus-id. i.e. 0. consequently, eg20t will complain to fail to register for no space for "0" i2c-bus. > > Looking at > > commit 03bde7c31a360f814ca42101d60563b1b803dca1 > Author: Wolfram Sang <wsa+renesas@sang-engineering.com> > Date: Thu Mar 12 17:17:59 2015 +0100 > > i2c: busses with dynamic ids should start after fixed ids for DT > > it could be that you need to set some OF attribute to reserve i2c bus > numbers <= 1 for static usage. Assuming you use OF. Or is it automatic, > Wolfram? > > If not, it may make sense to add a helper function exposing > __i2c_first_dynamic_bus_num to drivers (something like > i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly > makes sense if static i2c device definitions exist. If not, > i2c_add_adapter() is just as good. So something like: > > if (i2c_is_dynamic_bus_num(i)) > ret = i2c_add_adapter(pch_adap); > else { > pch_adap->nr = i; > ret = i2c_add_numbered_adapter(pch_adap); > } > > may make sense. Unless someone has a better idea. I totally agree with you and send off V2 patch soon per your suggestion. Yadi > >> Signed-off-by: Hu Yadi <Yadi.hu@windriver.com> >> --- >> drivers/i2c/busses/i2c-eg20t.c | 3 +-- >> 1 files changed, 1 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c >> index 7a51ddc..2f4c2af 100644 >> --- a/drivers/i2c/busses/i2c-eg20t.c >> +++ b/drivers/i2c/busses/i2c-eg20t.c >> @@ -913,8 +913,7 @@ static int __devinit pch_i2c_probe(struct pci_dev *pdev, >> >> pch_i2c_init(&adap_info->pch_data[i]); >> >> - pch_adap->nr = i; >> - ret = i2c_add_numbered_adapter(pch_adap); >> + ret = i2c_add_adapter(pch_adap); > Coding style is wrong here. Please use tabs, as ./scripts/checkpatch.pl > tells you. Always check your patches with this script before you post > them, thanks. > >> if (ret) { >> pch_pci_err(pdev, "i2c_add_adapter[ch:%d] FAILED\n", i); >> goto err_add_adapter; > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > The eg20t driver uses i2c_add_numbered_adapter() to register adapter: > > > > pch_adap->nr = i; > > ret = i2c_add_numbered_adapter(pch_adap); > > > > Variable i is assigned to 0, it means that i2c_eg20t is the first adapter > > by default. if another adapter registers before eg20t, above code return > > error for index conflict: > > > > i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED > > i2c_eg20t: probe of 0000:05:0c.2 failed with error -16 > > > > So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter() > > interface.since it dynamically allocates the index number. > > This does the exact opposite of: > > commit 07e8a51ff68353e01d795cceafbac9f54c49132b > Author: Feng Tang <feng.tang@intel.com> > Date: Thu Jan 12 15:38:02 2012 +0800 > > i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number We have the same problem with i2c-pasemi: http://www.spinics.net/lists/linux-i2c/msg25761.html PCI + i2c_add_numbered_adapter + another device adding i2c busses And we cannot simply change back to i2c_add_adapter() because it will regress on those systems which originally wanted to have i2c_add_numbered_adapter(). > Looking at > > commit 03bde7c31a360f814ca42101d60563b1b803dca1 > Author: Wolfram Sang <wsa+renesas@sang-engineering.com> > Date: Thu Mar 12 17:17:59 2015 +0100 > > i2c: busses with dynamic ids should start after fixed ids for DT > > it could be that you need to set some OF attribute to reserve i2c bus > numbers <= 1 for static usage. Assuming you use OF. Or is it automatic, > Wolfram? No, you need to define an alias in the devicetree. I don't think this platform is a DT user, though? > If not, it may make sense to add a helper function exposing > __i2c_first_dynamic_bus_num to drivers (something like > i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly > makes sense if static i2c device definitions exist. If not, > i2c_add_adapter() is just as good. So something like: > > if (i2c_is_dynamic_bus_num(i)) > ret = i2c_add_adapter(pch_adap); > else { > pch_adap->nr = i; > ret = i2c_add_numbered_adapter(pch_adap); > } > > may make sense. Unless someone has a better idea. Interesting idea. Need to think about it some more. I didn't have a proper solution so far... Thanks, Wolfram
> If not, it may make sense to add a helper function exposing > __i2c_first_dynamic_bus_num to drivers (something like > i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly > makes sense if static i2c device definitions exist. If not, > i2c_add_adapter() is just as good. So something like: > > if (i2c_is_dynamic_bus_num(i)) > ret = i2c_add_adapter(pch_adap); > else { > pch_adap->nr = i; > ret = i2c_add_numbered_adapter(pch_adap); > } > > may make sense. Unless someone has a better idea. PASEMI does: smbus->adapter.nr = PCI_FUNC(dev->devfn); I am unsure if there is any guarantee in what order PCI_FUNCs are probed, yet I have the feeling we could try a little harder to get the numbered adapter. What about this (untested, just to get the idea)? static inline int i2c_add_adapter_try_numbered(struct i2c_adapter *new_adap) { int ret; struct i2c_adapter *old_adap = i2c_get_adapter(new_adap->nr); if (old_adap && new_adap->nr >= __i2c_first_dynamic_bus_num) { i2c_put_adapter(old_adap); dev_dbg(&new_adap->dev, "Static bus number occupied, trying dynamic number\n"); ret = i2c_add_adapter(new_adap); } else { ret = i2c_add_numbered_adapter(new_adap); } return ret; } I used 'static inline' because I think the drivers needing this should carry the extra weight. But no major objection to put sth like this also into the core. The documentation for this function should carry a big note that this is only a workaround and it should not be used directly. Thoughts? Wolfram
On 2016年09月18日 05:49, Wolfram Sang wrote: >>> The eg20t driver uses i2c_add_numbered_adapter() to register adapter: >>> >>> pch_adap->nr = i; >>> ret = i2c_add_numbered_adapter(pch_adap); >>> >>> Variable i is assigned to 0, it means that i2c_eg20t is the first adapter >>> by default. if another adapter registers before eg20t, above code return >>> error for index conflict: >>> >>> i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED >>> i2c_eg20t: probe of 0000:05:0c.2 failed with error -16 >>> >>> So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter() >>> interface.since it dynamically allocates the index number. >> This does the exact opposite of: >> >> commit 07e8a51ff68353e01d795cceafbac9f54c49132b >> Author: Feng Tang <feng.tang@intel.com> >> Date: Thu Jan 12 15:38:02 2012 +0800 >> >> i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number > We have the same problem with i2c-pasemi: > http://www.spinics.net/lists/linux-i2c/msg25761.html > > PCI + i2c_add_numbered_adapter + another device adding i2c busses > > And we cannot simply change back to i2c_add_adapter() because it will > regress on those systems which originally wanted to have > i2c_add_numbered_adapter(). I think the is one totally different issue with i2c-pasemi, in which PCI device prior to probed occupied bus number supposed to be used by i2c-pasemi. For my case, i2c-kempld register dynamically itself by default, consequently it would occupied i2c bus 0 and cause to fails for adding new i2c bus with i2c_add_number_adpater(0). although i2c-kempld provide variable 'i2c_bus' to control bus assignment, it isso inconvenient we have to manually modify it to avoid possible conflict. so the reasonable solution looks like: i2c-kempld.c i2c->adap.nr = i2c_bus; if (i2c_bus >= -1) /* using pci id to register if default=-1 for dynamic assignment*/ i2c->adap.n = pdev->id; ret = i2c_add_numbered_adapter(&i2c->adap); how do you like it? Yadi > >> Looking at >> >> commit 03bde7c31a360f814ca42101d60563b1b803dca1 >> Author: Wolfram Sang <wsa+renesas@sang-engineering.com> >> Date: Thu Mar 12 17:17:59 2015 +0100 >> >> i2c: busses with dynamic ids should start after fixed ids for DT >> >> it could be that you need to set some OF attribute to reserve i2c bus >> numbers <= 1 for static usage. Assuming you use OF. Or is it automatic, >> Wolfram? > No, you need to define an alias in the devicetree. I don't think this > platform is a DT user, though? > >> If not, it may make sense to add a helper function exposing >> __i2c_first_dynamic_bus_num to drivers (something like >> i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly >> makes sense if static i2c device definitions exist. If not, >> i2c_add_adapter() is just as good. So something like: >> >> if (i2c_is_dynamic_bus_num(i)) >> ret = i2c_add_adapter(pch_adap); >> else { >> pch_adap->nr = i; >> ret = i2c_add_numbered_adapter(pch_adap); >> } >> >> may make sense. Unless someone has a better idea. > Interesting idea. Need to think about it some more. I didn't have a > proper solution so far... > > Thanks, > > Wolfram > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> For my case, i2c-kempld register dynamically itself by default, > consequently it would occupied i2c bus 0 and cause to fails for adding new > i2c bus with i2c_add_number_adpater(0). This is exactly the PASEMI case as well. A video card got probed before the pasemi driver and took all bus numbers it wants. > i2c-kempld.c ... > how do you like it? Not much. If we change kempld, then another driver might come along next time and we have the same issue again. I want a generic solution.
Hi Wolfram, On Sun, 18 Sep 2016 21:22:50 +0200, Wolfram Sang wrote: > > > If not, it may make sense to add a helper function exposing > > __i2c_first_dynamic_bus_num to drivers (something like > > i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly > > makes sense if static i2c device definitions exist. If not, > > i2c_add_adapter() is just as good. So something like: > > > > if (i2c_is_dynamic_bus_num(i)) > > ret = i2c_add_adapter(pch_adap); > > else { > > pch_adap->nr = i; > > ret = i2c_add_numbered_adapter(pch_adap); > > } > > > > may make sense. Unless someone has a better idea. > > PASEMI does: > > smbus->adapter.nr = PCI_FUNC(dev->devfn); > > I am unsure if there is any guarantee in what order PCI_FUNCs are > probed, yet I have the feeling we could try a little harder to get the > numbered adapter. What about this (untested, just to get the idea)? > > static inline int i2c_add_adapter_try_numbered(struct i2c_adapter *new_adap) > { > int ret; > struct i2c_adapter *old_adap = i2c_get_adapter(new_adap->nr); > > if (old_adap && new_adap->nr >= __i2c_first_dynamic_bus_num) { Note that __i2c_first_dynamic_bus_num is not currently available to device driver, so your function can't actually be inlined. This is the reason why I proposed to introduce i2c_is_dynamic_bus_num(). If you prefer to expose __i2c_first_dynamic_bus_num directly instead, that's fine with me, but then you probably want to rename it. > i2c_put_adapter(old_adap); > dev_dbg(&new_adap->dev, "Static bus number occupied, trying dynamic number\n"); > ret = i2c_add_adapter(new_adap); > } else { > ret = i2c_add_numbered_adapter(new_adap); You may be leaking a reference to old_adap here (if old_adap is set but new_adap->nr < __i2c_first_dynamic_bus_num.) > } > > return ret; > } I'm a bit confused by the logic, it seems unnecessarily complex to me (but please keep in mind I am working in a noisy environment these days so maybe it's just me.) If old_adap is set, i2c_add_numbered_adapter() has no chance of working. So why are you additionally comparing with __i2c_first_dynamic_bus_num? For me you should check either, not both. My own proposal was checking __i2c_first_dynamic_bus_num. To be honest I can't see the added value of relying on i2c_get_adapter() instead. Would the result not be exactly the same? Plus it seems racy, just because i2c_get_adapter() returns NULL at one point in time doesn't mean the bus numbers will not have been assigned by the time you call i2c_add_numbered_adapter(). > I used 'static inline' because I think the drivers needing this should > carry the extra weight. But no major objection to put sth like this also > into the core. The documentation for this function should carry a big > note that this is only a workaround and it should not be used directly. I don't care much where it goes, to be honest. If you consider it a workaround, what would be the "real fix" for you? I was wondering if selecting one of these drivers could set a Kconfig option to initialize __i2c_first_dynamic_bus_num to a non-zero value. Unfortunately there does not seem to be a way to set a numeric value to a Kconfig option using select. We would have to do it indirectly as with CONFIG_HZ: choice default I2C_RESERVED_BUS_NR_0 config I2C_RESERVED_BUS_NR_0 config I2C_RESERVED_BUS_NR_2 endchoice config I2C_RESERVED_BUS_NR int default 0 if I2C_RESERVED_BUS_NR_0 default 2 if I2C_RESERVED_BUS_NR_2 config I2C_EG20T tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C" depends on PCI && (X86_32 || MIPS || COMPILE_TEST) select I2C_RESERVED_BUS_NR_2 And in i2c-core.c: __i2c_first_dynamic_bus_num = I2C_RESERVED_BUS_NR; If that's possible at all... I'm not sure if select works on choice config options. Alternatively we could set the default directly based on driver selection: config I2C_RESERVED_BUS_NR int default 0 default 2 if I2C_EG20T This is more simple but a little harder to maintain. One possible problem is if the number of buses isn't known at build time but could change depending on the hardware, for example. Also I don't know if more than one such driver can be included in the same kernel. Or we can make it a user-visible option and leave it on whoever configures the kernel to get it right. In all cases it would move the decision to build-time instead of being set dynamically at run-time. Note that I am not claiming this is necessarily better than my initial proposal. I just wanted to mention the possibility.
Hi all, On Mon, 19 Sep 2016 11:02:47 +0200, Jean Delvare wrote: > My own proposal was checking __i2c_first_dynamic_bus_num. To be honest > I can't see the added value of relying on i2c_get_adapter() instead. > Would the result not be exactly the same? Plus it seems racy, just > because i2c_get_adapter() returns NULL at one point in time doesn't > mean the bus numbers will not have been assigned by the time you call > i2c_add_numbered_adapter(). > (...) > If you consider it a workaround, what would be the "real fix" for you? > > I was wondering if selecting one of these drivers could set a Kconfig > option to initialize __i2c_first_dynamic_bus_num to a non-zero value. > Unfortunately there does not seem to be a way to set a numeric value to > a Kconfig option using select. We would have to do it indirectly as > with CONFIG_HZ: > > choice > default I2C_RESERVED_BUS_NR_0 > config I2C_RESERVED_BUS_NR_0 > config I2C_RESERVED_BUS_NR_2 > endchoice > > config I2C_RESERVED_BUS_NR > int > default 0 if I2C_RESERVED_BUS_NR_0 > default 2 if I2C_RESERVED_BUS_NR_2 > > config I2C_EG20T > tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C" > depends on PCI && (X86_32 || MIPS || COMPILE_TEST) > select I2C_RESERVED_BUS_NR_2 > > And in i2c-core.c: > > __i2c_first_dynamic_bus_num = I2C_RESERVED_BUS_NR; > > If that's possible at all... I'm not sure if select works on choice > config options. > > Alternatively we could set the default directly based on driver > selection: > > config I2C_RESERVED_BUS_NR > int > default 0 > default 2 if I2C_EG20T > > This is more simple but a little harder to maintain. One possible > problem is if the number of buses isn't known at build time but could > change depending on the hardware, for example. Also I don't know if > more than one such driver can be included in the same kernel. > > Or we can make it a user-visible option and leave it on whoever > configures the kernel to get it right. In all cases it would move the > decision to build-time instead of being set dynamically at run-time. > > Note that I am not claiming this is necessarily better than my initial > proposal. I just wanted to mention the possibility. I slept over it and start wondering if we aren't trying to address the problem at the wrong end. The initial reason why i2c_add_numbered_adapter() was used in the i2c-eg20t driver (and, I suppose, i2c-pasemi and all other bus drivers) is because the platform initialization code/data (native or OF-based) may be declaring slave I2C devices on these buses. This bumps __i2c_first_dynamic_bus_num, and the bus driver needs to call i2c_add_numbered_adapter() to get bus numbers below that. Maybe in that case the bus driver should ALSO get the static bus number information from the platform initialization code/data. Then the bus driver would check that information, and use it to call i2c_add_numbered_adapter() as appropriate if present, or simply call i2c_add_adapter() if not (or ask i2c_add_numbered_adapter for bus number -1, which has the same result.) Looks like drivers i2c-s3c2410, i2c-designware-*, i2c-kempld and i2c-pxa are already getting this right. So maybe we don't need to introduce another mechanism, but instead use what is already in place?
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c index 7a51ddc..2f4c2af 100644 --- a/drivers/i2c/busses/i2c-eg20t.c +++ b/drivers/i2c/busses/i2c-eg20t.c @@ -913,8 +913,7 @@ static int __devinit pch_i2c_probe(struct pci_dev *pdev, pch_i2c_init(&adap_info->pch_data[i]); - pch_adap->nr = i; - ret = i2c_add_numbered_adapter(pch_adap); + ret = i2c_add_adapter(pch_adap); if (ret) { pch_pci_err(pdev, "i2c_add_adapter[ch:%d] FAILED\n", i); goto err_add_adapter;