From: Uwe Kleine-König <mailto:[email protected]> Sent: Tuesday,
September 08, 2015 5:27 AM
> To: Gao Pan-B54642
> Cc: [email protected]; [email protected]; Li Frank-B20596; Duan
> Fugang-B38611; [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
