Re: [Patch v2] i2c: imx: implement bus recovery

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Fri, Jul 17, 2015 at 10:36:45AM +0800, Gao Pan wrote:
> Implement bus recovery methods for i2c-imx so we can recover from
> situations where SCL/SDA are stuck low.
> 
> Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
> pinctrl to gpio mode by calling pinctrl sleep set function, and then
> use GPIO to emulate the i2c protocol to send nine dummy clock to recover
> i2c device. After recovery, set i2c pinctrl to default group setting.
> 
> V2:
> As Uwe Kleine-König's suggestion, the version do below changes:
>  -replace of_get_named_gpio() with devm_gpiod_get_optional()
>  -move gpio_direction_input() and gpio_direction_output() call to the
>   prepare callback
>  -use 0 and 1 as return value for the get_scl and get_sda callbacks

this section usally goes after the triple-dash below. Otherwise this is
included in the commit log which it shouldn't.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> index ce4311d..a9fe525 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> @@ -14,6 +14,8 @@ Optional properties:
>    The absence of the propoerty indicates the default frequency 100 kHz.
>  - dmas: A list of two dma specifiers, one for each entry in dma-names.
>  - dma-names: should contain "tx" and "rx".
> +- scl-gpio: specify the gpio related to SCL pin
> +- sda-gpio: specify the gpio related to SDA pin

In the meantime I learned that "...-gpios" is the recommended name for
such properties, even if conceptually there can only be a single gpio
here. (You might want to help to change that?!)

> @@ -436,7 +444,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>  		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
>  			dev_dbg(&i2c_imx->adapter.dev,
>  				"<%s> I2C bus is busy\n", __func__);
> -			return -ETIMEDOUT;
> +			return i2c_recover_bus(&i2c_imx->adapter);
>  		}
>  		schedule();
>  	}
> @@ -450,7 +458,7 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
>  
>  	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
>  		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
> -		return -ETIMEDOUT;
> +		return i2c_recover_bus(&i2c_imx->adapter);

I'm still not convinced that doing that here unconditionally is a good
idea. Wolfram will know for sure.

> +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	if (i2c_imx->pins.sda && i2c_imx->pins.scl) {
> +		pinctrl_pm_select_sleep_state(&adap->dev);

Your requirement that the sleep state should configure the pins as gpio
is strange. Maybe better introduce a dedicated state for recovery? At
least you should document this requirement.

> @@ -1004,12 +1073,13 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  
>  	/* Setup i2c_imx driver structure */
>  	strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx->adapter.name));
> -	i2c_imx->adapter.owner		= THIS_MODULE;
> -	i2c_imx->adapter.algo		= &i2c_imx_algo;
> -	i2c_imx->adapter.dev.parent	= &pdev->dev;
> -	i2c_imx->adapter.nr		= pdev->id;
> -	i2c_imx->adapter.dev.of_node	= pdev->dev.of_node;
> -	i2c_imx->base			= base;
> +	i2c_imx->adapter.owner = THIS_MODULE;
> +	i2c_imx->adapter.algo = &i2c_imx_algo;
> +	i2c_imx->adapter.dev.parent = &pdev->dev;
> +	i2c_imx->adapter.nr = pdev->id;
> +	i2c_imx->adapter.dev.of_node = pdev->dev.of_node;
> +	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;

What happens if the device tree only specifies a gpio for sda but not
scl? The recovery stuff is still hooked up, right? What about only doing
that if both properties are valid? This would also get rid of the tests
for validity in the recovery callbacks.

> @@ -1037,6 +1107,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	/* Set up adapter data */
>  	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
>  
> +	/* Init recover pins */
> +	i2c_imx->pins.sda =
> +		devm_gpiod_get_optional(&pdev->dev, "sda-gpio", 0);
> +	i2c_imx->pins.scl =
> +		devm_gpiod_get_optional(&pdev->dev, "scl-gpio", 0);

0 is wrong here, better use GPIOD_ASIS. Is there a reason not to
configure the direction as input already here?

> +
> +	if (IS_ERR(i2c_imx->pins.sda)) {
> +		ret = PTR_ERR(i2c_imx->pins.sda);
> +		dev_err(&pdev->dev, "Failed to get sda gpio: %d\n", ret);
> +		i2c_imx->pins.sda = NULL;
> +	}

As you're using devm_gpiod_get_optional there is no justification to
ignore the error code.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |