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

[Stephen Warren <swarren@wwwdotorg.org>]

On 02/13/2013 11:02 AM, Doug Anderson wrote:
> 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.

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

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

> +Required properties:
> +- compatible: i2c-arbitrator

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

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

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

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

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

> +static int i2c_arbitrator_probe(struct platform_device *pdev)

> +	/* 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.

> +	/* 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.

> +static int i2c_arbitrator_remove(struct platform_device *pdev)

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

> +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);

> +MODULE_LICENSE("GPL");

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