[RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Walter Lozano]

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-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
 MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
-- 
1.7.10.4

Re: [RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Walter Lozano]

On Mon, Dec 22, 2014 at 4:02 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> 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

-- 
Walter Lozano, VanguardiaSur
www.vanguardiasur.com.ar

Re: [RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Ezequiel Garcia]

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!
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]


> >> 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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Ezequiel Garcia]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]



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,
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] i2c: designware: Avoid initcall and initialize the driver like a regular one

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]


> >> 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]