* PGP Signed by an unknown keySo in non-ext control modes, should it return with doing nothing? And for ext-control case, should it do reverse of disable?
On Wed, Jan 25, 2012 at 09:18:30PM +0530, Laxman Dewangan wrote:
+static int tps65910_set_suspend_enable(struct regulator_dev *dev)I'm really confuseed now. This definitely looks like it's doing the
+{
+ struct tps65910_reg *pmic = rdev_get_drvdata(dev);
+ int id = rdev_get_id(dev);
+ /*
+ * If regulator is controlled through external control then
+ * it can be enable/disable by toggling external signal.
+ */
+ if (pmic->board_ext_control[id])
+ return 0;
+ else
+ return tps65910_set_mode(dev, REGULATOR_MODE_NORMAL);
+}
wrong thing for the non-ext_control case, it's setting the mode which
really isn't what this is supposed to do and collides with any actual
configuration of the mode that might happen...
I though following case:+ /*...and I'd really expect something that reverses these changes?
+ * Keep the regulator in OFF state if it needs to be disable
+ * in suspend state.
+ */
+ if (pmic->board_ext_control[id]) {
+ u8 regoffs = (pmic->ext_sleep_control[id]>> 8)& 0xFF;
+ u8 bit_pos = (1<< pmic->ext_sleep_control[id]& 0xFF);
+ int ret;
+ ret = tps65910_clear_bits(mfd,
+ TPS65910_SLEEP_KEEP_LDO_ON + regoffs, bit_pos);
+ if (!ret)
+ ret = tps65910_set_bits(mfd,
+ TPS65910_SLEEP_SET_LDO_OFF + regoffs, bit_pos);
+ if (ret< 0)
+ dev_err(mfd->dev,
+ "Error in configuring SLEEP register\n");
The actual bits setting up the ext_control look OK - can you split thoseYes, this will be easy to review also. I will split the change in two parts, one for external control configuration and other for suspend modes.
off from the bits implementing the suspend mode callbacks please so they
can be applied while the callbacks are reviewed?
* Unknown Key
* 0x6E30FDDD