| 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
| ||
|
Message-ID: <aQzpvR-030zgA82E@smile.fi.intel.com>
Date: Thu, 6 Nov 2025 20:32:29 +0200
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>, Hans de Goede <hansg@...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 v3 5/5] regulator: ltm8054: Support output current limit
control
On Thu, Nov 06, 2025 at 03:11:50PM +0100, Romain Gantois wrote:
> The LTM8054 supports setting a fixed output current limit using a sense
> resistor connected to a dedicated pin. This limit can then be lowered
> dynamically by varying the voltage level of the CTL pin.
>
> Support controlling the LTM8054's output current limit.
...
> #include <linux/array_size.h>
> +#include <linux/completion.h>
> #include <linux/device.h>
> #include <linux/device/devres.h>
> #include <linux/device/driver.h>
> #include <linux/errno.h>
>
> #include <linux/gpio/consumer.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/jiffies.h>
> +#include <linux/lockdep.h>
> #include <linux/math64.h>
> +#include <linux/minmax.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> #include <linux/regulator/of_regulator.h>
> #include <linux/types.h>
>
> +#include <linux/units.h>
> +#include <linux/workqueue.h>
This will be updated accordingly.
...
> +struct ltm8054_ctl_pin_work {
> + struct work_struct work;
> + unsigned int ctl_val;
> + bool write;
> + int ret;
> +};
Have you ran `pahole`? It might suggest a better layout to save a few bytes.
...
> +static void ltm8054_do_ctl_work(struct work_struct *work)
> +{
> + struct ltm8054_ctl_pin_work *ctl_work = container_of_const(work,
> + struct ltm8054_ctl_pin_work,
> + work);
> + struct ltm8054_priv *priv = container_of_const(ctl_work,
> + struct ltm8054_priv,
> + ctl_work);
These read better in slightly different split:
struct ltm8054_ctl_pin_work *ctl_work =
container_of_const(work, struct ltm8054_ctl_pin_work, work);
struct ltm8054_priv *priv =
container_of_const(ctl_work, struct ltm8054_priv, ctl_work);
...
> + mutex_lock(&priv->ctl_work_lock);
> + val = ctl_work->ctl_val;
> + write = ctl_work->write;
> + mutex_unlock(&priv->ctl_work_lock);
Why not scoped_guard() from cleanup,h?
...
> + /* Standard IIO voltage unit is mV, scale accordingly. */
> + if (write)
> + ret = iio_write_channel_processed_scale(priv->ctl_dac,
> + val, 1000);
One line. It just 82 characters.
> + else
> + ret = iio_read_channel_processed_scale(priv->ctl_dac,
> + &val, 1000);
Ditto.
And perhaps use MILLI/KILO?
...
> + pr_debug("LTM8054: %s CTL IO channel, val: %duV\n", write ? "wrote" : "reading", val);
Besides str_write_read() from string_choices.h this should be dev_dbg().
> + mutex_lock(&priv->ctl_work_lock);
> + ctl_work->ret = ret;
> + ctl_work->ctl_val = val;
> + mutex_unlock(&priv->ctl_work_lock);
scoped_guard()
> + complete(&priv->ctl_rw_done);
> +}
...
> +static int ltm8054_ctl_pin_rw(struct ltm8054_priv *priv, bool write, unsigned int *ctl_val)
> +{
> + struct ltm8054_ctl_pin_work *ctl_work = &priv->ctl_work;
> + int ret = 0;
Redundant assignment.
> + lockdep_assert_not_held(&priv->ctl_work_lock);
> +
> + /* The get/set_current_limit() callbacks have an active regulator core
/*
* The proper style of multi-line comment
* is depicted in this example. Use it.
*/
> + * reservation ID (obtained with ww_acquire_init()).
> + *
> + * Or, the IO channel driver may call something like
> + * regulator_enable(), meaning this thread would acquire a new
> + * regulator core reservation ID before the current one is dropped
> + * (using ww_acquire_fini()). This is forbidden.
> + *
> + * Thus, perform the IO channel read/write in a different thread, and
> + * wait for it to complete, with a timeout to avoid deadlocking.
> + */
> +
> + scoped_guard(mutex, &priv->ctl_work_lock) {
> + if (work_busy(&ctl_work->work))
> + return -EBUSY;
> +
> + if (write) {
> + ctl_work->ctl_val = *ctl_val;
> + ctl_work->write = 1;
> + } else {
> + ctl_work->write = 0;
> + }
> +
> + schedule_work(&ctl_work->work);
> + }
> +
> + ret = wait_for_completion_timeout(&priv->ctl_rw_done, LTM8054_CTL_RW_TIMEOUT);
> + reinit_completion(&priv->ctl_rw_done);
> +
> + if (unlikely(!ret))
> + return -ETIMEDOUT;
> +
> + scoped_guard(mutex, &priv->ctl_work_lock) {
> + ret = ctl_work->ret;
> + if (!ret && !write)
> + *ctl_val = ctl_work->ctl_val;
Return directly.
if (ret)
return ret;
if (!write)
...
> + }
> + return ret;
return 0;
> +}
...
> +static struct iio_channel *ltm8054_init_ctl_dac(struct platform_device *pdev)
> +{
> + struct iio_channel *ctl_dac;
> + enum iio_chan_type type;
> + int ret;
> +
> + ctl_dac = devm_iio_channel_get(&pdev->dev, "ctl");
> + if (IS_ERR(ctl_dac)) {
> + if (PTR_ERR(ctl_dac) == -ENODEV)
> + return ERR_PTR(-EPROBE_DEFER);
Hmm... Are you sure about this?
> +
> + return ctl_dac;
> + }
> +
> + ret = iio_get_channel_type(ctl_dac, &type);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (type != IIO_VOLTAGE)
> + return ERR_PTR(-EINVAL);
> +
> + return ctl_dac;
> +}
...
> static int ltm8054_probe(struct platform_device *pdev)
> {
> struct regulator_config config = { };
> + struct iio_channel *ctl_dac = NULL;
> struct device *dev = &pdev->dev;
> struct regulator_dev *rdev;
> struct ltm8054_priv *priv;
> int ret;
>
> + /* Do this first, as it might defer. */
> + if (device_property_match_string(dev, "io-channel-names", "ctl") >= 0) {
> + ctl_dac = ltm8054_init_ctl_dac(pdev);
> + if (IS_ERR(ctl_dac))
> + return dev_err_probe(dev, PTR_ERR(ctl_dac),
> + "failed to initialize CTL DAC\n");
> + }
> +
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> return -ENOMEM;
> + platform_set_drvdata(pdev, priv);
Do we need this? I think "no". See below how.
> + priv->dev = dev;
> priv->rdesc.name = "ltm8054-regulator";
> - priv->rdesc.ops = <m8054_regulator_ops;
> + priv->rdesc.ops = <m8054_no_ctl_ops;
> priv->rdesc.type = REGULATOR_VOLTAGE;
> priv->rdesc.owner = THIS_MODULE;
>
> + if (ctl_dac) {
> + priv->ctl_dac = ctl_dac;
> + INIT_WORK(&priv->ctl_work.work, ltm8054_do_ctl_work);
> + init_completion(&priv->ctl_rw_done);
Do devm-helpers.h APIs help with something here? Does
devm_add_action_or_reset() help with not covered cases?
> + mutex_init(&priv->ctl_work_lock);
Use devm_mutex_init() and don't forget the error check.
> + priv->rdesc.ops = <m8054_ctl_ops;
> + }
> +
> config.dev = dev;
> config.driver_data = priv;
>From this...
> ret = ltm8054_of_parse(dev, priv, &config);
> - if (ret)
> - return dev_err_probe(dev, ret, "failed to parse device tree\n");
> + if (ret) {
> + ret = dev_err_probe(dev, ret, "failed to parse device tree\n");
> + goto out_err;
> + }
>
> rdev = devm_regulator_register(dev, &priv->rdesc, &config);
> - if (IS_ERR(rdev))
> - return dev_err_probe(dev, PTR_ERR(rdev), "failed to register regulator\n");
> + if (IS_ERR(rdev)) {
> + ret = dev_err_probe(dev, PTR_ERR(rdev), "failed to register regulator\n");
> + goto out_err;
> + }
>
> return 0;
> +
> +out_err:
> + if (ctl_dac) {
> + cancel_work_sync(&priv->ctl_work.work);
> + mutex_destroy(&priv->ctl_work_lock);
> + }
> +
> + return ret;
> +}
> +
> +static void ltm8054_remove(struct platform_device *pdev)
> +{
> + struct ltm8054_priv *priv = platform_get_drvdata(pdev);
> +
> + if (priv->ctl_dac) {
> + cancel_work_sync(&priv->ctl_work.work);
> + mutex_destroy(&priv->ctl_work_lock);
> + }
> }
...to this no changes are needed.
...
> .probe = ltm8054_probe,
> + .remove = ltm8054_remove,
Neither is this.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists