Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 Aug 2020 12:53:04 +0200
From:   Marco Felsch <m.felsch@pengutronix.de>
To:     Robin Gong <yibin.gong@nxp.com>
Cc:     robh+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
        festevam@gmail.com, lgirdwood@gmail.com, broonie@kernel.org,
        Anson.Huang@nxp.com, linux-arm-kernel@lists.infradead.org,
        devicetree@vger.kernel.org, linux-imx@nxp.com,
        kernel@pengutronix.de, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks
Message-ID: <20200831105304.6qcpjfmranay6q4y@pengutronix.de>
References: <1598892515-30950-1-git-send-email-yibin.gong@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1598892515-30950-1-git-send-email-yibin.gong@nxp.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Robin,

On 20-09-01 00:48, Robin Gong wrote:
> BuckX enable mode
> 00b = OFF
> 01b = ON by PMIC_ON_REQ = H
> 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L
> 11b = Always ON
> 
> For such enable mode, enable_value should be clearly set in requlator desc,
> 00/11 is not enough, correct it now for different bucks. For example, buck2
> is designed for vddarm which could be off in 'PMIC_STBY_REQ = H' after kernel
> enter suspend, so should be set '10b' as ON, while others is '01b' as ON.
> All are the same as the default setting which means bucks no need to be
> enabled again during kernel boot even if they have been enabled already after
> pmic on.

I wouldn't hard-code the regulator behaviour because the behaviour comes
from the system design which in most cases are comming from our hw-guys.
Till now I saw a few intelligent designs don't following the pmic user
recommendations to save money. I would love to specify the regulator
behaviour/mode within the dt or acpi.

> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
>  drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
> index eb5822b..79f2a5a 100644
> --- a/drivers/regulator/pca9450-regulator.c
> +++ b/drivers/regulator/pca9450-regulator.c
> @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK1OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK1CTRL,
>  			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
>  			.vsel_mask = BUCK2OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> -			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_mask = BUCK2_ENMODE_MASK,

Unrelated change?

> +			.enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -299,6 +301,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK3OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK3CTRL,
>  			.enable_mask = BUCK3_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -324,6 +327,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK4OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK4CTRL,
>  			.enable_mask = BUCK4_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -342,6 +346,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK5OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK5CTRL,
>  			.enable_mask = BUCK5_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -360,6 +365,7 @@ static const struct pca9450_regulator_desc pca9450a_regulators[] = {
>  			.vsel_mask = BUCK6OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK6CTRL,
>  			.enable_mask = BUCK6_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -475,6 +481,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK1OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK1CTRL,
>  			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -499,7 +506,8 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
>  			.vsel_mask = BUCK2OUT_DVS0_MASK,
>  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> -			.enable_mask = BUCK1_ENMODE_MASK,
> +			.enable_mask = BUCK2_ENMODE_MASK,

Unrelated change?

Regards,
  Marco
> +			.enable_val = BUCK_ENMODE_ONREQ_STBYREQ,
>  			.owner = THIS_MODULE,
>  			.of_parse_cb = pca9450_set_dvs_levels,
>  		},
> @@ -525,6 +533,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK4OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK4CTRL,
>  			.enable_mask = BUCK4_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -543,6 +552,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK5OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK5CTRL,
>  			.enable_mask = BUCK5_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> @@ -561,6 +571,7 @@ static const struct pca9450_regulator_desc pca9450bc_regulators[] = {
>  			.vsel_mask = BUCK6OUT_MASK,
>  			.enable_reg = PCA9450_REG_BUCK6CTRL,
>  			.enable_mask = BUCK6_ENMODE_MASK,
> +			.enable_val = BUCK_ENMODE_ONREQ,
>  			.owner = THIS_MODULE,
>  		},
>  	},
> -- 
> 2.7.4
>