linux-kernel - Re: [PATCH v8 3/6] regulator: pf1550: add support for regulator

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: <aG_x8VELlUvLxezY@fedora>
Date: Thu, 10 Jul 2025 13:01:37 -0400
From: Samuel Kayode <samuel.kayode@...oirfairelinux.com>
To: Sean Nyekjaer <sean@...nix.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Sebastian Reichel <sre@...nel.org>, Frank Li <Frank.li@....com>,
	imx@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	linux-pm@...r.kernel.org, Abel Vesa <abelvesa@...nel.org>,
	Abel Vesa <abelvesa@...ux.com>, Robin Gong <b38343@...escale.com>,
	Robin Gong <yibin.gong@....com>,
	Enric Balletbo i Serra <eballetbo@...il.com>
Subject: Re: [PATCH v8 3/6] regulator: pf1550: add support for regulator

On Thu, Jul 10, 2025 at 02:49:21PM +0000, Sean Nyekjaer wrote:
> > +#define PF_SW1(_chip, match, _name, mask, voltages)	{	\
> > +	.desc = {	\
> > +		.name = #_name,	\
> > +		.of_match = of_match_ptr(match),	\
> > +		.regulators_node = of_match_ptr("regulators"),	\
> > +		.n_voltages = ARRAY_SIZE(voltages),	\
> > +		.ops = &pf1550_sw1_ops,	\
> > +		.type = REGULATOR_VOLTAGE,	\
> > +		.id = _chip ## _ ## _name,	\
> > +		.owner = THIS_MODULE,	\
> > +		.volt_table = voltages,	\
> > +		.vsel_reg = _chip ## _PMIC_REG_ ## _name ## _VOLT, \
> > +		.vsel_mask = (mask),	\
> > +	},	\
> > +	.stby_reg = _chip ## _PMIC_REG_ ## _name ## _STBY_VOLT,	\
> > +	.stby_mask = (mask),	\
> > +}
> 
> This is unused.
>
If checking of the DVS status for the SW1 regulator is added as you requested.
This would prove beneficial because it is the preferred method when DVS is
disabled for the SW1. This is the case for the default variant, A1, of the
PMIC.
> > +
> > +#define PF_SW3(_chip, match, _name, min, max, mask, step)	{	\
> 
> [...]
> 
> > +
> > +static struct pf1550_desc pf1550_regulators[] = {
> > +	PF_SW3(PF1550, "sw1", SW1, 600000, 1387500, 0x3f, 12500),
> > +	PF_SW3(PF1550, "sw2", SW2, 600000, 1387500, 0x3f, 12500),
> > +	PF_SW3(PF1550, "sw3", SW3, 1800000, 3300000, 0xf, 100000),
> 
> Seems weird they all use the PF_SW3 macro.
> 
The PF_SW3 macro is very generic. It is the preferred macro when a step has to
be provided which is the case for SW1 & SW2 with DVS enabled. The default
variant, A1, has SW2 enabled.
> > +	PF_VREF(PF1550, "vrefddr", VREFDDR, 1200000),
> > +	PF_LDO1(PF1550, "ldo1", LDO1, 0x1f, pf1550_ldo13_volts),
> > +	PF_LDO2(PF1550, "ldo2", LDO2, 0xf, 1800000, 3300000, 100000),
> > +	PF_LDO1(PF1550, "ldo3", LDO3, 0x1f, pf1550_ldo13_volts),
> > +};
> > +
> 
> [...]
> 
> > +
> > +static int pf1550_regulator_probe(struct platform_device *pdev)
> > +{
> > +	const struct pf1550_ddata *pf1550 = dev_get_drvdata(pdev->dev.parent);
> > +	struct regulator_config config = { };
> > +	struct pf1550_regulator_info *info;
> > +	int i, irq = -1, ret = 0;
> > +
> > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info)
> > +		return -ENOMEM;
> > +
> > +	config.regmap = dev_get_regmap(pf1550->dev, NULL);
> > +	if (!config.regmap)
> > +		return dev_err_probe(&pdev->dev, -ENODEV,
> > +				     "failed to get parent regmap\n");
> > +
> > +	config.dev = pf1550->dev;
> > +	config.regmap = pf1550->regmap;
> > +	info->dev = &pdev->dev;
> > +	info->pf1550 = pf1550;
> > +
> > +	memcpy(info->regulator_descs, pf1550_regulators,
> > +	       sizeof(info->regulator_descs));
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pf1550_regulators); i++) {
> > +		struct regulator_desc *desc;
> > +
> > +		desc = &info->regulator_descs[i].desc;
> > +
> > +		if (desc->id == PF1550_SW2 && pf1550->dvs_enb) {
> 
> We should enter here if dvs_enb == false.
> My A6 variant reported 0.625V instead of the correct 1.35V
> 
Yeah, that would happen with the current if statement.

Since dvs_enb is true when DVS is enabled (OTP_SW2_DVS_ENB == 0), I should
modify the if statment to:
(desc->id == PF1550_SW2 && !pf1550->dvs_enb) /* OTP_SW2_DVS_ENB == 1 */

I think that would be a more readable solution.
> > +			/* OTP_SW2_DVS_ENB == 1? */
> > +			desc->volt_table = pf1550_sw12_volts;
> > +			desc->n_voltages = ARRAY_SIZE(pf1550_sw12_volts);
> > +			desc->ops = &pf1550_sw1_ops;
> > +		}
> >

Thanks,
Sam