[PATCH v1 1/4] i2c: mux: Add i2c-arbitrator 'mux' driver

[Doug Anderson]

On Wed, Feb 13, 2013 at 4:54 PM, Doug Anderson <dianders at chromium.org> wrote:
>>>> You should be able to replace all that with:
>>>>
>>>> module_platform_driver(&i2c_arbitrator_driver);
>>>
>>> Sigh.  Yeah, I had that.  ...but it ends up getting initted
>>> significantly later in the init process and that was uncovering bugs
>>> in other drivers where they weren't expressing their dependencies
>>> properly.  I was going to try to fix those bugs separately but it
>>> seemed to make some sense to prioritize this bus a little bit anyway
>>> by using subsys_initcall().  ...but maybe that's just wrong.
>>>
>>> Unless you think it's a bug or terrible form to use subsys_initcall()
>>> I'd rather leave that.  Is that OK?
>>
>> It's certainly a bug if it doesn't work correctly as
>> module_platform_driver(). It'll have to be fixed sometime.
>>
>> I don't think it's a big enough issue for me to object to the patch
>> providing it gets fixed sometime, but I've certainly seem other people
>> push back harder on using subsys_initcall for expressing dependencies;
>> it's not very extensible - what happens if there's another bug in some
>> other driver that requires an even earlier level of initcall?
>
> I don't like it either.  I'll give it a few hours tomorrow and
> hopefully I can track down the problem.  If I can't track it down or
> if I come up with a really good justification for why it's needed I'll
> leave it with subsys_initcall() unless someone gives me a big nak.

OK, so I dug into my problems here a little bit.  All of the problems
are with a private branch that includes stuff not fully upstream,
but...

The problem is that we've got a regulator (tps65090) on this bus.
Right now the first code that wants to use tps65090 runs from the
set_power() callback of "platform-lcd".  It looks like:
   lcd_fet = regulator_get(NULL, "lcd_vdd");

...and "platform-lcd" is instantiated really early via
platform_device_register() for some reason.

I tried to fix it by moving platform-lcd to actually be instantiated
via the device tree (with platform data populated through
of_platform_populate).  I then hooked up regulators through the device
tree:

    platform-lcd {
        compatible = "platform-lcd";
        vcd_led-supply = <&vcd_led>;
        lcd_vdd-supply = <&lcd_vdd>;
    };

...I verified that these worked (needed a small mod to tps65090) when
I used subsys_initcall().  AKA: I could rename lcd_vdd-supply to
doug-supply and then change the code to ask for doug-supply and it
worked so I know that the device tree regulator association worked.

...but when I moved to module_platform_driver() then things still broke.

[    1.510000] platform-lcd supply lcd_vdd not found, using dummy regulator

I was sorta hoping that there would be some magic where
regulator_get() would find the device tree node for the regulator and
then resolve the chain.  ...but maybe that's a pipe dream.


Is there some better way I should be expressing dependencies?  Do I
need to try to hack something together with -EAGAIN (ick!)?  I will
point out that the i2c bus that we're arbitrating on also registers
with subsys_initcall().


In any case, I've spent my few hours today.  It's not a waste of time
since the above learnings were good, but I haven't actually found a
way to avoid the subsys_initcall().  Unless someone can point to what
I'm missing, I'll send another patch up with subsys_initcall(),
hopefully by the end of the day...


Thanks for listening!

-Doug