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: <20240428175337.61850e2a@jic23-huawei>
Date: Sun, 28 Apr 2024 17:53:37 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Aren Moynihan <aren@...cevolution.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec
 <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, Liam
 Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Andy
 Shevchenko <andy.shevchenko@...il.com>, Ondrej Jirman <megi@....cz>, Uwe
 Kleine-König <u.kleine-koenig@...gutronix.de>,
 linux-iio@...r.kernel.org, phone-devel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev, Willow
 Barraco <contact@...lowbarraco.fr>
Subject: Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and
 power it off during suspend

On Tue, 23 Apr 2024 18:33:05 -0400
Aren Moynihan <aren@...cevolution.org> wrote:

> From: Ondrej Jirman <megi@....cz>
> 
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
> 
> Signed-off-by: Ondrej Jirman <megi@....cz>
> Signed-off-by: Aren Moynihan <aren@...cevolution.org>

Suggestions inline.  Key thing is take the whole thing devm_ managed
and your life gets much easier.  It is mixing the two approaches that
causes problems and often the best plan is to do everything in probe/remove
with devm_ calls to do the cleanup for you.

> ---
> 
> Notes:
>     Changes in v2:
>      - always enable / disable regulators and rely on a dummy regulator if
>        one isn't specified
>      - replace usleep_range with fsleep
>      - reorder includes so iio headers are last
>      - add missing error handling to resume
> 
>  drivers/iio/light/stk3310.c | 49 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 7b71ad71d78d..a0547eeca3e3 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -13,6 +13,8 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -117,6 +119,7 @@ struct stk3310_data {
>  	struct regmap_field *reg_int_ps;
>  	struct regmap_field *reg_flag_psint;
>  	struct regmap_field *reg_flag_nf;
> +	struct regulator *vdd_reg;
>  };
>  
>  static const struct iio_event_spec stk3310_events[] = {
> @@ -607,6 +610,10 @@ static int stk3310_probe(struct i2c_client *client)
>  
>  	mutex_init(&data->lock);
>  
> +	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(data->vdd_reg))
> +		return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
> +
>  	ret = stk3310_regmap_init(data);
>  	if (ret < 0)
>  		return ret;
> @@ -617,9 +624,17 @@ static int stk3310_probe(struct i2c_client *client)
>  	indio_dev->channels = stk3310_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
>  
> +	ret = regulator_enable(data->vdd_reg);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "regulator vdd enable failed\n");
> +
> +	/* we need a short delay to allow the chip time to power on */
> +	fsleep(1000);
> +
>  	ret = stk3310_init(indio_dev);
>  	if (ret < 0)
> -		return ret;
> +		goto err_vdd_disable;
>  
>  	if (client->irq > 0) {
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -645,32 +660,60 @@ static int stk3310_probe(struct i2c_client *client)
>  
>  err_standby:
>  	stk3310_set_state(data, STK3310_STATE_STANDBY);

Move this handling to a devm_add_action_or_reset() callback in a precursor patch.
That will fix the current ordering issue wrt to the irq registration.

Then use devm_iio_device_register() in that precursor patch.

> +err_vdd_disable:
> +	regulator_disable(data->vdd_reg);

Add a devm_add_action_or_reset() callback to disable this regulator in this patch.
Register that just after the enable.

That way the ordering will be maintained for all calls.
>  	return ret;
>  }
>  
>  static void stk3310_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct stk3310_data *data = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  	stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> +	regulator_disable(data->vdd_reg);

With above suggested changes, you can drop the remove function entirely.

>  }
>  
>  static int stk3310_suspend(struct device *dev)
>  {
>  	struct stk3310_data *data;
> +	int ret;
>  
>  	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>  
> -	return stk3310_set_state(data, STK3310_STATE_STANDBY);
> +	ret = stk3310_set_state(data, STK3310_STATE_STANDBY);
> +	if (ret)
> +		return ret;
> +
> +	regcache_mark_dirty(data->regmap);
> +	regulator_disable(data->vdd_reg);
> +
> +	return 0;
>  }
>  
>  static int stk3310_resume(struct device *dev)
>  {
> -	u8 state = 0;
>  	struct stk3310_data *data;
> +	u8 state = 0;
> +	int ret;
>  
>  	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> +
> +	ret = regulator_enable(data->vdd_reg);
> +	if (ret) {
> +		dev_err(dev, "Failed to re-enable regulator vdd\n");
> +		return ret;
> +	}
> +
> +	fsleep(1000);
> +
> +	ret = regcache_sync(data->regmap);
> +	if (ret) {
> +		dev_err(dev, "Failed to restore registers: %d\n", ret);
> +		return ret;
> +	}
> +
>  	if (data->ps_enabled)
>  		state |= STK3310_STATE_EN_PS;
>  	if (data->als_enabled)


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ