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

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

Hello,

On Wed, Aug 19, 2015 at 05:52:11PM +0800, Gao Pan wrote:
> @@ -963,6 +973,59 @@ out:
>  	return (result < 0) ? result : num;
>  }
>  
> +static int i2c_imx_get_scl(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +
> +	return gpiod_get_value(i2c_imx->pins.scl);
> +}
> +
> +static int i2c_imx_get_sda(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +
> +	return gpiod_get_value(i2c_imx->pins.sda);
> +}
> +
> +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	gpiod_set_value(i2c_imx->pins.scl, val);
> +}

<nitpick>
These three functions are very similar (i.e. 1 line respectively for
variable declaration, driver data assignment and gpio operation), but
the last has an empty line less. 
</nitpick>

> +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);
> +	pinctrl_pm_select_sleep_state(&adap->dev);

I still think this is not nice, the v2 thread is still active with Linus
W. being involved.

> +	gpiod_direction_input(i2c_imx->pins.sda);
> +	gpiod_direction_output(i2c_imx->pins.scl, 1);
> +}
> [...]
> @@ -1011,12 +1074,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;
In a former review round I suggested to only do this assignment after
the two gpios were aquired successfully. Then the above checks could be

	if (i2c_imx->adapter.bus_recovery_info)

instead of

	if (i2c_imx->pins.sda && i2c_imx->pins.scl)

which IMHO looks nicer and more robust.

Best regards
Uwe

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