linux-kernel - Re: [PATCH 3/4] regulator: Support the LTM8054 voltage regulator

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: <aMliRTuUDNPkeM8C@smile.fi.intel.com>
Date: Tue, 16 Sep 2025 16:12:37 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH 3/4] regulator: Support the LTM8054 voltage regulator

On Tue, Sep 16, 2025 at 12:24:08PM +0200, Romain Gantois wrote:
> Add a stub driver for the  Linear Technology LTM8054 Buck-Boost voltage
> regulator. This version only supports enabling/disabling the regulator via
> a GPIO, and reporting the output voltage level from the resistor divider
> values given in the device tree.

...

> +#include <linux/module.h>
> +#include <linux/of.h>

I think we have already something agnostic in regulator API to get a regulator
from a firmware node (rather than from specific OF/etc one).

> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>

Can you keep it ordered? This way it's easy to maintain and avoid potential
duplication (note, there are also many headers are missing here, but Mark
usually not insisting in following IWYU principle [1])

...

> +/* The LTM8054 regulates its FB pin to 1.2V */
> +#define LTM8054_FB_V 1200000

It's actually _mV

#define LTM8054_FB_mV	1200000

...

> +static int ltm8054_scale(unsigned int uV, u32 r1, u32 r2)
> +{
> +	u64 tmp;
> +
> +	tmp = (u64)uV * r1;
> +	do_div(tmp, r2);
> +
> +	return uV + (unsigned int)tmp;

Why one needs a casting here?

> +}

...

> +static const struct regulator_ops ltm8054_regulator_ops = {
> +};

Why it can be simply as

static const struct regulator_ops ltm8054_regulator_ops;

...

> +static int ltm8054_of_parse(struct device *dev, struct ltm8054_priv *priv,
> +			    struct regulator_config *config)
> +{
> +	struct device_node *np = dev->of_node;
> +	u32 r[2];
> +	int ret;
> +
> +	config->of_node = np;
> +
> +	ret = of_property_read_u32_array(np, "lltc,fb-voltage-divider", r, 2);

device_property_read_u32_array() ?

ARRAY_SIZE() instead of 2

> +	if (ret) {
> +		dev_err(dev, "Failed to parse voltage divider\n");
> +		return ret;
> +	}
> +
> +	priv->rdesc.fixed_uV = ltm8054_scale(LTM8054_FB_V, r[0], r[1]);
> +	priv->rdesc.min_uV = priv->rdesc.fixed_uV;
> +	priv->rdesc.n_voltages = 1;
> +
> +	config->init_data = of_get_regulator_init_data(dev,
> +						       np,
> +						       &priv->rdesc);
> +	if (!config->init_data) {
> +		dev_err(dev, "failed to parse init data\n");
> +		return -EINVAL;
> +	}
> +
> +	config->ena_gpiod = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(config->ena_gpiod)) {
> +		dev_err(dev, "unable to acquire enable gpio\n");
> +		return PTR_ERR(config->ena_gpiod);

All messages in cases of EPROBE_DEFER are problematic (for sure with GPIO),
as it may well flood the logs.
Solution: Use

		return dev_err_probe(...);

pattern instead,

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

...

> +static int ltm8054_probe(struct platform_device *pdev)
> +{
> +	struct regulator_config config = { 0 };

'0' is not required. The { } will have the same effect.

> +	struct regulator_dev *rdev;
> +	struct ltm8054_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->rdesc.name = "ltm8054-regulator",
> +	priv->rdesc.ops = &ltm8054_regulator_ops,
> +	priv->rdesc.type = REGULATOR_VOLTAGE,
> +	priv->rdesc.owner = THIS_MODULE,
> +
> +	config.dev = &pdev->dev;
> +	config.driver_data = priv;
> +
> +	ret = ltm8054_of_parse(&pdev->dev, priv, &config);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to parse device tree\n");
> +
> +	rdev = devm_regulator_register(&pdev->dev, &priv->rdesc, &config);
> +	if (IS_ERR(rdev))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rdev), "failed to register regulator\n");

Using

	struct device *dev = &pdev->dev;

at the top will allow to make a few lines shorter.

> +	return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused ltm8054_of_match[] = {
> +	{ .compatible = "lltc,ltm8054", },

Inner comma is not required.

> +	{},

Drop the trailing comma here as it's a terminator entry. The absence of it will
give a hint to the compiler as well.

> +};

...

> +static struct platform_driver ltm8054_driver = {
> +	.probe = ltm8054_probe,
> +	.driver = {
> +		.name  = "ltm8054",
> +		.of_match_table = of_match_ptr(ltm8054_of_match),

Please, do not use of_match_ptr() and/or ACPI_PTR() in a new code.

> +	},
> +};

> +

Unneeded blank line.

> +module_platform_driver(ltm8054_driver);

-- 
With Best Regards,
Andy Shevchenko