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

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

Hello,

Cc += Linus Walleij

On Wed, Aug 19, 2015 at 03:44:49AM +0000, Gao Pandy wrote:
> From: Uwe Kleine-König <mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sent: Thursday, August 13, 2015 4:15 PM
> > > +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.
> 
> In general, pinctrl sleep mode is gpio function. I will add this in binding doc. Thanks.

Linus, do you have to say something here? It might be right to have the
gpio function as configuration for sleep mode, but it doesn't look right
for me to use this for recovery purposes.

 
> > 
> > > @@ -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.
> 
> It requires device tree to specify gpio for sda and scl in both. Once only sda is defined, 
> the driver doesn't carry out the recovery process.

Right, and still it is convinced it is able to recover the bus, right?
Can you please change this?

> > 
> > > @@ -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?
>  
> Thanks, 0 is inappropriate here. Actually, we should use GPIOD_OUT_HIGH
> to set the gpios high. 

Are you sure? Consider the bus is blocked and you switch to gpio mode to
do recovery you have a device pulling SDA to 0 and the imx pulling it to
1. That's wrong, isn't it?

Best regards
Uwe

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