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. > > 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; > } 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html