On Mon, 20 May 2024 at 15:18, Andre Przywara <andre.przyw...@arm.com> wrote:
>
> On Mon, 20 May 2024 13:48:41 +0100
> Peter Robinson <pbrobin...@gmail.com> wrote:
>
> Hi Peter,
>
> thanks for having a look!
>
> > Hi Andre,
> >
> > > this is the first series in an attempt to clean up the X-Powers AXP PMIC
> > > drivers used by the SPL for sunxi boards. So far we have a separate
> > > driver file for each AXP variant, but the code was largely the same,
> > > just differing by the regulator ranges.
> > >
> > > This adds a new generic driver, which reads the regulator description
> > > from an array of structs. This is similar to how the DM AXP driver used
> > > for U-Boot proper works, but is simplified, since we won't need the full
> > > feature set for the SPL, and we want to keep the code size small.
> >
> > Overall seems a reasonable approach, would it make sense to put the
> > regulator descriptions in a file that could be shared between this and
> > the DM driver so the information isn't duplicated and hence may
> > diverge over time with things like copy/paste errors?
>
> Yes, I thought about it, but it gets nasty:
> - The SPL should only have the regulator descriptions for the *one*
> selected AXP chip, whereas the DM driver can afford to describe all
> regulators and select the right ones via the compatible at runtime. This
> is to conserve memory for the SPL.
> - The SPL should only be interested in the DC/DC buck converters, we don't
> really need any LDOs. Again saves memory.
> - Each regulator entry for the SPL should be as small as possible, we don't
> want and need the regulator name, for instance, or the table pointer.
> Again to save memory.
>
> Currently the array for the three AXP313 regulators in the SPL is exactly
> 30 bytes long; in the DM driver, just for the small AXP313: 192 bytes. The
> difference is even more pronounced for the larger AXPs.
>
> I am hopeful there might be some macro magic to express this, like:
> static const struct axp_regulator_plat axp313_regulators[] = {
> SPL( "dcdc1", 0x10, BIT(0), 0x13, 0x7f, 500, 1540, 10, 70 )
> SPL( "dcdc2", 0x10, BIT(1), 0x14, 0x7f, 500, 1540, 10, 70 )
> SPL( "dcdc3", 0x10, BIT(2), 0x15, 0x7f, 500, 1840, 10, 70 )
> PROPER( "aldo1", 0x10, BIT(3), 0x16, 0x1f, 500, 3500, 100, NA )
> PROPER( "dldo1", 0x10, BIT(4), 0x17, 0x1f, 500, 3500, 100, NA )
> {}
> };
> But we still need something to avoid the array altogether when another AXP
> is selected, which might require #ifdefs.
> So it might be possible, but the devil is probably in the details.
>
> For now I just wanted to avoid adding more AXP driver files to
> the drivers/power directory, so I consider this a future optimisation.
That all makes sense, thanks for the great explanation!
> Happy to take patches :-D
>
> Cheers,
> Andre
>
> > Peter
> >
> > > To help bisect-ability, and to simplify review, each of the simpler AXP
> > > drivers covered by this approach is handled in a separate patch.
> > > We just convert the AXP313, AXP305, AXP717 for now, and on the way add
> > > support for the AXP707, just because it's now very easy, and we will
> > > need it soon enough.
> > > The other AXP SPL drivers are more complex, and support more regulators,
> > > but my hunch is that we really just need the DC/DC converters in the
> > > SPL. However I need to prove and test this, so I will convert the other
> > > AXP chips later.
> > >
> > > Please have a look and comment whether the approach in general is a good
> > > idea.
> > >
> > > Cheers,
> > > Andre
> > >
> > > Andre Przywara (6):
> > > power: pmic: sunxi: only build AXP drivers for SPL
> > > power: pmic: sunxi: introduce generic SPL AXP DC/DC driver
> > > power: pmic: sunxi: replace AXP717 SPL driver
> > > power: pmic: sunxi: use generic AXP SPL driver for AXP313
> > > power: pmic: sunxi: use generic AXP SPL driver for AXP305
> > > power: pmic: sunxi: add AXP707 support
> > >
> > > drivers/power/Makefile | 8 +-
> > > drivers/power/axp305.c | 82 ------------------
> > > drivers/power/axp313.c | 133 -----------------------------
> > > drivers/power/axp717.c | 92 --------------------
> > > drivers/power/axp_spl.c | 184 ++++++++++++++++++++++++++++++++++++++++
> > > 5 files changed, 189 insertions(+), 310 deletions(-)
> > > delete mode 100644 drivers/power/axp305.c
> > > delete mode 100644 drivers/power/axp313.c
> > > delete mode 100644 drivers/power/axp717.c
> > > create mode 100644 drivers/power/axp_spl.c
> > >
> > > --
> > > 2.35.8
> > >
>
