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

Stephen,

Thank you for the review.  Comments below (including changes I have
done locally).  I probably won't have time to test / repost until
tomorrow.


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

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

+This mechanism is used instead of standard i2c multimaster to avoid some of the
+subtle driver and silicon bugs that are often present with i2c multimaster.

Being really pick here, but I2C should be capitalized in free-form text.
There are a few other places where the comment applies.

Done.

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.

quoted

+- bus-arbitration-gpios: Two GPIOs to use with the GPIO-based bus
+    arbitration protocol.  The first should be an output, and is used to
+    claim the I2C bus for us (AP_CLAIM).  The second should be an input and
+    signals that the other side wants to claim the bus (EC_CLAIM).

Is it worth using two separate properties here, so they each get a
unique name. That way, nobody has the remember which order the two GPIOs
come in.

Yes, I think it's better too.  Done.

quoted

diff --git a/drivers/i2c/muxes/i2c-arbitrator.c b/drivers/i2c/muxes/i2c-arbitrator.c

quoted

+enum {
+     I2C_ARB_GPIO_AP,                /* AP claims i2c bus */
+     I2C_ARB_GPIO_EC,                /* EC claims i2c bus */
+
+     I2C_ARB_GPIO_COUNT,
+};

Oh, I see from later code that those are indices into the
bus-arbitration-gpios DT property. I thought they were states in some
state machine at first. A comment might help here, if you continue to
use one property.

Removed this bit of code.

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

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

+static int i2c_arbitrator_probe(struct platform_device *pdev)

quoted

+     /* Request GPIOs */
+     ret = of_get_named_gpio(np, "bus-arbitration-gpios", I2C_ARB_GPIO_AP);
+     if (gpio_is_valid(ret)) {
+             arb->ap_gpio = ret;
+             ret = gpio_request_one(arb->ap_gpio, GPIOF_OUT_INIT_HIGH,
+                                    "bus-arbitration-ap");
+     }
+     if (WARN_ON(ret))
+             goto ap_request_failed;

you could use devm_gpio_request_one() and save some cleanup logic.

Whoops, didn't realize that was there.  Done.

quoted

+     /* Arbitration parameters */
+     if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
+                              &arb->slew_delay_us))
+             arb->slew_delay_us = 10;

The DT binding document says that property is required. Either the code
should error out here, or the document updated to indicate that the
properties are optional, and specify what the defaults are.

Done.  Now optional.

quoted

+static int i2c_arbitrator_remove(struct platform_device *pdev)

quoted

+     platform_set_drvdata(pdev, NULL);

You shouldn't have to do that; nothing should care what the pdata value
is while the device isn't probed anyway.

Done.  Code was stolen from i2c_mux_gpio_remove(), so perhaps we
should remove it there too.  I'll send up a quick cleanup patch
tomorrow.

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?

quoted

+MODULE_LICENSE("GPL");

The header says GPL v2 only, so "GPL v2".

Done.