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

Romain Gantois
2025-09-16T14:18:09Z

[-- Attachment #1: Type: text/plain, Size: 5528 bytes --]

Hello Andy,

On Tuesday, 16 September 2025 15:12:37 CEST Andy Shevchenko wrote:
> 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).
>

IIRC the "of_match" regulator descriptor property can be used for this, I'll
have a second look and see if I can use that instead.

> > +#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])
>
> ...

I usually also try to follow IWYU for header inclusions so I'll look for those
that I missed.

>
> > +/* 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?
>

Both of those are unsigned so the cast here is indeed unnecessary.

> > +}
>
> ...
>
> > +static const struct regulator_ops ltm8054_regulator_ops = {
> > +};
>
> Why it can be simply as
>
> static const struct regulator_ops ltm8054_regulator_ops;
>

Yeah, this was mostly to have a clean diff on patch 4/4, I'll see if I can drop
this struct and introduce it in patch 4/4. I wouldn't want to use it
uninitialized though.

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

Yes, indeed.

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

Yes, I used that in the caller function but it doesn't solve the flooding
issue, so I'll move dev_err_probe() to this function 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);

Thanks for the review,

--
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]