Received-SPF: pass (google.com: domain of linux-kernel+bounces-161506-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 28 Apr 2024 17:53:37 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Aren Moynihan <aren@peacevolution.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
 <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec
 <jernej.skrabec@gmail.com>, Samuel Holland <samuel@sholland.org>, Liam
 Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, Andy
 Shevchenko <andy.shevchenko@gmail.com>, Ondrej Jirman <megi@xff.cz>, Uwe
 =?UTF-8?B?S2xlaW5lLUvDtm5pZw==?= <u.kleine-koenig@pengutronix.de>,
 linux-iio@vger.kernel.org, phone-devel@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, Willow
 Barraco <contact@willowbarraco.fr>
Subject: Re: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and
 power it off during suspend
Message-ID: <20240428175337.61850e2a@jic23-huawei>
In-Reply-To: <20240423223309.1468198-4-aren@peacevolution.org>
References: <20240423223309.1468198-2-aren@peacevolution.org>
	<20240423223309.1468198-4-aren@peacevolution.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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

> From: Ondrej Jirman <megi@xff.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@xff.cz>
> Signed-off-by: Aren Moynihan <aren@peacevolution.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)