[PATCH v2 1/3] i2c: mux: Add i2c-arbitrator-cros-ec 'mux' driver

[Doug Anderson]

Stephen,

New version will come shortly with fixes below...


On Fri, Feb 15, 2013 at 8:39 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 02/14/2013 05:21 PM, Doug Anderson wrote:
>> The i2c-arbitrator-cros-ec driver implements the arbitration scheme
>> that the Embedded Controller (EC) on the ARM Chromebook expects to use
>> for bus multimastering.  This i2c-arbitrator-cros-ec driver could also
>> be used in other places where standard I2C bus arbitration can't be
>> used and two extra GPIOs are available for arbitration.
>>
>> This driver is based on code that Simon Glass added to the i2c-s3c2410
>> driver in the Chrome OS kernel 3.4 tree.  The current incarnation as a
>> mux driver is as suggested by Grant Likely.  See
>> <https://patchwork.kernel.org/patch/1877311/> for some history.
>
>> diff --git a/drivers/i2c/muxes/i2c-arbitrator-cros-ec.c b/drivers/i2c/muxes/i2c-arbitrator-cros-ec.c
>
>> +static int i2c_arbitrator_probe(struct platform_device *pdev)
>
>> +     arb->parent = of_find_i2c_adapter_by_node(parent_np);
>> +     if (WARN_ON(!arb->parent))
>> +             return -EINVAL;
>
> I think for all error paths after this point, a call to
> i2c_put_adapter() is needed.

Right.  I messed that up when I reworked.  I'll reorder this one to
last since it's the only one that needs cleanup.  Thanks for catching!


>> +     /* Request GPIOs */
>> +     ret = of_get_named_gpio_flags(np, "ap-claim-gpio", 0, &gpio_flags);
>> +     if (WARN_ON(!gpio_is_valid(ret)))
>> +             return ret;
>
> You shouldn't warn in all cases; -EPROBE_DEFER can quite legitimately
> happen and only temporarily prevents probe() - it's retried later.

Done.


>> +     arb->ap_gpio = ret;
>
> Nit: Perhaps simply assign arb->ap_gpio directly from the
> of_get_named_gpio_flags call? It would make that call slightly clearer.

With the addition of checks for -EPROBE_DEFER this causes several line
wraps including the "if" test so not doing.


>> +     arb->ap_gpio_release = !!(gpio_flags & OF_GPIO_ACTIVE_LOW);
>> +     out_init = (gpio_flags & OF_GPIO_ACTIVE_LOW) ?
>> +             GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
>
> Nit: I'd be tempted to test arb->ap_gpio_release here, rather than
> extracting the value from the flags again. Semantically equivalent, but
> it seems to slightly more directly show why it's being tested.

I wrote it both ways and decided I liked this way better.  IMHO the
symmetry between the ec_gpio_release code and the ap_gpio_release code
is worth the extra test.  ...but I can see both sides of the argument
so will change it if you wish.


>> +subsys_initcall(i2c_arbitrator_init);
>
> You mentioned that you only saw problems using
> module_init/module_platform_driver in your downstream tree, so the
> problem doesn't affect upstream. Presumably those problems would be
> fixed when upstreaming any other drivers into the mainline kernel. I'd
> still be tempted to just use module_platform_driver here. But, I guess
> I'm fine with the patch either way; I'll leave the call to Wolfram.

I take it you thought my "since this provides a bus it should be a
subsystem" argument was BS?  It seems like the "it needs to be this
way for correctness" is a pretty weak argument, but even if we fix the
other problems it still seems like it is more ideal for bus providers
to be registered as subsystems just to avoid defers (so you get a
faster boot).

If it will make the patch more palatable I'll move it to
module_platform_driver() and we'll figure out how to make it work.


-Doug