From: linux-i2c-owner@xxxxxxxxxxxxxxx <mailto:linux-i2c-owner@xxxxxxxxxxxxxxx> Sent: Friday, September 18, 2015 3:55 PM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; shawnguo@xxxxxxxxxx; kernel@xxxxxxxxxxxxxx; linux- > i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan Fugang-B38611; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [Patch V5] i2c: imx: implement bus recovery > > Hello, > > 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; ...... > > > + if (IS_ERR(i2c_imx->pinctrl_pins_default)) > > + dev_warn(&pdev->dev, "could not get default state\n"); > > + else { > > + i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx- > >pinctrl, > > + I2C_PINCTRL_STATE_GPIO); > > + if (IS_ERR(i2c_imx->pinctrl_pins_gpio)) > > + dev_warn(&pdev->dev, "could not get gpio state\n"); > > 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))) { 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; } i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info; } > > > + } > > + > > /* Request IRQ */ > > ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0, > > pdev->name, i2c_imx); > > @@ -1057,6 +1146,24 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > if (ret < 0) > > goto rpm_disable; > > > > + /* Init recover pins */ > > + 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; > > + } > > + > > if (i2c_imx->pins.sda && i2c_imx->pins.scl) > > > + i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info; > > + > > /* Set up clock divider */ > > i2c_imx->bitrate = IMX_I2C_BIT_RATE; > > ret = of_property_read_u32(pdev->dev.of_node, > > 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 -- 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
