Re: [Patch v2] i2c: imx: implement bus recovery

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hallo Linus,

On Tue, Aug 25, 2015 at 04:18:16PM +0200, Linus Walleij wrote:
> On Wed, Aug 19, 2015 at 9:02 AM, Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Wed, Aug 19, 2015 at 03:44:49AM +0000, Gao Pandy wrote:
> >> From: Uwe Kleine-König <mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sent: Thursday, August 13, 2015 4:15 PM
> >> > > +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.
> >
> > Linus, do you have to say something here? It might be right to have the
> > gpio function as configuration for sleep mode, but it doesn't look right
> > for me to use this for recovery purposes.
> 
> What it usually means is that the pin has a function mode such
> that an asynchronous edge detector is connected to it on the
> outer pad ring, maybe in tristate or some pull-up/down configuration.
> This makes is possible for the system to power
> off completely until an event occurs there with only some
> very peripheral electronics powered up.
> 
> I think the terminology depend on the use case. See the section
> "GPIO mode pitfalls" in Documentation/pinctrl.txt
> 
> If the use case is around the i2c traffic, it is a mode related to I2C,
> and if this mode is called "GPIO mode" in the data sheet
> is irrelevant, because it is obviously not used for the generic
> input/output but the specific I2C. The terminology should be
> made familiar to whoever needs to go in and read the code later.
The background info that was obviously missing from the part of the
thread I sent to you is that pinctrl_pm_select_sleep_state is used to
prepare bitbanging on the i2c bus to do bus recovery. (The controller
doesn't implement this, so we have to resort to manually drive the
pins.)

Is this good enough? I'd like to see implemented a separate pinctrl set
for bitbanging instead of relying on the sleep state being the right
thing to enable gpio functionality.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |