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
