On Fri, May 31, 2013 at 23:04:59, 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? Just to be double sure. Seems it is unwanted. > > [...] > > > @@ -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 > Regards, Gururaja -- 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
