Message-ID: <5205519.GXAFRqVoOG@fw-rgant>
Date: Tue, 16 Sep 2025 16:17:56 +0200
From: Romain Gantois <romain.gantois@...tlin.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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

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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists