Re: [PATCH] regulator: axp20x: AXP717: set ramp_delay

[Philippe Simons]

On Sat, Dec 7, 2024 at 5:37 AM Chen-Yu Tsai <wens@xxxxxxxx> wrote:
>
> On Fri, Dec 6, 2024 at 8:38 PM Philippe Simons
> <simons.philippe@xxxxxxxxx> wrote:
> >
> > AXP717 datasheet says that regulator ramp delay is 15.625 us/step,
> > which is 10mV in our case.
>
> Thanks! It looks like the reason the delay is needed is because the
> voltage ramp control feature is on by default on this chip.
>
> That is not entirely correct. 10mV is the smallest step forDCDC1,
> DCDC2, and DCDC3, so it makes sense to calculate the ramp delay
> using that.
>
> For DCDC4, the step is 100mV, so the delay would be different.
>
> > Add a AXP_DESC_RANGES_DELAY macro and update AXP_DESC_RANGES macro to
> > expand to AXP_DESC_RANGES_DELAY with ramp_delay = 0
> >
> > Signed-off-by: Philippe Simons <simons.philippe@xxxxxxxxx>
>
> The ramp rate / delay is actually configurable between 15.625 us/step
> and 31.250 us/step. However the setting shared among all DCDCs that
> support this function and have it turned on. I'm not sure how that
> should be modeled if we want to make it configurable.

there is axp20x_set_ramp_delay() ops, but it seems to only handle
axp209 DCDC2 and LDO3

>
> Also looking at older AXP PMICs:
>
> - AXP803: ramp rate 2.5mV / us, on by default, has register bit that can
>           polled to check for ramp completion
> - AXP813: same as AXP803
>
> - AXP805: Similar to AXP717, where rate is given in time / step, the
>           options are the same two and step is different for different
>           buck regulators; difference is not all buck regulators support
>           this feature, and also the rate is individually configurable
>           for each one. Off by default.
>
> - AXP223: 1.6mV/us or 0.8mV/us; off by default; only two buck regulators
>           support it; individually configurable.
>
> > ---
> >  drivers/regulator/axp20x-regulator.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > index a8e91d9d0..8f035db13 100644
> > --- a/drivers/regulator/axp20x-regulator.c
> > +++ b/drivers/regulator/axp20x-regulator.c
> > @@ -419,8 +419,8 @@
> >                 .ops            = &axp20x_ops_fixed                             \
> >         }
> >
> > -#define AXP_DESC_RANGES(_family, _id, _match, _supply, _ranges, _n_voltages,   \
> > -                       _vreg, _vmask, _ereg, _emask)                           \
> > +#define AXP_DESC_RANGES_DELAY(_family, _id, _match, _supply, _ranges, _n_voltages,     \
> > +                       _vreg, _vmask, _ereg, _emask, _ramp_delay)      \
> >         [_family##_##_id] = {                                                   \
> >                 .name           = (_match),                                     \
> >                 .supply_name    = (_supply),                                    \
> > @@ -437,8 +437,15 @@
> >                 .linear_ranges  = (_ranges),                                    \
> >                 .n_linear_ranges = ARRAY_SIZE(_ranges),                         \
> >                 .ops            = &axp20x_ops_range,                            \
> > +               .ramp_delay = (_ramp_delay),                    \
> >         }
> >
> > +#define AXP_DESC_RANGES(_family, _id, _match, _supply, _ranges, _n_voltages,   \
> > +                       _vreg, _vmask, _ereg, _emask)                           \
> > +       AXP_DESC_RANGES_DELAY(_family, _id, _match, _supply, _ranges, _n_voltages,      \
> > +               _vreg, _vmask, _ereg, _emask, 0)
> > +
> > +
> >  static const int axp209_dcdc2_ldo3_slew_rates[] = {
> >         1600,
> >          800,
> > @@ -781,18 +788,18 @@ static const struct linear_range axp717_dcdc3_ranges[] = {
> >  };
> >
> >  static const struct regulator_desc axp717_regulators[] = {
> > -       AXP_DESC_RANGES(AXP717, DCDC1, "dcdc1", "vin1",
> > +       AXP_DESC_RANGES_DELAY(AXP717, DCDC1, "dcdc1", "vin1",
> >                         axp717_dcdc1_ranges, AXP717_DCDC1_NUM_VOLTAGES,
> >                         AXP717_DCDC1_CONTROL, AXP717_DCDC_V_OUT_MASK,
> > -                       AXP717_DCDC_OUTPUT_CONTROL, BIT(0)),
> > -       AXP_DESC_RANGES(AXP717, DCDC2, "dcdc2", "vin2",
> > +                       AXP717_DCDC_OUTPUT_CONTROL, BIT(0), 640),
> > +       AXP_DESC_RANGES_DELAY(AXP717, DCDC2, "dcdc2", "vin2",
> >                         axp717_dcdc2_ranges, AXP717_DCDC2_NUM_VOLTAGES,
> >                         AXP717_DCDC2_CONTROL, AXP717_DCDC_V_OUT_MASK,
> > -                       AXP717_DCDC_OUTPUT_CONTROL, BIT(1)),
> > -       AXP_DESC_RANGES(AXP717, DCDC3, "dcdc3", "vin3",
> > +                       AXP717_DCDC_OUTPUT_CONTROL, BIT(1), 640),
> > +       AXP_DESC_RANGES_DELAY(AXP717, DCDC3, "dcdc3", "vin3",
> >                         axp717_dcdc3_ranges, AXP717_DCDC3_NUM_VOLTAGES,
> >                         AXP717_DCDC3_CONTROL, AXP717_DCDC_V_OUT_MASK,
> > -                       AXP717_DCDC_OUTPUT_CONTROL, BIT(2)),
> > +                       AXP717_DCDC_OUTPUT_CONTROL, BIT(2), 640),
> >         AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
>
> Can you also add it to DCDC4 for completeness?

I'll do in V2

>
>
> Thanks
> ChenYu
>
>
> >                  AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
> >                  AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
> > --
> > 2.47.1
> >