Re: [PATCH INTERNAL v1 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

Julien Panis
2023-03-22T09:11:13Z

On 2/24/23 14:31, Esteban Blanc wrote:
> From: Jerome Neanne <jneanne@baylibre.com>
>
> This patch adds support for TPS6594 regulators (bucks and LDOs).
> The output voltages are configurable and are meant to supply power
> to the main processor and other components.
> Bucks can be used in single or multiphase mode, depending on PMIC
> part number.
>
> Signed-off-by: Jerome Neanne <jneanne@baylibre.com>
> ---

(...)

> +static int tps6594_regulator_probe(struct platform_device *pdev)
> +{
> + struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
> + struct regulator_dev *rdev;
> + struct regulator_config config = {};
> + u8 buck_configured[BUCK_NB] = { 0 };
> + u8 buck_multi[MULTI_PHASE_NB] = { 0 };
> + int i;
> + int error;
> + int irq;
> + int ext_reg_irq_nb = 2;
> + struct tps6594_regulator_irq_data *irq_data;
> + struct tps6594_ext_regulator_irq_data *irq_ext_reg_data;
> + struct tps6594_regulator_irq_type *irq_type;
> + struct regulator_dev *rdevbucktbl[BUCK_NB];
> + struct regulator_dev *rdevmultitbl[MULTI_PHASE_NB];
> + struct regulator_dev *rdevldotbl[LDO_NB];
> +
> + int multi_phase_id;
> + int multi_phase_case = 0xFFFF;
> +
> + config.dev = tps->dev;
> + config.driver_data = tps;
> + config.regmap = tps->regmap;
> +
> + /*
> + * Switch case defines different possible multi phase config
> + * This is based on dts custom property: multi-phase-id
> + * Using compatible or device rev is a too complex alternative
> + * Default case is no Multiphase buck.
> + * In case of Multiphase configuration, value should be defined for
> + * buck_configured to avoid creating bucks for every buck in multiphase
> + */
> +
> + if (device_property_present(tps->dev, "ti,multi-phase-id")) {

Question @ Mark/Liam:
Shouldn't we use the generic 'regulator-coupled-with' property
instead of 'ti,multi-phase-id' ?
I am in charge of upstreaming dt-bindings and maintainers
pointed out the similarity between 'multi-phase' and 'coupled'
regulator concepts. Does 'regulator-coupled-with' mean that
outputs of buck converters are combined ? If so, this generic
property should replace our specific 'ti,multi-phase-id' prop,
I guess.

> + device_property_read_u32(tps->dev, "ti,multi-phase-id", &multi_phase_id);
> + switch (multi_phase_id) {
> + case 12:
> + buck_multi[0] = 1;
> + buck_configured[0] = 1;
> + buck_configured[1] = 1;
> + multi_phase_case = TPS6594_BUCK_12;
> + break;
> + case 34:
> + buck_multi[1] = 1;
> + buck_configured[2] = 1;
> + buck_configured[3] = 1;
> + multi_phase_case = TPS6594_BUCK_34;
> + break;
> + case 123:
> + buck_multi[2] = 1;
> + buck_configured[0] = 1;
> + buck_configured[1] = 1;
> + buck_configured[2] = 1;
> + multi_phase_case = TPS6594_BUCK_123;
> + break;
> + case 1234:
> + buck_multi[3] = 1;
> + buck_configured[0] = 1;
> + buck_configured[1] = 1;
> + buck_configured[2] = 1;
> + buck_configured[3] = 1;
> + multi_phase_case = TPS6594_BUCK_1234;
> + break;
> + }
> + }
> +
> + for (i = 0; i < MULTI_PHASE_NB; i++) {
> + if (buck_multi[i] == 0)
> + continue;
> +
> + rdev = devm_regulator_register(&pdev->dev, &multi_regs[i], &config);
> + if (IS_ERR(rdev)) {
> + dev_err(tps->dev, "failed to register %s regulator\n",
> + pdev->name);
> + return PTR_ERR(rdev);
> + }
> + rdevmultitbl[i] = rdev;
> + }
> +

(...)