Fix rtq2208 BUCK ramp_delay and LDO dvs setting
v2 cover Fix rtq2208 BUCK ramp_delay and LDO dvs setting
Hi,
This series patches is for hardware modification of RTQ2208. ramp_delay range of BUCK is changed.
The maximum ramp up and down range of BUCK are shorten
from 64mVstep/us to 16mVstep/us.
The LDO's Vout is adjustable if the hardware setting allow it,
and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position. In this version, a software bug has been fixed.rtq2208_ldo_match is no longer a local variable.
It prevents invalid memory access when devm_of_regulator_put_matches is called.
Alina Yu (4):
regulator: rtq2208: Fix LDO discharge register and add vsel setting
regulator: rtq2208: Fix LDO to be compatible with both fixed and
adjustable vout
regulator: rtq2208: Fix invalid memory access when
devm_of_regulator_put_matches is called
regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of
16mVstep/us
drivers/regulator/rtq2208-regulator.c | 166 ++++++++++++++++++++++------------ 1 file changed, 106 insertions(+), 60 deletions(-)
Signature · 1 line
2.7.4
PATCH v2 1/4 regulator: rtq2208: Fix LDO discharge register and add vsel setting
The LDO's Vout is adjustable if the hardware setting allows it, and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position.
--- drivers/regulator/rtq2208-regulator.c | 100 +++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 39 deletions(-)
drivers/regulator/rtq2208-regulator.c +61 -39
@@ -26,6 +26,7 @@#define RTQ2208_REG_BUCK_H_CFG0 0xA2#define RTQ2208_REG_LDO1_CFG 0xB1#define RTQ2208_REG_LDO2_CFG 0xC1 +#define RTQ2208_REG_LDO_DVS_CTRL 0xD0/* Mask */ #define RTQ2208_BUCK_NR_MTP_SEL_MASK GENMASK(7, 0)
@@ -40,6 +41,10 @@#define RTQ2208_EN_DIS_MASK BIT(0)#define RTQ2208_BUCK_RAMP_SEL_MASK GENMASK(2, 0)#define RTQ2208_HD_INT_MASK BIT(0) +#define RTQ2208_LDO1_DISCHG_EN_MASK BIT(4) +#define RTQ2208_LDO1_VOSEL_SD_MASK BIT(5) +#define RTQ2208_LDO2_DISCHG_EN_MASK BIT(6) +#define RTQ2208_LDO2_VOSEL_SD_MASK BIT(7)/* Size */ #define RTQ2208_VOUT_MAXNUM 256
@@ -318,23 +323,6 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)return IRQ_HANDLED ; } -#define RTQ2208_REGULATOR_INFO (_name, _base) \ -{ \ - .name = #_name, \ - .base = _base, \ -} -#define BUCK_RG_BASE (_id) RTQ2208_REG_BUCK_##_id##_CFG0 -#define BUCK_RG_SHIFT (_base, _shift) (_base + _shift) -#define LDO_RG_BASE (_id) RTQ2208_REG_LDO##_id##_CFG -#define LDO_RG_SHIFT (_base, _shift) (_base + _shift) -#define VSEL_SHIFT (_sel) (_sel ? 3 : 1) -#define MTP_SEL_MASK (_sel) RTQ2208_BUCK_EN_NR_MTP_SEL##_sel##_MASK - -static const struct linear_range rtq2208_vout_range[] = { -REGULATOR_LINEAR_RANGE (400000 ,0 ,180 ,5000 ), -REGULATOR_LINEAR_RANGE (1310000 ,181 ,255 ,10000 ), -}; -static int rtq2208_of_get_fixed_voltage (struct device *dev,struct of_regulator_match *rtq2208_ldo_match,int n_fixed) {
@@ -373,6 +361,34 @@ static int rtq2208_of_get_fixed_voltage(struct device *dev,return 0 ; } + +#define BUCK_INFO (_name, _id) \ +{ \ + .name = _name, \ + .base = RTQ2208_REG_BUCK_##_id##_CFG0, \ + .enable_reg = BUCK_RG_SHIFT(RTQ2208_REG_BUCK_##_id##_CFG0, 2), \ + .dis_reg = RTQ2208_REG_BUCK_##_id##_CFG0, \ +} + +#define LDO_INFO (_name, _id) \ +{ \ + .name = _name, \ + .base = RTQ2208_REG_LDO##_id##_CFG, \ + .enable_reg = RTQ2208_REG_LDO##_id##_CFG, \ + .dis_mask = RTQ2208_LDO##_id##_DISCHG_EN_MASK, \ + .dis_on = RTQ2208_LDO##_id##_DISCHG_EN_MASK, \ + .vsel_mask = RTQ2208_LDO##_id##_VOSEL_SD_MASK, \ +} + +#define BUCK_RG_SHIFT (_base, _shift) (_base + _shift) +#define VSEL_SHIFT (_sel) (_sel ? 3 : 1) +#define MTP_SEL_MASK (_sel) RTQ2208_BUCK_EN_NR_MTP_SEL##_sel##_MASK + +static const struct linear_range rtq2208_vout_range[] = { +REGULATOR_LINEAR_RANGE (400000 ,0 ,180 ,5000 ), +REGULATOR_LINEAR_RANGE (1310000 ,181 ,255 ,10000 ), +}; +static void rtq2208_init_regulator_desc (struct rtq2208_regulator_desc *rdesc,int mtp_sel,int idx,struct of_regulator_match *rtq2208_ldo_match,int *ldo_idx) {
@@ -380,17 +396,22 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, instatic const struct {char *name;int base; +int enable_reg; +int dis_reg; +int dis_mask; +int dis_on; +int vsel_mask; } regulator_info[] = { -RTQ2208_REGULATOR_INFO (buck-b,BUCK_RG_BASE (B )), -RTQ2208_REGULATOR_INFO (buck-c,BUCK_RG_BASE (C )), -RTQ2208_REGULATOR_INFO (buck-d,BUCK_RG_BASE (D )), -RTQ2208_REGULATOR_INFO (buck-a,BUCK_RG_BASE (A )), -RTQ2208_REGULATOR_INFO (buck-f,BUCK_RG_BASE (F )), -RTQ2208_REGULATOR_INFO (buck-g,BUCK_RG_BASE (G )), -RTQ2208_REGULATOR_INFO (buck-h,BUCK_RG_BASE (H )), -RTQ2208_REGULATOR_INFO (buck-e,BUCK_RG_BASE (E )), -RTQ2208_REGULATOR_INFO (ldo2,LDO_RG_BASE (2 )), -RTQ2208_REGULATOR_INFO (ldo1,LDO_RG_BASE (1 )), +BUCK_INFO ("buck-b" ,B ), +BUCK_INFO ("buck-c" ,C ), +BUCK_INFO ("buck-d" ,D ), +BUCK_INFO ("buck-a" ,A ), +BUCK_INFO ("buck-f" ,F ), +BUCK_INFO ("buck-g" ,G ), +BUCK_INFO ("buck-h" ,H ), +BUCK_INFO ("buck-e" ,E ), +LDO_INFO ("ldo2" ,2 ), +LDO_INFO ("ldo1" ,1 ), }, *curr_info; curr_info = regulator_info + idx;
@@ -402,15 +423,13 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in desc->owner =THIS_MODULE ; desc->type =REGULATOR_VOLTAGE ; desc->enable_mask = mtp_sel ?MTP_SEL_MASK (1 ) :MTP_SEL_MASK (0 ); - desc->active_discharge_on =RTQ2208_EN_DIS_MASK ; + desc->enable_reg = curr_info->enable_reg; desc->active_discharge_off =0 ; - desc->active_discharge_mask =RTQ2208_EN_DIS_MASK ; rdesc->mode_mask =RTQ2208_BUCK_NRMODE_MASK ;if (idx >=RTQ2208_BUCK_B && idx <=RTQ2208_BUCK_E ) {/* init buck desc */ - desc->enable_reg =BUCK_RG_SHIFT (curr_info->base,2 ); desc->ops = &rtq2208_regulator_buck_ops; desc->vsel_reg = curr_info->base +VSEL_SHIFT (mtp_sel); desc->vsel_mask =RTQ2208_BUCK_NR_MTP_SEL_MASK ;
@@ -418,8 +437,10 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in desc->linear_ranges = rtq2208_vout_range; desc->n_linear_ranges =ARRAY_SIZE (rtq2208_vout_range); desc->ramp_reg =BUCK_RG_SHIFT (curr_info->base,5 ); - desc->active_discharge_reg = curr_info->base; desc->of_map_mode = rtq2208_of_map_mode; + desc->active_discharge_reg = curr_info->dis_reg; + desc->active_discharge_on =RTQ2208_EN_DIS_MASK ; + desc->active_discharge_mask =RTQ2208_EN_DIS_MASK ; rdesc->mode_reg =BUCK_RG_SHIFT (curr_info->base,2 ); rdesc->suspend_config_reg =BUCK_RG_SHIFT (curr_info->base,4 );
@@ -427,14 +448,11 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in rdesc->suspend_mode_mask =RTQ2208_BUCK_STRMODE_MASK ; }else {/* init ldo desc */ - desc->enable_reg = curr_info->base; - desc->ops = &rtq2208_regulator_ldo_ops; - desc->n_voltages =1 ; - desc->active_discharge_reg =LDO_RG_SHIFT (curr_info->base,2 ); - - rtq2208_ldo_match[*ldo_idx].name = desc->name; - rtq2208_ldo_match[*ldo_idx].driver_data = rdesc; - rtq2208_ldo_match[(*ldo_idx)++].desc = desc; + desc->active_discharge_reg =RTQ2208_REG_LDO_DVS_CTRL ; + desc->active_discharge_on = curr_info->dis_on; + desc->active_discharge_mask = curr_info->dis_mask; + desc->vsel_reg =RTQ2208_REG_LDO_DVS_CTRL ; + desc->vsel_mask = curr_info->vsel_mask; rdesc->suspend_config_reg = curr_info->base; rdesc->suspend_enable_mask =RTQ2208_LDO_EN_STR_MASK ;
@@ -458,6 +476,10 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *return -ENOMEM ;rtq2208_init_regulator_desc (rdesc[i], mtp_sel, idx, rtq2208_ldo_match, &ldo_idx); + +/* init ldo dvs ability */ +if (idx >=RTQ2208_LDO2 ) + rtq2208_ldo_match[idx -RTQ2208_LDO2 ].desc = &rdesc[i]->desc; }/* init ldo fixed_uV */
Signature · 1 line
2.7.4
PATCH v2 2/4 regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
In reply to Alina YuIn 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.
--- drivers/regulator/rtq2208-regulator.c | 41 +++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-)
drivers/regulator/rtq2208-regulator.c +32 -9
@@ -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" );
Signature · 1 line
2.7.4
PATCH v2 4/4 regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of 16mVstep/us
In reply to Alina YuThe maximum ramp up and down range of BUCK are shorten from 64mVstep/us to 16mVstep/us.
Therefore, the RTQ2208_RAMP_VALUE_MAX_uV is modified to 16000uV in this version.
--- drivers/regulator/rtq2208-regulator.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/regulator/rtq2208-regulator.c +3 -4
@@ -53,7 +53,7 @@/* Value */ #define RTQ2208_RAMP_VALUE_MIN_uV 500 -#define RTQ2208_RAMP_VALUE_MAX_uV 64000 +#define RTQ2208_RAMP_VALUE_MAX_uV 16000#define RTQ2208_BUCK_MASK (uv_irq, ov_irq) (1 << ((uv_irq) % 8) | 1 << ((ov_irq) % 8))
@@ -147,12 +147,11 @@ static int rtq2208_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)* Because the relation of seleltion and value is like that * * seletion: value -* 000: 64mv -* 001: 32mv +* 010: 16mv * ... * 111: 0.5mv * -* For example, if I would like to select 64mv, the fls(ramp_delay) - 1 will be 0b111, +* For example, if I would like to select 16mv, the fls(ramp_delay) - 1 will be 0b010, * and I need to use 0b111 - sel to do the shifting */
Signature · 1 line
2.7.4
PATCH v2 3/4 regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called
In reply to Alina YuIn this patch, a software bug has been fixed. rtq2208_ldo_match is no longer a local variable.
It prevents invalid memory access when devm_of_regulator_put_matches is called.
--- drivers/regulator/rtq2208-regulator.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
drivers/regulator/rtq2208-regulator.c +12 -10
@@ -246,6 +246,11 @@ static const unsigned int rtq2208_ldo_volt_table[] = { 3300000, }; +static struct of_regulator_match rtq2208_ldo_match[] = { + {.name ="ldo2" , }, + {.name ="ldo1" , }, +}; +static unsigned int rtq2208_of_map_mode (unsigned int mode) {switch (mode) {
@@ -340,8 +345,7 @@ static irqreturn_t rtq2208_irq_handler(int irqno, void *devid)return IRQ_HANDLED ; } -static int rtq2208_of_get_ldo_dvs_ability (struct device *dev, -struct of_regulator_match *rtq2208_ldo_match,int n_fixed) +static int rtq2208_of_get_ldo_dvs_ability (struct device *dev) {struct device_node *np;struct of_regulator_match *match;
@@ -356,14 +360,14 @@ static int rtq2208_of_get_ldo_dvs_ability(struct device *dev,if (!np) np = dev->of_node; - ret =of_regulator_match (dev, np, rtq2208_ldo_match, n_fixed); + ret =of_regulator_match (dev, np, rtq2208_ldo_match,ARRAY_SIZE (rtq2208_ldo_match));of_node_put (np);if (ret <0 )return ret; -for (i =0 ; i < n_fixed; i++) { +for (i =0 ; i <ARRAY_SIZE (rtq2208_ldo_match); i++) { match = rtq2208_ldo_match + i; init_data = match->init_data; desc = (struct regulator_desc *)match->desc;
@@ -413,8 +417,7 @@ static const struct linear_range rtq2208_vout_range[] = { REGULATOR_LINEAR_RANGE(1310000, 181, 255, 10000), }; -static void rtq2208_init_regulator_desc (struct rtq2208_regulator_desc *rdesc,int mtp_sel, -int idx,struct of_regulator_match *rtq2208_ldo_match,int *ldo_idx) +static void rtq2208_init_regulator_desc (struct rtq2208_regulator_desc *rdesc,int mtp_sel,int idx) {struct regulator_desc *desc; static const struct {
@@ -486,8 +489,7 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, instatic int rtq2208_parse_regulator_dt_data (int n_regulator,const unsigned int *regulator_idx_table,struct rtq2208_regulator_desc *rdesc[RTQ2208_LDO_MAX ],struct device *dev) { -struct of_regulator_match rtq2208_ldo_match[2 ]; -int mtp_sel, ret, i, idx, ldo_idx =0 ; +int mtp_sel, i, idx, ret;/* get mtp_sel0 or mtp_sel1 */ mtp_sel =device_property_read_bool (dev,"richtek,mtp-sel-high" );
@@ -499,14 +501,14 @@ static int rtq2208_parse_regulator_dt_data(int n_regulator, const unsigned int *if (!rdesc[i])return -ENOMEM ; -rtq2208_init_regulator_desc (rdesc[i], mtp_sel, idx, rtq2208_ldo_match, &ldo_idx); +rtq2208_init_regulator_desc (rdesc[i], mtp_sel, idx);/* init ldo dvs ability */ if (idx >=RTQ2208_LDO2 ) rtq2208_ldo_match[idx -RTQ2208_LDO2 ].desc = &rdesc[i]->desc; } - ret =rtq2208_of_get_ldo_dvs_ability (dev, rtq2208_ldo_match, ldo_idx); + ret =rtq2208_of_get_ldo_dvs_ability (dev);if (ret)return dev_err_probe (dev, ret,"Failed to get ldo fixed_uV\n" );
Signature · 1 line
2.7.4
Mark Brown
maintainerRe: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
In reply to Alina Yudrivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 13 lines · Alina Yu
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.drivers/regulator/rtq2208-regulator.c +9
-0+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.
Mark Brown
maintainerRe: [PATCH v2 3/4] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called
In reply to Alina YuQuoted 4 lines · Alina Yu
In this patch, a software bug has been fixed.
rtq2208_ldo_matchis no longer a local variable.
It prevents invalid memory access whendevm_of_regulator_put_matchesis called.
Fixes should generally go at the start of the series, that way they can be merged without any spurious dependencies on later work.
Mark Brown
maintainerRe: [PATCH v2 1/4] regulator: rtq2208: Fix LDO discharge register and add vsel setting
In reply to Alina YuQuoted 3 lines · Alina Yu
The LDO's Vout is adjustable if the hardware setting allows it, and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position.
The discharge register change should really be a separate patch since it's a bugfix, though it's probably not a super urgent one.
Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
In reply to Mark BrownQuoted 6 lines · Mark Brown
Quoted 3 lines · Alina Yu
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.
Quoted 13 lines
drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 9 lines
drivers/regulator/rtq2208-regulator.c +9
-0+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
Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 21 lines · Alina Yu
Quoted 6 lines · Mark Brown
Quoted 3 lines · Alina Yu
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.Quoted 13 lines
drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 9 lines
drivers/regulator/rtq2208-regulator.c +9
-0+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.
Quoted 4 lines
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
Quoted 2 lines
Mark Brown
maintainerRe: (subset) [PATCH v2 0/4] Fix rtq2208 BUCK ramp_delay and LDO dvs setting
In reply to Alina YuQuoted 11 lines · Alina Yu
This series patches is for hardware modification of RTQ2208.
ramp_delayrange of BUCK is changed.
The maximum ramp up and down range of BUCK are shorten
from 64mVstep/us to 16mVstep/us.
The LDO's Vout is adjustable if the hardware setting allow it,
and it can be set either 1800mv or 3300mv.
Additionally, the discharge register has been moved to another position. In this version, a software bug has been fixed.rtq2208_ldo_matchis no longer a local variable.
It prevents invalid memory access whendevm_of_regulator_put_matchesis called.
Quoted 1 line
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/4] regulator: rtq2208: Fix LDO discharge register and add vsel setting
commit: 38bcec0e7cbbd6566c12ae4f2b7a48bd50cd215c
[3/4] regulator: rtq2208: Fix invalid memory access when devm_of_regulator_put_matches is called
(no commit info)
[4/4] regulator: rtq2208: Fix the BUCK ramp_delay range to maximum of 16mVstep/us
commit: d1ef160b45a0010d1f1b3d601230457243a8f3e8All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks,
Mark
Mark Brown
maintainerRe: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
In reply to Alina YuQuoted 17 lines · Alina Yu
Quoted 15 lines · Mark Brown
drivers/regulator/rtq2208-regulator.c static int rtq2208_of_get_fixed_voltage(struct device *dev, · line 382
Quoted 9 lines · Alina Yu
drivers/regulator/rtq2208-regulator.c +9
-0+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.
Quoted 2 lines
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.
Mark Brown
maintainerRe: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
In reply to Alina YuQuoted 3 lines · Alina Yu
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.
Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 11 lines · Mark Brown
Quoted 3 lines · Alina Yu
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
Mark Brown
maintainerRe: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 9 lines · Alina Yu
Quoted 5 lines · Mark Brown
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.
Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 9 lines · Mark Brown
Quoted 7 lines · Alina Yu
Quoted 5 lines · Mark Brown
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.
Quoted 5 lines
Quoted 1 line
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
Mark Brown
maintainerRe: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 3 lines · Alina Yu
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 byconstraints->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.
Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 11 lines · Mark Brown
Quoted 3 lines · Alina Yu
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 byconstraints->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
Mark Brown
maintainerRe: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 1 line · Alina Yu
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.
Re: [PATCH v2 2/4] regulator: rtq2208: Fix LDO to be compatible with both fixed and adjustable vout
Quoted 7 lines · Mark Brown
Quoted 1 line · Alina Yu
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