On Thu, Jul 10, 2025 at 01:01:37PM +0100, Samuel Kayode wrote:
> 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 */
- if (desc->id == PF1550_SW2 && pf1550->dvs_enb) {
+ if (desc->id == PF1550_SW2 && !pf1550->dvs_enb) {
Yes that's what I have been running my tests with :)
I will continue with testing the onkey and battery charger
>
> 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
/Sean
