linux-kernel - Re: [PATCH v2 3/7] regulator: qcom-labibb: Implement pull-down, softstart, active discharge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YAEfrIR/HZmQLeQM@builder.lan>
Date:   Thu, 14 Jan 2021 22:53:00 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
Cc:     linux-arm-msm@...r.kernel.org, agross@...nel.org,
        lgirdwood@...il.com, broonie@...nel.org, robh+dt@...nel.org,
        sumit.semwal@...aro.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, phone-devel@...r.kernel.org,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org
Subject: Re: [PATCH v2 3/7] regulator: qcom-labibb: Implement pull-down,
 softstart, active discharge

On Wed 13 Jan 13:42 CST 2021, AngeloGioacchino Del Regno wrote:

> Soft start is required to avoid inrush current during LAB ramp-up and
> IBB ramp-down, protecting connected hardware to which we supply voltage.
> 
> Since soft start is configurable on both LAB and IBB regulators, it
> was necessary to add two DT properties, respectively "qcom,soft-start-us"
> to control LAB ramp-up and "qcom,discharge-resistor-kohms" to control
> the discharge resistor for IBB ramp-down, which obviously brought the
> need of implementing a of_parse callback for both regulators.
> 
> Finally, also implement pull-down mode in order to avoid unpredictable
> behavior when the regulators are disabled (random voltage spikes etc).
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...ainline.org>
> ---
>  drivers/regulator/qcom-labibb-regulator.c | 94 +++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/drivers/regulator/qcom-labibb-regulator.c b/drivers/regulator/qcom-labibb-regulator.c
> index d364f54ad294..38ab1eba1c59 100644
> --- a/drivers/regulator/qcom-labibb-regulator.c
> +++ b/drivers/regulator/qcom-labibb-regulator.c
> @@ -29,12 +29,23 @@
>  #define LABIBB_STATUS1_VREG_OK_BIT	BIT(7)
>  #define LABIBB_CONTROL_ENABLE		BIT(7)
>  
> +#define REG_LABIBB_PD_CTL		0x47
> + #define LAB_PD_CTL_MASK		GENMASK(1, 0)
> + #define IBB_PD_CTL_MASK		(BIT(0) | BIT(7))
> + #define LAB_PD_CTL_STRONG_PULL		BIT(0)
> + #define IBB_PD_CTL_HALF_STRENGTH	BIT(0)
> + #define IBB_PD_CTL_EN			BIT(7)
> +
>  #define REG_LABIBB_CURRENT_LIMIT	0x4b
>   #define LAB_CURRENT_LIMIT_MASK		GENMASK(2, 0)
>   #define IBB_CURRENT_LIMIT_MASK		GENMASK(4, 0)
>   #define LAB_CURRENT_LIMIT_OVERRIDE_EN	BIT(3)
>   #define LABIBB_CURRENT_LIMIT_EN	BIT(7)
>  
> +#define REG_IBB_PWRUP_PWRDN_CTL_1	0x58
> + #define IBB_CTL_1_DISCHARGE_EN		BIT(2)
> +
> +#define REG_LABIBB_SOFT_START_CTL	0x5f
>  #define REG_LABIBB_SEC_ACCESS		0xd0
>   #define LABIBB_SEC_UNLOCK_CODE		0xa5
>  
> @@ -60,6 +71,8 @@ struct labibb_regulator {
>  	struct labibb_current_limits	uA_limits;
>  	u16				base;
>  	u8				type;
> +	u8				dischg_sel;
> +	u8				soft_start_sel;
>  };
>  
>  struct labibb_regulator_data {
> @@ -120,6 +133,70 @@ static int qcom_labibb_get_current_limit(struct regulator_dev *rdev)
>  	return (cur_step * lim->uA_step) + lim->uA_min;
>  }
>  
> +static int qcom_labibb_set_soft_start(struct regulator_dev *rdev)
> +{
> +	struct labibb_regulator *vreg = rdev_get_drvdata(rdev);
> +	u32 val = 0;
> +
> +	if (vreg->type == QCOM_IBB_TYPE)
> +		val = vreg->dischg_sel;
> +	else
> +		val = vreg->soft_start_sel;
> +
> +	return regmap_write(rdev->regmap, rdev->desc->soft_start_reg, val);
> +}
> +
> +static int qcom_labibb_get_table_sel(const int *table, int sz, u32 value)
> +{
> +	int i;
> +
> +	for (i = 0; i < sz; i++)
> +		if (table[i] == value)
> +			return i;
> +	return -EINVAL;
> +}
> +
> +/* IBB discharge resistor values in KOhms */
> +static const int dischg_resistor_values[] = { 300, 64, 32, 16 };
> +
> +/* Soft start time in microseconds */
> +static const int soft_start_values[] = { 200, 400, 600, 800 };
> +
> +static int qcom_labibb_of_parse_cb(struct device_node *np,
> +				   const struct regulator_desc *desc,
> +				   struct regulator_config *config)
> +{
> +	struct labibb_regulator *vreg = config->driver_data;
> +	u32 dischg_kohms, soft_start_time;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "qcom,discharge-resistor-kohms",
> +				       &dischg_kohms);
> +	if (ret)
> +		dischg_kohms = 300;

Nit, if you just initialize dischg_kohms to 300 during definition you
can rely on of_property_read_u32() not updating the value on failure...

That said, I think this patch looks good.

Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>

Regards,
Bjorn