From: Uwe Kleine-König <mailto:u.kleine-koenig@xxxxxxxxxxxxxx> Sent: Monday, September 21, 2015 2:33 PM > To: Gao Pan-B54642 > Cc: Li Frank-B20596; wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; > kernel@xxxxxxxxxxxxxx; Duan Fugang-B38611; shawnguo@xxxxxxxxxx; linux- > arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [Patch V5] i2c: imx: implement bus recovery > > Hello, > > On Mon, Sep 21, 2015 at 04:29:20AM +0000, Gao Pandy wrote: > > > On Fri, Sep 11, 2015 at 06:42:34PM +0800, Gao Pan wrote: > > > > V5: > > > > -introduce a dedicated gpio state for bus recovery. > > > > -assign adapter.bus_recovery_info after the two gpios were aquired > > > > successfully. > > > > > > You also do this if the gpios were not acquired successfully. > > > > Thanks. If the gpios are not acquired successfully, the context goes > > to label clk_disable. So the assignment is skipped. > > Please see the following code. > > > > ...... > > > > if (IS_ERR(i2c_imx->pins.sda)) { > > ret = PTR_ERR(i2c_imx->pins.sda); > > goto clk_disable; > > } > > if (IS_ERR(i2c_imx->pins.scl)) { > > ret = PTR_ERR(i2c_imx->pins.scl); > > goto clk_disable; > > } > > > > i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info; > > > > ...... > > Right, if devm_gpiod_get_optional returns an error probing fails and > everything is right. If however devm_gpiod_get_optional returns NULL (i.e. > the dt doesn't contain an scl-gpios property) you happily use them even > though gpio_set_value are stubs in this case. Thanks, you are right. > > > IMHO pinctrl_lookup_state returning an error is enough to not try a > > > recovery. > > > > Thanks, you are right. How about the following logic. > > > > if (!(IS_ERR(i2c_imx->pinctrl_pins_default)) && > > !(IS_ERR(i2c_imx->pinctrl_pins_gpio))) { > > After the ! you don't need a (, but apart from this the logic is fine. > > > i2c_imx->pins.sda = > > devm_gpiod_get_optional(&pdev->dev, "sda-gpios", > GPIOD_IN); > > i2c_imx->pins.scl = > > devm_gpiod_get_optional(&pdev->dev, > > "scl-gpios", GPIOD_IN); > > > > if (IS_ERR(i2c_imx->pins.sda)) { > > ret = PTR_ERR(i2c_imx->pins.sda); > > goto clk_disable; > > } > > > > if (IS_ERR(i2c_imx->pins.scl)) { > > ret = PTR_ERR(i2c_imx->pins.scl); > > goto clk_disable; > > } > > As said above, here you need an > > if (i2c_imx->pins.scl && i2c_imx->pins.sda) > > i2c_imx->adapter.bus_recovery_info = > &i2c_imx_bus_recovery_info; > > } Thank you very much, will fix it in next version. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | http://www.pengutronix.de/ -- 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
