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

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

On Wed, Mar 13, 2013 at 09:36:21AM -0700, 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.
> 
> Signed-off-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Tested-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

I'd like to have the bindings more generic. They should allow for n
possible masters IMO. It doesn't need to be implemented right now, but
it should be possible to add that later.

> ---
> Changes in v4: None
> Changes in v3:
> - Handle of_find_i2c_adapter_by_node() failure more properly by
>   changing init order.
> - Don't warn on -EPROBE_DEFER from calls that could return it.
> - Move to module_platform_driver().  As we pull in parts of the system
>   that rely on devices under this i2c bus we'll need to make sure they
>   can handle the fact that they'll be initted later now.
> 
> Changes in v2:
> - Renamed to i2c-arbitrator-cros-ec.
> - Documented "microsecond" properties as optional; removed
>   "bus-arbitration" prefix since it was just extra wordy.
> - Split GPIOs into two properties to make it cleaner.
> - Capitalized I2C in freeform text.
> - Get 'active low' from device tree.
> 
>  .../bindings/i2c/i2c-arbitrator-cros-ec.txt        |  76 +++++++

I wonder about a more generic name. i2c-arb-gpio-challenge.* maybe?

>  drivers/i2c/muxes/Kconfig                          |  11 +
>  drivers/i2c/muxes/Makefile                         |   2 +
>  drivers/i2c/muxes/i2c-arbitrator-cros-ec.c         | 222 +++++++++++++++++++++
>  4 files changed, 311 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-arbitrator-cros-ec.txt
>  create mode 100644 drivers/i2c/muxes/i2c-arbitrator-cros-ec.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-arbitrator-cros-ec.txt b/Documentation/devicetree/bindings/i2c/i2c-arbitrator-cros-ec.txt
> new file mode 100644
> index 0000000..1f893e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-arbitrator-cros-ec.txt
> @@ -0,0 +1,76 @@
> +GPIO-based Arbitration used by the ARM Chromebook (exynos5250-snow)
> +===================================================================
> +This uses GPIO lines between the AP (Application Processor) and an attached
> +EC (Embedded Controller) which both want to talk on the same I2C bus as master.
> +
> +The AP and EC each have a 'bus claim' line, which is an output that the
> +other can see. These are both active low, with pull-ups enabled.
> +
> +- AP_CLAIM: output from AP, signalling to the EC that the AP wants the bus
> +- EC_CLAIM: output from EC, signalling to the AP that the EC wants the bus

I'd like to drop the specific terms of AP and EC and just talk about
multiple masters.

> +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.
> +
> +
> +Algorithm:
> +
> +The basic algorithm is to assert your line when you want the bus, then make
> +sure that the other side doesn't want it also. A detailed explanation is best
> +done with an example.
> +
> +Let's say the AP wants to claim the bus. It:
> +1. Asserts AP_CLAIM.
> +2. Waits a little bit for the other side to notice (slew time, say 10
> +   microseconds).
> +3. Checks EC_CLAIM. If this is not asserted then the AP has the bus and we are
> +   done.
> +4. Otherwise, wait for a few milliseconds and see if EC_CLAIM is released.
> +5. If not, back off, release the claim and wait for a few more milliseconds.
> +6. Go back to 1 (until retry time has expired).
> +
> +
> +Required properties:
> +- compatible: i2c-arbitrator-cros-ec
> +- ap-claim-gpio: The GPIO that we (the AP) use to claim the bus.
> +- ec-claim-gpio: The GPIO that the other side (the EC) uses the claim the bus.

An array based approach like in the i2c-mux-gpio driver would be more
generic. Just mention that the driver only supports 2 entries at the
moment.

> +- Standard I2C mux properties. See mux.txt in this directory.
> +- Single I2C child bus node at reg 0. See mux.txt in this directory.
> +
> +Optional properties:
> +- slew-delay-us: microseconds to wait for a GPIO to go high. Default is 10 us.
> +- wait-retry-us: we'll attempt another claim after this many microseconds.
> +    Default is 3000 us.
> +- wait-free-us: we'll give up after this many microseconds. Default is 50000 us.

Grant, I assume it is okay to introduce these generic bindings?

> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 0be5b83..ca19378 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -5,6 +5,17 @@
>  menu "Multiplexer I2C Chip support"
>  	depends on I2C_MUX
>  
> +config I2C_ARBITRATOR_CROS_EC
> +	tristate "GPIO-based I2C arbitrator used on exynos5250-snow"
> +	depends on GENERIC_GPIO && OF
> +	help
> +	  If you say yes to this option, support will be included for an
> +	  I2C multimaster arbitration scheme using GPIOs that is used in
> +	  the Samsung ARM Chromebook (exynos5250-snow).
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-arbitrator-cros-ec.
> +

This text could be more generic then, too.

> +static int i2c_arbitrator_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *parent_np;
> +	struct i2c_arbitrator_data *arb;
> +	enum of_gpio_flags gpio_flags;
> +	unsigned long out_init;
> +	int ret;
> +
> +	/* We only support probing from device tree; no platform_data */
> +	if (WARN_ON(!np))
> +		return -ENODEV;

Too much WARN_ON for my taste.

Thanks,

   Wolfram