In this patch, LDO's adjustable and fixed Vout settings are compatible.
The LDO Vout ability depends on the init_data->constraints.
If adjustable, the Vout can be set to either 1800mV or 3300mV.
Signed-off-by: Alina Yu <alina_yu@richtek.com>
---
drivers/regulator/rtq2208-regulator.c | 41 +++++++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 9 deletions(-)
diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 63d4037..80a4d2f 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -220,7 +220,7 @@ static const struct regulator_ops rtq2208_regulator_buck_ops = {
.set_suspend_mode = rtq2208_set_suspend_mode,
};
-static const struct regulator_ops rtq2208_regulator_ldo_ops = {
+static const struct regulator_ops rtq2208_regulator_ldo_fix_ops = {
.enable = regulator_enable_regmap,
.disable = regulator_disable_regmap,
.is_enabled = regulator_is_enabled_regmap,
@@ -229,6 +229,23 @@ static const struct regulator_ops rtq2208_regulator_ldo_ops = {
.set_suspend_disable = rtq2208_set_suspend_disable,
};
+static const struct regulator_ops rtq2208_regulator_ldo_adj_ops = {
+ .enable = regulator_enable_regmap,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .list_voltage = regulator_list_voltage_table,
+ .set_voltage_sel = regulator_set_voltage_sel_regmap,
+ .get_voltage_sel = regulator_get_voltage_sel_regmap,
+ .set_active_discharge = regulator_set_active_discharge_regmap,
+ .set_suspend_enable = rtq2208_set_suspend_enable,
+ .set_suspend_disable = rtq2208_set_suspend_disable,
+};
+
+static const unsigned int rtq2208_ldo_volt_table[] = {
+ 1800000,
+ 3300000,
+};
+
static unsigned int rtq2208_of_map_mode(unsigned int mode)
{
switch (mode) {
@@ -323,12 +340,12 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)
return IRQ_HANDLED;
}
-static int rtq2208_of_get_fixed_voltage(struct device *dev,
+static int rtq2208_of_get_ldo_dvs_ability(struct device *dev,
struct of_regulator_match *rtq2208_ldo_match, int n_fixed)
{
struct device_node *np;
struct of_regulator_match *match;
- struct rtq2208_regulator_desc *rdesc;
+ struct regulator_desc *desc;
struct regulator_init_data *init_data;
int ret, i;
@@ -349,13 +366,20 @@ static int rtq2208_of_get_fixed_voltage(struct device *dev,
for (i = 0; i < n_fixed; i++) {
match = rtq2208_ldo_match + i;
init_data = match->init_data;
- rdesc = (struct rtq2208_regulator_desc *)match->driver_data;
+ desc = (struct regulator_desc *)match->desc;
- if (!init_data || !rdesc)
+ if (!init_data || !desc)
continue;
- if (init_data->constraints.min_uV == init_data->constraints.max_uV)
- rdesc->desc.fixed_uV = init_data->constraints.min_uV;
+ if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+ desc->n_voltages = 1;
+ desc->fixed_uV = init_data->constraints.min_uV;
+ desc->ops = &rtq2208_regulator_ldo_fix_ops;
+ } else {
+ desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
+ desc->volt_table = rtq2208_ldo_volt_table;
+ desc->ops = &rtq2208_regulator_ldo_adj_ops;
+ }
}
return 0;
@@ -482,8 +506,7 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *
rtq2208_ldo_match[idx - RTQ2208_LDO2].desc = &rdesc[i]->desc;
}
- /* init ldo fixed_uV */
- ret = rtq2208_of_get_fixed_voltage(dev, rtq2208_ldo_match, ldo_idx);
+ ret = rtq2208_of_get_ldo_dvs_ability(dev, rtq2208_ldo_match, ldo_idx);
if (ret)
return dev_err_probe(dev, ret, "Failed to get ldo fixed_uV\n");
--
2.7.4On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:
> In this patch, LDO's adjustable and fixed Vout settings are compatible.
> The LDO Vout ability depends on the init_data->constraints.
> If adjustable, the Vout can be set to either 1800mV or 3300mV.
> + if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
> + desc->n_voltages = 1;
> + desc->fixed_uV = init_data->constraints.min_uV;
> + desc->ops = &rtq2208_regulator_ldo_fix_ops;
> + } else {
> + desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
> + desc->volt_table = rtq2208_ldo_volt_table;
> + desc->ops = &rtq2208_regulator_ldo_adj_ops;
> + }
Why are you making this change? The operations supported by the
regulator don't change depending on if the system is going to chnage the
voltage.
On Wed, May 01, 2024 at 11:19:05AM +0900, Mark Brown wrote:
> On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:
>
> > In this patch, LDO's adjustable and fixed Vout settings are compatible.
> > The LDO Vout ability depends on the init_data->constraints.
> > If adjustable, the Vout can be set to either 1800mV or 3300mV.
>
> > + if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
> > + desc->n_voltages = 1;
> > + desc->fixed_uV = init_data->constraints.min_uV;
> > + desc->ops = &rtq2208_regulator_ldo_fix_ops;
> > + } else {
> > + desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
> > + desc->volt_table = rtq2208_ldo_volt_table;
> > + desc->ops = &rtq2208_regulator_ldo_adj_ops;
> > + }
>
> Why are you making this change? The operations supported by the
> regulator don't change depending on if the system is going to chnage the
> voltage.
The change is necessary due to the requirement of the SD card for high/default and ultra-high-speed modes. The system needs to adjust the LDO Vout accordingly.
In ultra-high-speed mode, the LDO Vout needs to be adjusted to 1.8V; otherwise, it will remain at 1.8V.
Thanks,
Alina
On Thu, May 02, 2024 at 03:30:29PM +0800, Alina Yu wrote:
> On Wed, May 01, 2024 at 11:19:05AM +0900, Mark Brown wrote:
> > On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:
> > > + if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
> > > + desc->n_voltages = 1;
> > > + desc->fixed_uV = init_data->constraints.min_uV;
> > > + desc->ops = &rtq2208_regulator_ldo_fix_ops;
> > > + } else {
> > > + desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
> > > + desc->volt_table = rtq2208_ldo_volt_table;
> > > + desc->ops = &rtq2208_regulator_ldo_adj_ops;
> > > + }
> > Why are you making this change? The operations supported by the
> > regulator don't change depending on if the system is going to chnage the
> > voltage.
> The change is necessary due to the requirement of the SD card for high/default and ultra-high-speed modes. The system needs to adjust the LDO Vout accordingly.
> In ultra-high-speed mode, the LDO Vout needs to be adjusted to 1.8V; otherwise, it will remain at 1.8V.
That just sounds like normal constraints usage, and something that
applies to a wide variety of systems not just this specific regulator.
If there are limits on what voltages can be configured the constraints
will enforce them.
On Thu, May 02, 2024 at 03:30:29PM +0800, Alina Yu wrote:
> On Wed, May 01, 2024 at 11:19:05AM +0900, Mark Brown wrote:
> > On Tue, Apr 30, 2024 at 05:58:25PM +0800, Alina Yu wrote:
> >
> > > In this patch, LDO's adjustable and fixed Vout settings are compatible.
> > > The LDO Vout ability depends on the init_data->constraints.
> > > If adjustable, the Vout can be set to either 1800mV or 3300mV.
> >
> > > + if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
> > > + desc->n_voltages = 1;
> > > + desc->fixed_uV = init_data->constraints.min_uV;
> > > + desc->ops = &rtq2208_regulator_ldo_fix_ops;
> > > + } else {
> > > + desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
> > > + desc->volt_table = rtq2208_ldo_volt_table;
> > > + desc->ops = &rtq2208_regulator_ldo_adj_ops;
> > > + }
> >
> > Why are you making this change? The operations supported by the
> > regulator don't change depending on if the system is going to chnage the
> > voltage.
>
> The change is necessary due to the requirement of the SD card for high/default and ultra-high-speed modes. The system needs to adjust the LDO Vout accordingly.
> In ultra-high-speed mode, the LDO Vout needs to be adjusted to 1.8V; otherwise, it will remain at 1.8V.
>
>
Apologies for the misunderstanding. Let me provide further clarification regarding the LDO Vout.
For the fixed LDO Vout, it will be set to either 0.9V or 1.2V, which are outside the range of 1.8V to 3.3V.
The determination of whether it is fixed or adjustable lies solely with the user.
This modification aims to ensure compatibility with the user's application.
Thanks,
Alina
>
>
On Thu, May 02, 2024 at 05:26:14PM +0800, Alina Yu wrote: > For the fixed LDO Vout, it will be set to either 0.9V or 1.2V, which are outside the range of 1.8V to 3.3V. > The determination of whether it is fixed or adjustable lies solely with the user. > This modification aims to ensure compatibility with the user's application. That's a substantail reconfiguration of the regulator, it would be better to have an explicit property for these non-standard fixed voltages rather than trying to do this using constraints, or at the very least have validation that the values being set are supported by the hardware. The code should also be very clear about what is going on.
On Fri, May 03, 2024 at 10:41:04AM +0900, Mark Brown wrote:
> On Thu, May 02, 2024 at 05:26:14PM +0800, Alina Yu wrote:
>
> > For the fixed LDO Vout, it will be set to either 0.9V or 1.2V, which are outside the range of 1.8V to 3.3V.
> > The determination of whether it is fixed or adjustable lies solely with the user.
> > This modification aims to ensure compatibility with the user's application.
>
> That's a substantail reconfiguration of the regulator, it would be
> better to have an explicit property for these non-standard fixed
> voltages rather than trying to do this using constraints, or at the very
> least have validation that the values being set are supported by the
> hardware. The code should also be very clear about what is going on.
May I add the 'richtek,use-fix-dvs' property back ?
which is the proerty I add in v1 patch series to let the user determines whether the LDO Vout is fixed or not.
+ fixed_uV = of_property_read_bool(match->of_node, "richtek,use-fix-dvs");
+
+ if (fixed_uV) {
+ desc->n_voltages = 1;
+ desc->fixed_uV = init_data->constraints.min_uV;
+ desc->ops = &rtq2208_regulator_ldo_fix_ops;
+ } else {
+ desc->n_voltages = ARRAY_SIZE(rtq2208_ldo_volt_table);
+ desc->volt_table = rtq2208_ldo_volt_table;
+ desc->ops = &rtq2208_regulator_ldo_adj_ops;
+ }
Thanks,
Alina
On Fri, May 03, 2024 at 03:35:36PM +0800, Alina Yu wrote: > On Fri, May 03, 2024 at 10:41:04AM +0900, Mark Brown wrote: > > That's a substantail reconfiguration of the regulator, it would be > > better to have an explicit property for these non-standard fixed > > voltages rather than trying to do this using constraints, or at the very > > least have validation that the values being set are supported by the > > hardware. The code should also be very clear about what is going on. > May I add the 'richtek,use-fix-dvs' property back ? It sounds like it might be better to add a property specifying the specific fixed voltage rather than overloading the constraints for this purpose.
On Mon, May 06, 2024 at 11:58:29PM +0900, Mark Brown wrote:
> On Fri, May 03, 2024 at 03:35:36PM +0800, Alina Yu wrote:
> > On Fri, May 03, 2024 at 10:41:04AM +0900, Mark Brown wrote:
>
> > > That's a substantail reconfiguration of the regulator, it would be
> > > better to have an explicit property for these non-standard fixed
> > > voltages rather than trying to do this using constraints, or at the very
> > > least have validation that the values being set are supported by the
> > > hardware. The code should also be very clear about what is going on.
>
> > May I add the 'richtek,use-fix-dvs' property back ?
>
> It sounds like it might be better to add a property specifying the
> specific fixed voltage rather than overloading the constraints for this
> purpose.
May I modify the code into this ?
I'll add 'richtek,fixed-microvolt' property in dtsi; remove 'regulator-min-microvolt' and 'regulator-max-microvolt'
to prevent fail caused by constraints->apply_uV.
+ u32 fixed_uV;
int ret, i;
- if (init_data->constraints.min_uV == init_data->constraints.max_uV) {
+ if (!of_property_read_u32(match->of_node, "richtek,fixed-microvolt", &fixed_uV)) {
desc->n_voltages = 1;
- desc->fixed_uV = init_data->constraints.min_uV;
+ desc->fixed_uV = fixed_uV;
desc->ops = &rtq2208_regulator_ldo_fix_ops;
...
Thanks,
Alina
On Wed, May 08, 2024 at 02:54:02PM +0800, Alina Yu wrote: > May I modify the code into this ? > I'll add 'richtek,fixed-microvolt' property in dtsi; remove 'regulator-min-microvolt' and 'regulator-max-microvolt' > to prevent fail caused by constraints->apply_uV. Adding the new property seems fine. You still need to permit the min/max microvolt properties for the case where the regulator is in normal mode and can vary, you could write rules that ensure that the constraints line up in the case where a fixed voltage is specified but I'm not sure it's worth the effort.
On Wed, May 08, 2024 at 08:53:34PM +0900, Mark Brown wrote:
> On Wed, May 08, 2024 at 02:54:02PM +0800, Alina Yu wrote:
>
> > May I modify the code into this ?
> > I'll add 'richtek,fixed-microvolt' property in dtsi; remove 'regulator-min-microvolt' and 'regulator-max-microvolt'
> > to prevent fail caused by constraints->apply_uV.
>
> Adding the new property seems fine. You still need to permit the
> min/max microvolt properties for the case where the regulator is in
> normal mode and can vary, you could write rules that ensure that the
> constraints line up in the case where a fixed voltage is specified but
> I'm not sure it's worth the effort.
Or may I add the following condition to check the constraints.min_uV and constraints.max_uV match the specified fixed voltage ?
+ u32 fixed_uV;
int ret, i;
...
+ /* specify working fixed voltage if the propery exists */
+ ret = of_property_read_u32(match->of_node, "richtek,fixed-microvolt", &fixed_uV);
+
+ if (!ret) {
+ if (fixed_uV != init_data->constraints.min_uV ||
+ fixed_uV != init_data->constraints.max_uV)
+ return -EINVAL;
+
desc->n_voltages = 1;
+ desc->fixed_uV = fixed_uV;
...
Thanks,
Alina
On Thu, May 09, 2024 at 05:15:03PM +0800, Alina Yu wrote: > Or may I add the following condition to check the constraints.min_uV and constraints.max_uV match the specified fixed voltage ? That seems like a reasonable check, though I think we should just do that in the core any time we have a fixed voltage regulator rather than doing it in a single driver.
On Thu, May 09, 2024 at 05:40:19PM +0200, Mark Brown wrote: > On Thu, May 09, 2024 at 05:15:03PM +0800, Alina Yu wrote: > > > Or may I add the following condition to check the constraints.min_uV and constraints.max_uV match the specified fixed voltage ? > > That seems like a reasonable check, though I think we should just do > that in the core any time we have a fixed voltage regulator rather than > doing it in a single driver. Hi, Mark Thank you so much for your review. I have compiled all of your suggestions and implemented the modifications for the v3 series. Best regards, Alina
© 2016 - 2025 Red Hat, Inc.