Messages in this thread Patch in this message | | | Date | Tue, 11 Dec 2018 16:47:06 +0000 | From | Mark Brown <> | Subject | Re: [RESEND PATCH v3 3/3] regulator: mcp16502: add regulator driver for MCP16502 |
| |
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? [unhandled content-type:application/pgp-signature] | |