On Tue, Dec 11, 2018 at 10:09:15AM +0000, Andrei.Stefanescu@xxxxxxxxxxxxx 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?
Attachment:
signature.asc
Description: PGP signature
