From: linux-i2c-owner@xxxxxxxxxxxxxxx <mailto:linux-i2c-owner@xxxxxxxxxxxxxxx> Sent: Wednesday, August 19, 2015 3:02 PM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan > Fugang-B38611; Linus Walleij > Subject: Re: [Patch v2] i2c: imx: implement bus recovery > > 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@xxxxxxxxxxxxxx> 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. Thanks for your precise review. > > > > > > > @@ -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? Yes, you are right, will change it in the next version. > > > > > > > @@ -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? Yes, you are right. The gpios should be set low. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
