[RESEND,v3,0/3] add support for MCP16502 PMIC
mbox series

Message ID 1544522876-15967-1-git-send-email-andrei.stefanescu@microchip.com
Headers show
Series
  • add support for MCP16502 PMIC
Related show

Message

Andrei.Stefanescu@microchip.com Dec. 11, 2018, 10:08 a.m. UTC
The resend is because I forgot to add Mark.

MCP16502 is a Power Management IC from Microchip.
It has 4 Buck outputs and 2 LDOs. The buck regulators
can be used in two modes: normal(FPWM) and low-power(Auto PFM).

This patch series adds support for the MCP16502 PMIC.

v3:
- use CONFIG_SUSPEND and CONFIG_PM to fix compile errors for some configs

v2:
- use lpm-gpios instead of lpm-gpio in devicetree bindings documentation
- describe the regulators present on the PMIC in the devicetree bindings
  documentation
- add SPDX license inside a C++ comment
- prefix macro
- remove mcp16502_update_regulator and mcp16502_read
- replace ?: with if-else
- change some if-else with switch statements for legibility
- use regmap helpers for regultor settings during runtime
- make mcp16502_get_status read the status from the PMIC STS registers
- use module_i2c_driver
- use the PMIC's Hibernate registers for suspend-to-mem, the PMIC's
  Low-power registers for standby and the PMIC's Active registers for
  normal runtime

Note about mcp16502_suspend:
- mcp16502_gpio_set_mode(mcp, MCP16502_OPMODE_HIB) has now been changed to
  mcp16502_gpio_set_mode(mcp, MCP16502_OPMODE_LPM) for legibility.

Note that the function call only sets the LPM pin of the PMIC to high.
This puts the PMIC in Low-power operating mode. Hibernate operating mode
is reached when the MPU sets the PWRHLD line to zero (typically when
entering suspend-to-ram).

Andrei Stefanescu (3):
  regulator: dt-bindings: add MCP16502 regulator bindings
  MAINTAINERS: add maintainer for MCP16502 PMIC driver
  regulator: mcp16502: add regulator driver for MCP16502

 .../bindings/regulator/mcp16502-regulator.txt      | 143 ++++++
 MAINTAINERS                                        |   7 +
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/mcp16502.c                       | 555 +++++++++++++++++++++
 5 files changed, 715 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mcp16502-regulator.txt
 create mode 100644 drivers/regulator/mcp16502.c

Comments

Mark Brown Dec. 11, 2018, 4:47 p.m. UTC | #1
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?
Andrei.Stefanescu@microchip.com Dec. 12, 2018, 8:01 a.m. UTC | #2
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
Mark Brown Dec. 12, 2018, 3:55 p.m. UTC | #3
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?
Andrei.Stefanescu@microchip.com Dec. 13, 2018, 12:19 p.m. UTC | #4
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
Mark Brown Dec. 13, 2018, 4:21 p.m. UTC | #5
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?