From: Uwe Kleine-König <mailto:u.kleine-koenig@xxxxxxxxxxxxxx> Sent: Tuesday, September 08, 2015 5:27 AM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan > Fugang-B38611; kernel@xxxxxxxxxxxxxx > Subject: Re: [Patch v4] i2c: imx: implement bus recovery > > Hello, > > On Wed, Aug 19, 2015 at 05:52:11PM +0800, Gao Pan wrote: > > @@ -963,6 +973,59 @@ out: > > return (result < 0) ? result : num; > > } > > > > +static int i2c_imx_get_scl(struct i2c_adapter *adap) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + > > + return gpiod_get_value(i2c_imx->pins.scl); > > +} > > + > > +static int i2c_imx_get_sda(struct i2c_adapter *adap) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + > > + return gpiod_get_value(i2c_imx->pins.sda); > > +} > > + > > +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val) { > > + struct imx_i2c_struct *i2c_imx; > > + > > + i2c_imx = container_of(adap, struct imx_i2c_struct, adapter); > > + gpiod_set_value(i2c_imx->pins.scl, val); } > > <nitpick> > These three functions are very similar (i.e. 1 line respectively for > variable declaration, driver data assignment and gpio operation), but the > last has an empty line less. > </nitpick> Thanks. > > +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); > > + pinctrl_pm_select_sleep_state(&adap->dev); > > I still think this is not nice, the v2 thread is still active with Linus > W. being involved. Thank you very much, will change it as Linus's suggestion in next version. > > + gpiod_direction_input(i2c_imx->pins.sda); > > + gpiod_direction_output(i2c_imx->pins.scl, 1); } > > [...] > > @@ -1011,12 +1074,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; > In a former review round I suggested to only do this assignment after the > two gpios were aquired successfully. Then the above checks could be > > if (i2c_imx->adapter.bus_recovery_info) > > instead of > > if (i2c_imx->pins.sda && i2c_imx->pins.scl) > > which IMHO looks nicer and more robust. Thanks, will correct 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
