From: Uwe Kleine-König <mailto:u.kleine-koenig@xxxxxxxxxxxxxx> Sent: Thursday, August 13, 2015 4:15 PM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan Fugang-B38611 > Subject: Re: [Patch v2] i2c: imx: implement bus recovery > > Hello, > > On Fri, Jul 17, 2015 at 10:36:45AM +0800, Gao Pan wrote: > > Implement bus recovery methods for i2c-imx so we can recover from > > situations where SCL/SDA are stuck low. > > > > Once i2c bus SCL/SDA are stuck low during transfer, config the i2c > > pinctrl to gpio mode by calling pinctrl sleep set function, and then > > use GPIO to emulate the i2c protocol to send nine dummy clock to > > recover i2c device. After recovery, set i2c pinctrl to default group > setting. > > > > V2: > > As Uwe Kleine-König's suggestion, the version do below changes: > > -replace of_get_named_gpio() with devm_gpiod_get_optional() -move > > gpio_direction_input() and gpio_direction_output() call to the > > prepare callback > > -use 0 and 1 as return value for the get_scl and get_sda callbacks > > this section usally goes after the triple-dash below. Otherwise this is > included in the commit log which it shouldn't. Thanks, will change it in the next version. > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > b/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > index ce4311d..a9fe525 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt > > @@ -14,6 +14,8 @@ Optional properties: > > The absence of the propoerty indicates the default frequency 100 kHz. > > - dmas: A list of two dma specifiers, one for each entry in dma-names. > > - dma-names: should contain "tx" and "rx". > > +- scl-gpio: specify the gpio related to SCL pin > > +- sda-gpio: specify the gpio related to SDA pin > > In the meantime I learned that "...-gpios" is the recommended name for > such properties, even if conceptually there can only be a single gpio > here. (You might want to help to change that?!) Thanks, will change it in the next version. > > @@ -436,7 +444,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct > *i2c_imx, int for_busy) > > if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) > { > > dev_dbg(&i2c_imx->adapter.dev, > > "<%s> I2C bus is busy\n", __func__); > > - return -ETIMEDOUT; > > + return i2c_recover_bus(&i2c_imx->adapter); > > } > > schedule(); > > } > > @@ -450,7 +458,7 @@ static int i2c_imx_trx_complete(struct > > imx_i2c_struct *i2c_imx) > > > > if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) { > > dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__); > > - return -ETIMEDOUT; > > + return i2c_recover_bus(&i2c_imx->adapter); > > I'm still not convinced that doing that here unconditionally is a good > idea. Wolfram will know for sure. Thanks, I will remove this change in next version. > > +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. > > > @@ -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. > > > @@ -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. > > + > > + if (IS_ERR(i2c_imx->pins.sda)) { > > + ret = PTR_ERR(i2c_imx->pins.sda); > > + dev_err(&pdev->dev, "Failed to get sda gpio: %d\n", ret); > > + i2c_imx->pins.sda = NULL; > > + } > > As you're using devm_gpiod_get_optional there is no justification to > ignore the error code. Thanks, will change it in next version. -- 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
