Hi,
This series patches is for hardware modification of RTQ2208.
ramp_delay range of bucks is changed.
The maximum ramp up and down range is shorten from 64mVstep/us tor 16mVstep/us/.
The LDO's Vout is adjustable if the haardware setting allow it, and it can be set either 1800mv or 3300mv.
Additionally, the discharge register is chaned to other position.
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,
which prevents invalid memory access when devm_of_regulator_put_matches is called.
Thank you,
Alina
---
Alina Yu (2):
regulator: dt-bindings: rtq2208: Add Richtek RTQ2208 SubPMIC
regulator: rtq2208: Add Richtek RTQ2208 SubPMIC driver
.../bindings/regulator/richtek,rtq2208.yaml | 10 ++
drivers/regulator/rtq2208-regulator.c | 169 +++++++++++++--------
2 files changed, 119 insertions(+), 60 deletions(-)
--
2.7.4
ramp_delay range of bucks is changed.
The maximum ramp up and down range is shorten from 64mVstep/us tor 16mVstep/us/.
The LDO's Vout is adjustable if the haardware setting allow it, and it can be set either 1800mv or 3300mv.
Additionally, the discharge register is chaned to other position.
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,
which prevents invalid memory access when devm_of_regulator_put_matches is called.
Signed-off-by: Alina Yu <[email protected]>
---
drivers/regulator/rtq2208-regulator.c | 169 ++++++++++++++++++++++------------
1 file changed, 109 insertions(+), 60 deletions(-)
diff --git a/drivers/regulator/rtq2208-regulator.c b/drivers/regulator/rtq2208-regulator.c
index 2d54844..95612cc 100644
--- a/drivers/regulator/rtq2208-regulator.c
+++ b/drivers/regulator/rtq2208-regulator.c
@@ -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
@@ -48,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))
@@ -142,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
*/
@@ -215,7 +219,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,
@@ -224,6 +228,28 @@ 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 struct of_regulator_match rtq2208_ldo_match[] = {
+ {.name = "ldo2", },
+ {.name = "ldo1", },
+};
+
static unsigned int rtq2208_of_map_mode(unsigned int mode)
{
switch (mode) {
@@ -318,30 +344,13 @@ 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)
+static int rtq2208_of_get_ldo_dvs_ability(struct device *dev)
{
struct device_node *np;
struct of_regulator_match *match;
- struct rtq2208_regulator_desc *rdesc;
+ struct regulator_desc *desc;
struct regulator_init_data *init_data;
+ bool fixed_uV;
int ret, i;
if (!dev->of_node)
@@ -351,46 +360,87 @@ static int rtq2208_of_get_fixed_voltage(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;
- 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;
+ 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;
+ }
}
return 0;
}
-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)
+
+#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 regulator_desc *desc;
static 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 +452,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 +466,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 +477,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;
@@ -444,8 +491,7 @@ static void rtq2208_init_regulator_desc(struct rtq2208_regulator_desc *rdesc, in
static 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");
@@ -457,11 +503,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;
}
- /* init ldo fixed_uV */
- ret = rtq2208_of_get_fixed_voltage(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");
--
2.7.4
Since there is no way to check is ldo is adjustable or not.
'richtek,use-fix-dvs' is added for that. user is supposed to know whether vout of ldo is adjustable.
Signed-off-by: Alina Yu <[email protected]>
---
.../devicetree/bindings/regulator/richtek,rtq2208.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
index 609c066..3951679 100644
--- a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
+++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
@@ -75,6 +75,14 @@ properties:
description:
regulator description for ldo[1-2].
+ properties:
+ richtek,use-fix-dvs:
+ type: boolean
+ description: |
+ ldo vout ability is determined by this setting. If it's set, the voltage is unadjustable.
+ There is no risk-free method for software to determine whether the ldo vout is fixed or not.
+ Therefore, it can only be done in this way.
+
required:
- compatible
- reg
@@ -180,6 +188,7 @@ examples:
regulator-min-microvolt = <1200000>;
regulator-max-microvolt = <1200000>;
regulator-always-on;
+ richtek,use-fix-dvs;
regulator-state-mem {
regulator-on-in-suspend;
};
@@ -188,6 +197,7 @@ examples:
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
regulator-always-on;
+ richtek,use-fix-dvs;
regulator-state-mem {
regulator-on-in-suspend;
};
--
2.7.4
On 29/04/2024 12:16, Alina Yu wrote:
> Since there is no way to check is ldo is adjustable or not.
> 'richtek,use-fix-dvs' is added for that. user is supposed to know whether vout of ldo is adjustable.
1. Please wrap commit message according to Linux coding style /
submission process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
2. Start sentences with capital letters. LDO is acronym.
3. Constraints already tell you that, don't they? Explain why they are
not enough and you need new property.
>
> Signed-off-by: Alina Yu <[email protected]>
> ---
> .../devicetree/bindings/regulator/richtek,rtq2208.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> index 609c066..3951679 100644
> --- a/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> +++ b/Documentation/devicetree/bindings/regulator/richtek,rtq2208.yaml
> @@ -75,6 +75,14 @@ properties:
> description:
> regulator description for ldo[1-2].
>
> + properties:
> + richtek,use-fix-dvs:
> + type: boolean
> + description: |
> + ldo vout ability is determined by this setting. If it's set, the voltage is unadjustable.
> + There is no risk-free method for software to determine whether the ldo vout is fixed or not.
> + Therefore, it can only be done in this way.
Wrap according to Linux style (as expressed in Linux coding style document).
Best regards,
Krzysztof
On Mon, Apr 29, 2024 at 06:16:47PM +0800, Alina Yu wrote:
> ramp_delay range of bucks is changed.
> The maximum ramp up and down range is shorten from 64mVstep/us tor 16mVstep/us/.
> The LDO's Vout is adjustable if the haardware setting allow it, and it can be set either 1800mv or 3300mv.
> Additionally, the discharge register is chaned to other position.
> 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,
> which prevents invalid memory access when devm_of_regulator_put_matches is called.
As covered in submitting-patches.rst since this is a bunch of different
changes that don't really overlap it should be split into multiple
patches, for example the change of the ramp time for the DCDCs should be
separate to all the LDO stuff.
> - if (init_data->constraints.min_uV == init_data->constraints.max_uV)
> - rdesc->desc.fixed_uV = init_data->constraints.min_uV;
> + 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;
> + }
The driver shouldn't need to look at the constraints to set up the
operations for the hardware, if the regulator can be configured for
multiple voltages just register it that way and let the core figure out
if we ever actually want to change the voltage.
On Mon, Apr 29, 2024 at 06:16:46PM +0800, Alina Yu wrote:
> Since there is no way to check is ldo is adjustable or not.
> 'richtek,use-fix-dvs' is added for that. user is supposed to know whether vout of ldo is adjustable.
As Krzysztof said we already know if the voltage can change since in
order for Linux to change the voltage there must be a voltage range
specified (and see comment on patch 2).