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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZF54t38o2PFsl_oX@surfacebook>
Date:   Fri, 12 May 2023 20:34:47 +0300
From:   andy.shevchenko@...il.com
To:     Esteban Blanc <eblanc@...libre.com>
Cc:     linus.walleij@...aro.org, lgirdwood@...il.com, broonie@...nel.org,
        a.zummo@...ertech.it, alexandre.belloni@...tlin.com,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-rtc@...r.kernel.org, jpanis@...libre.com,
        jneanne@...libre.com, aseketeli@...libre.com, sterzik@...com,
        u-kumar1@...com
Subject: Re: [PATCH v4 3/3] regulator: tps6594-regulator: Add driver for TI
 TPS6594 regulators

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

...

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