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

[andy . shevchenko]

Fri, May 12, 2023 at 04:17:55PM +0200, Esteban Blanc kirjoitti:
> From: Jerome Neanne <jneanne@xxxxxxxxxxxx>
> 
> 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.

...

> +	enum {
> +	MULTI_BUCK12,
> +	MULTI_BUCK123,
> +	MULTI_BUCK1234,
> +	MULTI_BUCK12_34,
> +	MULTI_FIRST = MULTI_BUCK12,
> +	MULTI_LAST = MULTI_BUCK12_34,
> +	MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1

Missing TABs?

> +	};

...

> +	for (multi = MULTI_FIRST ; multi < MULTI_NUM ; multi++) {

Just a remark: spaces before ; are not standard.

> +		np = of_find_node_by_name(tps->dev->of_node, multiphases[multi]);
> +		npname = of_node_full_name(np);
> +		np_pmic_parent = of_get_parent(of_get_parent(np));
> +		if (strcmp((of_node_full_name(np_pmic_parent)), tps->dev->of_node->full_name))
> +			continue;

Isn't there an API to compare OF node name with a given parameter?

> +		delta = strcmp(npname, multiphases[multi]);
> +		if (!delta) {
> +			switch (multi) {
> +			case MULTI_BUCK12:
> +				buck_multi[0] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				break;
> +			/* multiphase buck34 is supported only with buck12 */
> +			case MULTI_BUCK12_34:
> +				buck_multi[0] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;

> +				buck_multi[1] = 1;

Might be easier to read if this is grouped with [0] assignment above.

> +				buck_configured[2] = 1;
> +				buck_configured[3] = 1;
> +				break;
> +			case MULTI_BUCK123:
> +				buck_multi[2] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				buck_configured[2] = 1;
> +				break;
> +			case MULTI_BUCK1234:
> +				buck_multi[3] = 1;
> +				buck_configured[0] = 1;
> +				buck_configured[1] = 1;
> +				buck_configured[2] = 1;
> +				buck_configured[3] = 1;
> +				break;
> +			}
> +		}
> +	}

...

> +	irq_data = devm_kmalloc(tps->dev,
> +				ARRAY_SIZE(tps6594_bucks_irq_types) *
> +				REGS_INT_NB *
> +				sizeof(struct tps6594_regulator_irq_data) +
> +				ARRAY_SIZE(tps6594_ldos_irq_types) *
> +				REGS_INT_NB *
> +				sizeof(struct tps6594_regulator_irq_data),

We have respective macros in overflow.h that can be used here.

> +				GFP_KERNEL);
> +	if (!irq_data)
> +		return -ENOMEM;

...

> +		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);

		return dev_err_probe(...);
?

> +		}

...

> +		rdev = devm_regulator_register(&pdev->dev, &buck_regs[i], &config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(tps->dev, "failed to register %s regulator\n",
> +				pdev->name);
> +			return PTR_ERR(rdev);

Same question.

> +		}

...

> +			rdev = devm_regulator_register(&pdev->dev, &ldo_regs[i], &config);
> +			if (IS_ERR(rdev)) {
> +				dev_err(tps->dev,
> +					"failed to register %s regulator\n",
> +					pdev->name);
> +				return PTR_ERR(rdev);

Same question.

> +			}

> +	irq_ext_reg_data = devm_kmalloc(tps->dev,
> +					ext_reg_irq_nb *
> +					sizeof(struct tps6594_ext_regulator_irq_data),
> +					GFP_KERNEL);

devm_kmalloc_array()

> +	if (!irq_ext_reg_data)
> +		return -ENOMEM;

...

> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps6594_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_ext_reg_data[i]);
> +		if (error) {
> +			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);
> +			return error;

	return dev_err_probe(...);

?

> +		}
> +	}
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko