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

[Samuel Kayode <samuel.kayode@savoirfairelinux.com>]

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