Re: [PATCH v4 6/6] regulator: add Rockchip rk808 support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Ahmad,

I was tracking down some strange behavior and found something:

On 7/24/22 21:00, Ahmad Fatoum wrote:
> [...]
> +static struct rk_regulator_cfg rk809_reg[] = {
> +	{{
> +		/* .name = "DCDC_REG1", */
> +		.supply_name = "vcc1",
> +		.ops = &rk817_buck_ops_range,
> +		.n_voltages = RK817_BUCK1_SEL_CNT + 1,
> +		.linear_ranges = rk817_buck1_voltage_ranges,
> +		.n_linear_ranges = ARRAY_SIZE(rk817_buck1_voltage_ranges),
> +		.vsel_reg = RK817_BUCK1_ON_VSEL_REG,
> +		.vsel_mask = RK817_BUCK_VSEL_MASK,
> +		.enable_reg = RK817_POWER_EN_REG(0),
> +		.enable_mask = ENABLE_MASK(RK817_ID_DCDC1),
> +		.enable_val = ENABLE_MASK(RK817_ID_DCDC1),
> +		.disable_val = DISABLE_VAL(RK817_ID_DCDC1),
> +	}}, {{
> +		/* .name = "DCDC_REG2", */
> +		.supply_name = "vcc2",
> +		.ops = &rk817_buck_ops_range,
> +		.n_voltages = RK817_BUCK1_SEL_CNT + 1,
> +		.linear_ranges = rk817_buck1_voltage_ranges,
> +		.n_linear_ranges = ARRAY_SIZE(rk817_buck1_voltage_ranges),
> +		.vsel_reg = RK817_BUCK2_ON_VSEL_REG,
> +		.vsel_mask = RK817_BUCK_VSEL_MASK,
> +		.enable_reg = RK817_POWER_EN_REG(0),
> +		.enable_mask = ENABLE_MASK(RK817_ID_DCDC2),
> +		.enable_val = ENABLE_MASK(RK817_ID_DCDC2),
> +		.disable_val = DISABLE_VAL(RK817_ID_DCDC2),
> +	}}, {{
> +		/* .name = "DCDC_REG3", */
> +		.supply_name = "vcc3",
> +		.ops = &rk817_buck_ops_range,
> +		.n_voltages = RK817_BUCK1_SEL_CNT + 1,
> +		.linear_ranges = rk817_buck1_voltage_ranges,
> +		.n_linear_ranges = ARRAY_SIZE(rk817_buck1_voltage_ranges),
> +		.vsel_reg = RK817_BUCK3_ON_VSEL_REG,
> +		.vsel_mask = RK817_BUCK_VSEL_MASK,
> +		.enable_reg = RK817_POWER_EN_REG(0),
> +		.enable_mask = ENABLE_MASK(RK817_ID_DCDC3),
> +		.enable_val = ENABLE_MASK(RK817_ID_DCDC3),
> +		.disable_val = DISABLE_VAL(RK817_ID_DCDC3),
> +	}}, {{
> +		/* .name = "DCDC_REG4", */
> +		.supply_name = "vcc4",
> +		.ops = &rk817_buck_ops_range,
> +		.n_voltages = RK817_BUCK3_SEL_CNT + 1,
> +		.linear_ranges = rk817_buck3_voltage_ranges,
> +		.n_linear_ranges = ARRAY_SIZE(rk817_buck3_voltage_ranges),
> +		.vsel_reg = RK817_BUCK4_ON_VSEL_REG,
> +		.vsel_mask = RK817_BUCK_VSEL_MASK,
> +		.enable_reg = RK817_POWER_EN_REG(0),
> +		.enable_mask = ENABLE_MASK(RK817_ID_DCDC4),
> +		.enable_val = ENABLE_MASK(RK817_ID_DCDC4),
> +		.disable_val = DISABLE_VAL(RK817_ID_DCDC4),
> +	}}, {{
> +		/* .name = "DCDC_REG5", */
> +		.supply_name = "vcc9",
> +		.ops = &rk809_buck5_ops_range,
> +		.n_voltages = RK809_BUCK5_SEL_CNT,
> +		.linear_ranges = rk809_buck5_voltage_ranges,
> +		.n_linear_ranges = ARRAY_SIZE(rk809_buck5_voltage_ranges),
> +		.vsel_reg = RK809_BUCK5_CONFIG(0),
> +		.vsel_mask = RK809_BUCK5_VSEL_MASK,
> +		.enable_reg = RK817_POWER_EN_REG(3),
> +		.enable_mask = ENABLE_MASK(1),
> +		.enable_val = ENABLE_MASK(1),
> +		.disable_val = DISABLE_VAL(1),
> +	}},

The RK809 features an additional BUCK converter (DCDC_REG5), here on
index 4 of the array...

> +	RK817_DESC(/* "LDO_REG1", */ "vcc5", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(0), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(1), ENABLE_MASK(0),
> +		   DISABLE_VAL(0), 400),
> +	RK817_DESC(/* "LDO_REG2", */ "vcc5", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(1), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(1), ENABLE_MASK(1),
> +		   DISABLE_VAL(1), 400),
> +	RK817_DESC(/* "LDO_REG3", */ "vcc5", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(2), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(1), ENABLE_MASK(2),
> +		   DISABLE_VAL(2), 400),
> +	RK817_DESC(/* "LDO_REG4", */ "vcc6", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(3), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(1), ENABLE_MASK(3),
> +		   DISABLE_VAL(3), 400),
> +	RK817_DESC(/* "LDO_REG5", */ "vcc6", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(4), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(2), ENABLE_MASK(0),
> +		   DISABLE_VAL(0), 400),
> +	RK817_DESC(/* "LDO_REG6", */ "vcc6", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(5), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(2), ENABLE_MASK(1),
> +		   DISABLE_VAL(1), 400),
> +	RK817_DESC(/* "LDO_REG7", */ "vcc7", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(6), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(2), ENABLE_MASK(2),
> +		   DISABLE_VAL(2), 400),
> +	RK817_DESC(/* "LDO_REG8", */ "vcc7", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(7), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(2), ENABLE_MASK(3),
> +		   DISABLE_VAL(3), 400),
> +	RK817_DESC(/* "LDO_REG9", */ "vcc7", 600, 3400, 25,
> +		   RK817_LDO_ON_VSEL_REG(8), RK817_LDO_VSEL_MASK,
> +		   RK817_POWER_EN_REG(3), ENABLE_MASK(0),
> +		   DISABLE_VAL(0), 400),
> +	RK817_DESC_SWITCH(/* "SWITCH_REG1", */ "vcc9",
> +			  RK817_POWER_EN_REG(3), ENABLE_MASK(2), DISABLE_VAL(2)),
> +	RK817_DESC_SWITCH(/* "SWITCH_REG2", */ "vcc8",
> +			  RK817_POWER_EN_REG(3), ENABLE_MASK(3), DISABLE_VAL(3)),
> +};
> +static_assert(ARRAY_SIZE(rk809_reg) == RK809_NUM_REGULATORS);
> [...]
> +static struct of_regulator_match rk809_reg_matches[] = {
> +	MATCH(809, DCDC_REG1, DCDC1),
> +	MATCH(809, DCDC_REG2, DCDC2),
> +	MATCH(809, DCDC_REG3, DCDC3),
> +	MATCH(809, DCDC_REG4, DCDC4),
> +	MATCH(809, LDO_REG1, LDO1),
> +	MATCH(809, LDO_REG2, LDO2),
> +	MATCH(809, LDO_REG3, LDO3),
> +	MATCH(809, LDO_REG4, LDO4),
> +	MATCH(809, LDO_REG5, LDO5),
> +	MATCH(809, LDO_REG6, LDO6),
> +	MATCH(809, LDO_REG7, LDO7),
> +	MATCH(809, LDO_REG8, LDO8),
> +	MATCH(809, LDO_REG9, LDO9),
> +	MATCH(809, DCDC_REG5, DCDC5),

Here, BUCK 5 is at index 13...

> +	MATCH(809, SWITCH_REG1, SW1),
> +	MATCH(809, SWITCH_REG2, SW2),
> +};
> +static_assert(ARRAY_SIZE(rk809_reg_matches) == RK809_NUM_REGULATORS);
> [...]
> +static int rk808_regulator_probe(struct device_d *dev)
> +{
> +	struct rk808 *rk808 = dev->parent->priv;
> +	struct rk_regulator_cfg *regulators;
> +	struct of_regulator_match *matches;
> +	int ret, i, nregulators;
> +
> +	switch (rk808->variant) {
> +	case RK805_ID:
> +		regulators = rk805_reg;
> +		matches = rk805_reg_matches;
> +		nregulators = RK805_NUM_REGULATORS;
> +		break;
> +	case RK808_ID:
> +		regulators = rk808_reg;
> +		matches = rk808_reg_matches;
> +		nregulators = RK809_NUM_REGULATORS;
> +		break;
> +	case RK809_ID:
> +		regulators = rk809_reg;
> +		matches = rk809_reg_matches;
> +		nregulators = RK809_NUM_REGULATORS;
> +		break;
> +	case RK817_ID:
> +		regulators = rk817_reg;
> +		matches = rk817_reg_matches;
> +		nregulators = RK817_NUM_REGULATORS;
> +		break;
> +	case RK818_ID:
> +		regulators = rk818_reg;
> +		matches = rk818_reg_matches;
> +		nregulators = RK818_NUM_REGULATORS;
> +		break;
> +	default:
> +		dev_err(dev, "unsupported RK8XX ID %lu\n", rk808->variant);
> +		return -EINVAL;
> +	}
> +
> +	ret = rk808_regulator_dt_parse(&rk808->i2c->dev, matches, nregulators);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Instantiate the regulators */
> +	for (i = 0; i < nregulators; i++) {
> +		ret = rk808_regulator_register(rk808, i, &matches[i],
> +					       &regulators[i]);

... but here it a 1:1 mapping between matches and regulators is assumed.
In technical terms, board go brzzz.

I moved the DCDC_REG5 regulator description to index 13 for a quick fix.
If this is the desired solution, I can spin a patch. What do you think?

Best regards,
Michael

> [...]
[Index of Archives]     [Linux Embedded]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux