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

On 02/13/2013 05:34 PM, Doug Anderson wrote:

On Wed, Feb 13, 2013 at 1:02 PM, Stephen Warren [off-list ref] wrote:

quoted

On 02/13/2013 11:02 AM, Doug Anderson wrote:

quoted

The i2c-arbitrator driver implements the arbitration scheme that the
Embedded Controller (EC) on the ARM Chromebook expects to use for bus
multimastering.  This i2c-arbitrator 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.

quoted

diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator.txt

...

quoted

quoted

+Required properties:
+- compatible: i2c-arbitrator

That seems pretty generic. What if there are other arbitration schemes?

OK, going to go with i2c-arbitrator-cros-ec.  Hopefully that sounds OK.

That seems fine. The mechanism seems potentially a little more generic
than just for cros-ec though, but I guess there's no harm naming it
after the first implementation. That why "compatible" allows multiple
entries anyway.

quoted

quoted

+static int i2c_arbitrator_select(struct i2c_adapter *adap, void *data, u32 chan)
+{
+     const struct i2c_arbitrator_data *arb = data;
+     unsigned long stop_retry, stop_time;
+
+     /* Start a round of trying to claim the bus */
+     stop_time = jiffies + usecs_to_jiffies(arb->wait_free_us) + 1;
+     do {
+             /* Indicate that we want to claim the bus */
+             gpio_set_value(arb->ap_gpio, 0);

The GPIO signals appear to be active low in the code. Instead, I think
it'd make more sense to extract the polarity of the GPIO from DT, using
of_get_named_gpio_flags().

A little torn here.  It adds a bunch of complexity to the code to
handle this case and there are no known or anticipated users.  I only
wish that the GPIO polarity could be more hidden from drivers (add
functions like gpio_assert, gpio_deassert, etc)...

Yes, that would be nice. Alex, LinusW?

In any case, I've done it.  I used the "!!" trick a lot to convert
"zero/non-zero" to "0/1" to at least reduce the lines of code a
little.  I think this is common enough that it helps readability
rather than hurting it.

quoted

quoted

+static int __init i2c_arbitrator_init(void)
+{
+     return platform_driver_register(&i2c_arbitrator_driver);
+}
+subsys_initcall(i2c_arbitrator_init);
+
+static void __exit i2c_arbitrator_exit(void)
+{
+     platform_driver_unregister(&i2c_arbitrator_driver);
+}
+module_exit(i2c_arbitrator_exit);

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?