Re: [PATCH 11/11] i2c: omap: enhance pinctrl support

[Grygorii Strashko]

Hi Kevin, Gururaja

On 05/31/2013 08:34 PM, Kevin Hilman wrote:
> Hebbar Gururaja <gururaja.hebbar@xxxxxx> writes:
>
> > Amend the I2C omap pin controller to optionally take a pin control
> > handle and set the state of the pins to:
> >
> > - "default" on boot, resume and before performing an i2c transfer
> > - "idle" after initial default, after resume default, and after each
> > i2c xfer
> > - "sleep" on suspend()
> >
> > By optionally putting the pins into sleep state in the suspend callback
> > we can accomplish two things.
> > - One is to minimize current leakage from pins and thus save power,
> > - second, we can prevent the IP from driving pins output in an
> > uncontrolled manner, which may happen if the power domain drops the
> > domain regulator.
> >
> > Note:
> > A .suspend & .resume callback is added which simply puts the pins to sleep
> > state upon suspend & are moved to default & idle state upon resume.
> >
> > If any of the above pin states are missing in dt, a warning message
> > about the missing state is displayed.
> > If certain pin-states are not available, to remove this warning message
> > pass respective state name with null phandler.
> >
> > (Changes based on i2c-nomadik.c)
> >
> > Signed-off-by: Hebbar Gururaja <gururaja.hebbar@xxxxxx>
> > Cc: Tony Lindgren <tony@xxxxxxxxxxx>
> > Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> > Cc: linux-omap@xxxxxxxxxxxxxxx
> > Cc: linux-i2c@xxxxxxxxxxxxxxx
> [...]
>
> > @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >   
> >   out:
> >   	pm_runtime_mark_last_busy(dev->dev);
> > +
> >   	pm_runtime_put_autosuspend(dev->dev);
> > +	/* Optionally let pins go into idle state */
> > +	if (!IS_ERR(dev->pins_idle))
> > +		if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
> > +			dev_err(dev->dev, "could not set pins to idle state\n");
> This is wrong.  You're changing the muxing before the device actually
> goes idle.  Anything you want to happen when the device actually idles
> for real has to be in runtime PM callbacks.
>
> Looking below, I see it's already in the callbacks, so why is it here also?

I have two questions regarding this patch & the whole series:

1) PM runtime suspend/resume
Current sequence:
- resume
  |- omap_hwmod_enable()
     |- _enable()
         |- omap_hwmod_mux(oh->mux, _HWMOD_STATE_ENABLED)
            |- enable module (sysc&clocks)
  |- omap_i2c_runtime_resume()
- suspend
  |- omap_i2c_runtime_suspend()
  |- omap_hwmod_idle()
     |- _idle()
         |- disbale module (sysc&clocks)
         |- omap_hwmod_mux(oh->mux, _HWMOD_STATE_IDLE);

The new order will change this sequence - *Is it safe?*:
- resume
  |- omap_hwmod_enable()
     |- _enable()
        |- enable module (sysc&clocks) <---- Is it valid to enable 
module before PINs?
  |- omap_i2c_runtime_resume()
     |- PINs state set to DEFAULT
- suspend
  |- omap_i2c_runtime_suspend()
     |- PINs state set to IDLE
  |- omap_hwmod_idle()
     |- _idle()
         |- disbale module (sysc&clocks) <---- Is it valid to disable 
module after PINs?

2) System suspend (OFF-mode) with assumption that "noirq" stage will be 
used for PINs cfg
Current implementation: handled in the same way as PM runtime


The new implementation:   -- total mess is here((:
- suspend_noirq - I2C device can be still active because of PM auto-suspend
  |-_od_suspend_noirq
     |- omap_i2c_suspend_noirq
        |- PINs state set to SLEEP
  |- pm_generic_runtime_suspend
     |- omap_i2c_runtime_suspend()
        |- PINs state set to IDLE  <--- *oops* PINs state is IDLE and 
not SLEEP
  |- omap_device_idle()
     |- omap_hwmod_idle()
        |- _idle()
           |- disbale module (sysc&clocks)

- resume_noirq - I2C was active before suspend
  |-_od_resume_noirq
     |- omap_hwmod_enable()
        |- _enable()
           |- enable module (sysc&clocks)
     |- pm_generic_runtime_resume
        |- omap_i2c_runtime_resume()
           |- PINs state set to DEFAULT  <--- !!!!
     |- omap_i2c_resume_noirq
        |- PINs state set to DEFAULT
        |- PINs state set to IDLE        <--- *big oops* we have active 
module and its
                                              PINs state is IDLE

All mentioned above, applicable for most of all patches in series.
And It seems, that this functionality can't be implemented in such way (
- OMAP device FW and Driver core might handle PINs states changing and
  drivers (DT) should provide set of available states only.

Am I wrong?

Regards,
-grygorii


> [...]
>
> > @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
> >   		omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
> >   	}
> >   
> > +	if (!IS_ERR(_dev->pins_idle))
> > +		if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
> > +			dev_err(dev, "could not set pins to idle state\n");
> > +
> >   	return 0;
> >   }
> >   
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/