| Message ID | 1544522876-15967-1-git-send-email-andrei.stefanescu@microchip.com |
|---|---|
| Headers | show |
| Series | add support for MCP16502 PMIC | expand |
On Tue, Dec 11, 2018 at 10:09:15AM +0000, Andrei.Stefanescu@microchip.com wrote: > This patch adds a regulator driver for the MCP16502 PMIC. > This drivers supports basic operations through the > regulator interface such as: Overall this looks really good, just a couple of comments: > +++ b/drivers/regulator/mcp16502.c > @@ -0,0 +1,555 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * MCP16502 PMIC driver SPDX headers need to be C++ comments - please make the entire comment block a C++ one so it looks more intentional. > +#ifdef CONFIG_SUSPEND > +static int mcp16502_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct mcp16502 *mcp = i2c_get_clientdata(client); > + > + mcp16502_gpio_set_mode(mcp, MCP16502_OPMODE_LPM); > + > + return 0; > +} This puts the device into low power mode when the suspend function gets called but this might not be safe - devices using the regulator may not be suspended yet so could still need full regulation. Normally a GPIO triggered transition like this would be being done by hardware as part of the process of suspending the SoC. Is there some reason to do this manually?
Hi Mark, Thank you for the review. > SPDX headers need to be C++ comments - please make the entire comment > block a C++ one so it looks more intentional. I sent a new patch (v4) with the modified comment. >> +#ifdef CONFIG_SUSPEND >> +static int mcp16502_suspend(struct device *dev) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct mcp16502 *mcp = i2c_get_clientdata(client); >> + >> + mcp16502_gpio_set_mode(mcp, MCP16502_OPMODE_LPM); >> + >> + return 0; >> +} > This puts the device into low power mode when the suspend function gets > called but this might not be safe - devices using the regulator may not > be suspended yet so could still need full regulation. Normally a GPIO > triggered transition like this would be being done by hardware as part > of the process of suspending the SoC. Is there some reason to do this > manually? There is a line from the MPU (SHDN) which goes low only when the MPU turns off. That line is already connected to the PMIC and it differentiates between suspend-to-mem and standby. To switch to low-power, the PMIC must be controlled by the GPIO pin LPM. The suspend sequence is: - LPM pin goes high (PMIC enters Low-Power <-> Linux standby) - SHDN goes low (if target suspend state is mem) and then PMIC enters HIBERNATE Best regards, Andrei
On Wed, Dec 12, 2018 at 08:01:07AM +0000, Andrei.Stefanescu@microchip.com wrote: > > This puts the device into low power mode when the suspend function gets > > called but this might not be safe - devices using the regulator may not > > be suspended yet so could still need full regulation. Normally a GPIO > > triggered transition like this would be being done by hardware as part > > of the process of suspending the SoC. Is there some reason to do this > > manually? > There is a line from the MPU (SHDN) which goes low only when the MPU > turns off. That line is already connected to the PMIC and it differentiates > between suspend-to-mem and standby. To switch to low-power, the PMIC must > be controlled by the GPIO pin LPM. > The suspend sequence is: > - LPM pin goes high (PMIC enters Low-Power <-> Linux standby) > - SHDN goes low (if target suspend state is mem) and then PMIC enters > HIBERNATE This feels like it should be being controlled somewhere else, if it's actually causing a change in the PMIC state it seems like it wants to be done as late as possible in suspend to minimize the risks. At the very least suspend_late() for the driver seems appropriate. Could you submit a version with this feature at least split out into a separate patch please so we can apply the rest of the code while this is discussed?
Hi Mark, On 12.12.2018 17:55, Mark Brown wrote: > On Wed, Dec 12, 2018 at 08:01:07AM +0000, Andrei.Stefanescu@microchip.com wrote: > >>> This puts the device into low power mode when the suspend function gets >>> called but this might not be safe - devices using the regulator may not >>> be suspended yet so could still need full regulation. Normally a GPIO >>> triggered transition like this would be being done by hardware as part >>> of the process of suspending the SoC. Is there some reason to do this >>> manually? >> There is a line from the MPU (SHDN) which goes low only when the MPU >> turns off. That line is already connected to the PMIC and it differentiates >> between suspend-to-mem and standby. To switch to low-power, the PMIC must >> be controlled by the GPIO pin LPM. >> The suspend sequence is: >> - LPM pin goes high (PMIC enters Low-Power <-> Linux standby) >> - SHDN goes low (if target suspend state is mem) and then PMIC enters >> HIBERNATE > This feels like it should be being controlled somewhere else, if it's > actually causing a change in the PMIC state it seems like it wants to be > done as late as possible in suspend to minimize the risks. At the very > least suspend_late() for the driver seems appropriate. I tried with both suspend_late/resume_early and suspend_noirq/resume_noirq. Everything seems to work ok. Which one do you think is more appropriate? I am guessing that during suspend_late some devices may still need to be turned on and that it would be the safest during suspend_noirq. Note: the GPIO pin used for LPM is a special one (it maintains its voltage during suspend-to-mem). It is part of the SoC and its methods for setting the values do not sleep. > > Could you submit a version with this feature at least split out into a > separate patch please so we can apply the rest of the code while this is > discussed? Sent one patch series and another patch with the current suspend/resume implementation. Best regards, Andrei
On Thu, Dec 13, 2018 at 12:19:16PM +0000, Andrei.Stefanescu@microchip.com wrote: > I tried with both suspend_late/resume_early and suspend_noirq/resume_noirq. > Everything seems to work ok. > Which one do you think is more appropriate? I am guessing that during > suspend_late some devices may still need to be turned on and that it would > be the safest during suspend_noirq. I agree, noirq is going to be the safest option. Can you respin with that please?