Messages in this thread | | | Date | Fri, 27 Jan 2012 17:07:58 +0000 | From | Mark Brown <> | Subject | Re: [PATCH V3] regulator: tps65910: Sleep control through external inputs |
| |
On Wed, Jan 25, 2012 at 09:18:30PM +0530, Laxman Dewangan wrote:
> +static int tps65910_set_suspend_enable(struct regulator_dev *dev) > +{ > + 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); > +}
I'm really confuseed now. This definitely looks like it's doing the 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...
> + /* > + * 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");
...and I'd really expect something that reverses these changes?
The actual bits setting up the ext_control look OK - can you split those off from the bits implementing the suspend mode callbacks please so they can be applied while the callbacks are reviewed? [unhandled content-type:application/pgp-signature] | |