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

Andy Shevchenko
2025-09-16T13:12:44Z

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