| Message ID | 1419272149-28716-1-git-send-email-walter@vanguardiasur.com.ar |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, Dec 22, 2014 at 03:15:49PM -0300, Walter Lozano wrote: > Currently, the driver is relying on a subsys_initcall() to register > the platform driver struct. This is typically done to allow early uses > of the I2C bus (for instance, I2C regulators used from early machine code). > > While this might work on some cases, it breaks on others. For instance, > I2C devices with GPIO-wired interrupts will fail to request the > interrupt because of this initcall usage. > > This commits attempts to fix the above issue, by converting the I2C > driver into a regular kernel platform driver. This guarantees it will > probe after GPIOs drivers. > > Platforms based on devicetree won't be affected by this change. Huh, why is that? > Legacy platforms, relying on the I2C being available early, might need > to implement proper defered mechanisms to overcome potential problems. NAK. We can't say "Let's cause a regression to force people to fix things that used to work" IMO. You exactly pointed out the problem that using subsys_initcall() creates. What about fixing the drivers you use to support deferred probing when acquitin the irq? Regards, Wolfram
On Mon, Dec 22, 2014 at 4:02 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Mon, Dec 22, 2014 at 03:15:49PM -0300, Walter Lozano wrote: >> Currently, the driver is relying on a subsys_initcall() to register >> the platform driver struct. This is typically done to allow early uses >> of the I2C bus (for instance, I2C regulators used from early machine code). >> >> While this might work on some cases, it breaks on others. For instance, >> I2C devices with GPIO-wired interrupts will fail to request the >> interrupt because of this initcall usage. >> >> This commits attempts to fix the above issue, by converting the I2C >> driver into a regular kernel platform driver. This guarantees it will >> probe after GPIOs drivers. >> >> Platforms based on devicetree won't be affected by this change. > > Huh, why is that? > >> Legacy platforms, relying on the I2C being available early, might need >> to implement proper defered mechanisms to overcome potential problems. > > NAK. We can't say "Let's cause a regression to force people to fix > things that used to work" IMO. You exactly pointed out the problem that using > subsys_initcall() creates. > > What about fixing the drivers you use to support deferred probing when > acquitin the irq? Yes, probably it is the best approach to avoid possible issues with other drivers. I'll check your suggestion. Thanks for your comments. Regards, Walter
On 12/22/2014 04:02 PM, Wolfram Sang wrote: > On Mon, Dec 22, 2014 at 03:15:49PM -0300, Walter Lozano wrote: >> Currently, the driver is relying on a subsys_initcall() to register >> the platform driver struct. This is typically done to allow early uses >> of the I2C bus (for instance, I2C regulators used from early machine code). >> >> While this might work on some cases, it breaks on others. For instance, >> I2C devices with GPIO-wired interrupts will fail to request the >> interrupt because of this initcall usage. >> >> This commits attempts to fix the above issue, by converting the I2C >> driver into a regular kernel platform driver. This guarantees it will >> probe after GPIOs drivers. >> >> Platforms based on devicetree won't be affected by this change. > > Huh, why is that? > Unless I'm missing something here, our beloved DeviceTree guarantees to model the dependency between I2C slaves devices and the I2C master their connected to. So, a machine fully-based on DeviceTree would never attempt to use the I2C bus without first registering the master, right? This means there won't be any early users of the I2C platform driver in this scenario. >> Legacy platforms, relying on the I2C being available early, might need >> to implement proper defered mechanisms to overcome potential problems. > > NAK. We can't say "Let's cause a regression to force people to fix > things that used to work" IMO. You exactly pointed out the problem that using > subsys_initcall() creates. > > What about fixing the drivers you use to support deferred probing when > acquitin the irq? > Maybe we can fix the legacy ones instead. However, a quick look shows there aren't any! $ git grep i2c_designware drivers/i2c/busses/i2c-designware-pcidrv.c:MODULE_ALIAS("i2c_designware-pci"); drivers/i2c/busses/i2c-designware-platdrv.c:MODULE_ALIAS("platform:i2c_designware"); drivers/i2c/busses/i2c-designware-platdrv.c: .name = "i2c_designware", Looks like this patch is pretty harmless. Thanks!
> >> This guarantees it will probe after GPIOs drivers. BTW this is not true to the best of my knowledge. It will make that "very likely" but not "guarantee" anything. So, the race window is smaller but it is still there. You need a proper fix anyhow. > >> Platforms based on devicetree won't be affected by this change. > > > > Huh, why is that? > > > > Unless I'm missing something here, our beloved DeviceTree guarantees to > model the dependency between I2C slaves devices and the I2C master their > connected to. Frankly, you are missing quite some things here. The I2C core registers the clients when a master gets registered. No difference between platform and DT here. > So, a machine fully-based on DeviceTree would never attempt to use the I2C > bus without first registering the master, right? Neither would platform, that would be quite a bug. > This means there won't be any early users of the I2C platform driver in this > scenario. There won't be with platform as well. But I think you are missing the point. We are a *consumer* of GPIOs here. All of the above has nothing to do with GPIO controllers being already available. > >> Legacy platforms, relying on the I2C being available early, might need > >> to implement proper defered mechanisms to overcome potential problems. > > > > NAK. We can't say "Let's cause a regression to force people to fix > > things that used to work" IMO. You exactly pointed out the problem that using > > subsys_initcall() creates. > > > > What about fixing the drivers you use to support deferred probing when > > acquitin the irq? > > > > Maybe we can fix the legacy ones instead. However, a quick look shows there > aren't any! > > $ git grep i2c_designware > drivers/i2c/busses/i2c-designware-pcidrv.c:MODULE_ALIAS("i2c_designware-pci"); > drivers/i2c/busses/i2c-designware-platdrv.c:MODULE_ALIAS("platform:i2c_designware"); > drivers/i2c/busses/i2c-designware-platdrv.c: .name = "i2c_designware", > > Looks like this patch is pretty harmless. In-tree you are right. Out-of-tree, you probably aren't. I wouldn't care about the latter if that would block a real bugfix. But since the above patch is not the proper fix IMO, I prefer being stable here. Regards, Wolfram
On 12/23/2014 12:26 PM, Wolfram Sang wrote: > >>>> This guarantees it will probe after GPIOs drivers. > > BTW this is not true to the best of my knowledge. It will make that > "very likely" but not "guarantee" anything. So, the race window is > smaller but it is still there. You need a proper fix anyhow. > Right. >>>> Platforms based on devicetree won't be affected by this change. >>> >>> Huh, why is that? >>> >> >> Unless I'm missing something here, our beloved DeviceTree guarantees to >> model the dependency between I2C slaves devices and the I2C master their >> connected to. > > Frankly, you are missing quite some things here. The I2C core registers > the clients when a master gets registered. No difference between > platform and DT here. > >> So, a machine fully-based on DeviceTree would never attempt to use the I2C >> bus without first registering the master, right? > > Neither would platform, that would be quite a bug. > >> This means there won't be any early users of the I2C platform driver in this >> scenario. > > There won't be with platform as well. Oh, OK. Then maybe you can clarify why all those i2c busses need to be registered with initcall in the first place? > But I think you are missing the > point. We are a *consumer* of GPIOs here. All of the above has nothing > to do with GPIO controllers being already available. > Hm, true. I was missing the fact that probe call order does not guarantee a succesful probe order. >>>> Legacy platforms, relying on the I2C being available early, might need >>>> to implement proper defered mechanisms to overcome potential problems. >>> >>> NAK. We can't say "Let's cause a regression to force people to fix >>> things that used to work" IMO. You exactly pointed out the problem that using >>> subsys_initcall() creates. >>> >>> What about fixing the drivers you use to support deferred probing when >>> acquitin the irq? >>> >> >> Maybe we can fix the legacy ones instead. However, a quick look shows there >> aren't any! >> >> $ git grep i2c_designware >> drivers/i2c/busses/i2c-designware-pcidrv.c:MODULE_ALIAS("i2c_designware-pci"); >> drivers/i2c/busses/i2c-designware-platdrv.c:MODULE_ALIAS("platform:i2c_designware"); >> drivers/i2c/busses/i2c-designware-platdrv.c: .name = "i2c_designware", >> >> Looks like this patch is pretty harmless. > > In-tree you are right. Out-of-tree, you probably aren't. I wouldn't care > about the latter if that would block a real bugfix. But since the above > patch is not the proper fix IMO, I prefer being stable here. > Fair enough. Thanks for the feedback,
> >> This means there won't be any early users of the I2C platform driver in this > >> scenario. > > > > There won't be with platform as well. > > Oh, OK. Then maybe you can clarify why all those i2c busses need to be > registered with initcall in the first place? Because they want to access PMICs early to have voltages ready when other drivers are probed. This, again, is DT/platform independent. All this is cruft from the pre-deferred-probe era. And pretty annoying to deprecated although I'd love to do that.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 2b463c3..579e65c 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -333,17 +333,7 @@ static struct platform_driver dw_i2c_driver = { }, }; -static int __init dw_i2c_init_driver(void) -{ - return platform_driver_register(&dw_i2c_driver); -} -subsys_initcall(dw_i2c_init_driver); - -static void __exit dw_i2c_exit_driver(void) -{ - platform_driver_unregister(&dw_i2c_driver); -} -module_exit(dw_i2c_exit_driver); +module_platform_driver(dw_i2c_driver); MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>"); MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
Currently, the driver is relying on a subsys_initcall() to register the platform driver struct. This is typically done to allow early uses of the I2C bus (for instance, I2C regulators used from early machine code). While this might work on some cases, it breaks on others. For instance, I2C devices with GPIO-wired interrupts will fail to request the interrupt because of this initcall usage. This commits attempts to fix the above issue, by converting the I2C driver into a regular kernel platform driver. This guarantees it will probe after GPIOs drivers. Platforms based on devicetree won't be affected by this change. Legacy platforms, relying on the I2C being available early, might need to implement proper defered mechanisms to overcome potential problems. Signed-off-by: Walter Lozano <walter@vanguardiasur.com.ar> --- drivers/i2c/busses/i2c-designware-platdrv.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)