The vdd and leda supplies must be powered on for the chip to function
and can be powered off during system suspend.

Co-developed-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

Notes:
    I'm not sure what the proper way to handle attribution for this patch
    is. It was origionally based on a patch by Ondrej Jirman[1], but I have
    rewritten a large portion if it. I have included a Co-developed-by tag
    to indicate this, but haven't sent him this patch, so I'm not sure what
    to do about a Signed-off-by.
    
    1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82
    
    Changes in v3:
     - use bulk regulators instead of two individual ones
     - handle cleanup using devm callbacks instead of the remove function
    
    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 | 76 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 2e1aa551bdbc..f02b4d20c282 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>
@@ -130,6 +132,7 @@ struct stk3310_data {
 	struct regmap_field *reg_int_ps;
 	struct regmap_field *reg_flag_psint;
 	struct regmap_field *reg_flag_nf;
+	struct regulator_bulk_data supplies[2];
 };
 
 static const struct iio_event_spec stk3310_events[] = {
@@ -621,6 +624,31 @@ static irqreturn_t stk3310_irq_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static int stk3310_regulators_enable(struct stk3310_data *data)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret)
+		return ret;
+
+	/* we need a short delay to allow the chip time to power on */
+	fsleep(1000);
+
+	return 0;
+}
+
+static void stk3310_regulators_disable(void *private)
+{
+	int ret;
+	struct stk3310_data *data = private;
+	struct device *dev = &data->client->dev;
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret)
+		dev_err(dev, "failed to disable regulators: %d\n", ret);
+}
+
 static int stk3310_probe(struct i2c_client *client)
 {
 	int ret;
@@ -642,6 +670,13 @@ static int stk3310_probe(struct i2c_client *client)
 
 	mutex_init(&data->lock);
 
+	data->supplies[0].supply = "vdd";
+	data->supplies[1].supply = "leda";
+	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "get regulators failed\n");
+
 	ret = stk3310_regmap_init(data);
 	if (ret < 0)
 		return ret;
@@ -652,6 +687,16 @@ static int stk3310_probe(struct i2c_client *client)
 	indio_dev->channels = stk3310_channels;
 	indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
 
+	ret = stk3310_regulators_enable(data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "regulator enable failed\n");
+
+	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to register regulator cleanup\n");
+
 	ret = stk3310_init(indio_dev);
 	if (ret < 0)
 		return ret;
@@ -682,18 +727,45 @@ static int stk3310_probe(struct i2c_client *client)
 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);
+
+	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret) {
+		dev_err(dev, "failed to disable regulators: %d\n", ret);
+		return ret;
+	}
+
+	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 = stk3310_regulators_enable(data);
+	if (ret) {
+		dev_err(dev, "Failed to re-enable regulators: %d", ret);
+		return ret;
+	}
+
+	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)
-- 
2.47.0

On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:
> The vdd and leda supplies must be powered on for the chip to function
> and can be powered off during system suspend.
> 
> Co-developed-by: Ondrej Jirman <megi@xff.cz>

Missing SoB. Please, read Submitting Patches documentation for understanding
what has to be done here.

> Signed-off-by: Aren Moynihan <aren@peacevolution.org>

...

> Notes:
>     I'm not sure what the proper way to handle attribution for this patch
>     is. It was origionally based on a patch by Ondrej Jirman[1], but I have
>     rewritten a large portion if it. I have included a Co-developed-by tag
>     to indicate this, but haven't sent him this patch, so I'm not sure what
>     to do about a Signed-off-by.

Ah, seems you already aware of this issue. So, either drop Co-developed-by
(and if you wish you may give a credit in a free form inside commit message)
or make sure you get his SoB tag.

...

>  	mutex_init(&data->lock);

Somewhere (in the previous patch?) you want to switch to devm_mutex_init().

...

> +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
> +				      data->supplies);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "get regulators failed\n");

> +		return dev_err_probe(&client->dev, ret,
> +				     "regulator enable failed\n");

> +	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to register regulator cleanup\n");

With

	struct devuce *dev = &client->dev;

at the top of the function makes these and more lines neater.

...

>  static int stk3310_resume(struct device *dev)
>  {
> -	u8 state = 0;
>  	struct stk3310_data *data;
> +	u8 state = 0;
> +	int ret;

While changing to RCT order here, it seems you have inconsistent approach
elsewhere (in your own patches!). Please, be consistent with chosen style.

>  	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));

-- 
With Best Regards,
Andy Shevchenko

Hello Aren,

On 2024-10-28 15:38, Andy Shevchenko wrote:
> On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:
>> The vdd and leda supplies must be powered on for the chip to function
>> and can be powered off during system suspend.
>> 
>> Co-developed-by: Ondrej Jirman <megi@xff.cz>
> 
> Missing SoB. Please, read Submitting Patches documentation for 
> understanding
> what has to be done here.
> 
>> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> ...
> 
>> Notes:
>>     I'm not sure what the proper way to handle attribution for this 
>> patch
>>     is. It was origionally based on a patch by Ondrej Jirman[1], but I 
>> have
>>     rewritten a large portion if it. I have included a Co-developed-by 
>> tag
>>     to indicate this, but haven't sent him this patch, so I'm not sure 
>> what
>>     to do about a Signed-off-by.
> 
> Ah, seems you already aware of this issue. So, either drop 
> Co-developed-by
> (and if you wish you may give a credit in a free form inside commit 
> message)
> or make sure you get his SoB tag.

Perhaps the best and also easiest solution would be to provide an
Originally-by tag for Ondrej, because that's what it is.  The patch
was written originally by Ondrej, but you've changed many parts of
the patch while upstreaming it.

On Mon, Oct 28, 2024 at 04:38:37PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:
> > The vdd and leda supplies must be powered on for the chip to function
> > and can be powered off during system suspend.
> > 
> > Co-developed-by: Ondrej Jirman <megi@xff.cz>
> 
> Missing SoB. Please, read Submitting Patches documentation for understanding
> what has to be done here.
> 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> ...
> 
> > Notes:
> >     I'm not sure what the proper way to handle attribution for this patch
> >     is. It was origionally based on a patch by Ondrej Jirman[1], but I have
> >     rewritten a large portion if it. I have included a Co-developed-by tag
> >     to indicate this, but haven't sent him this patch, so I'm not sure what
> >     to do about a Signed-off-by.
> 
> Ah, seems you already aware of this issue. So, either drop Co-developed-by
> (and if you wish you may give a credit in a free form inside commit message)
> or make sure you get his SoB tag.

Alright, thanks for clarifying that.

> >  	mutex_init(&data->lock);
> 
> Somewhere (in the previous patch?) you want to switch to devm_mutex_init().

Good catch, it looks like that was being leaked before this refactor.
Yeah that sounds like the right place, I'll include it in v4.

> > +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
> > +				      data->supplies);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "get regulators failed\n");
> 
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "regulator enable failed\n");
> 
> > +	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "failed to register regulator cleanup\n");
> 
> With
> 
> 	struct devuce *dev = &client->dev;
> 
> at the top of the function makes these and more lines neater.
> 
[snip]
> 
> While changing to RCT order here, it seems you have inconsistent approach
> elsewhere (in your own patches!). Please, be consistent with chosen style.

Sounds easy enough to fix, I'll include these in v4.

Thanks taking the time to review
 - Aren

On Mon, 28 Oct 2024 12:37:14 -0400
Aren Moynihan <aren@peacevolution.org> wrote:

> On Mon, Oct 28, 2024 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > On Mon, Oct 28, 2024 at 10:19:57AM -0400, Aren Moynihan wrote:  
> > > The vdd and leda supplies must be powered on for the chip to function
> > > and can be powered off during system suspend.
> > > 
> > > Co-developed-by: Ondrej Jirman <megi@xff.cz>  
> > 
> > Missing SoB. Please, read Submitting Patches documentation for understanding
> > what has to be done here.
> >   
> > > Signed-off-by: Aren Moynihan <aren@peacevolution.org>  
> > 
> > ...
> >   
> > > Notes:
> > >     I'm not sure what the proper way to handle attribution for this patch
> > >     is. It was origionally based on a patch by Ondrej Jirman[1], but I have
> > >     rewritten a large portion if it. I have included a Co-developed-by tag
> > >     to indicate this, but haven't sent him this patch, so I'm not sure what
> > >     to do about a Signed-off-by.  
> > 
> > Ah, seems you already aware of this issue. So, either drop Co-developed-by
> > (and if you wish you may give a credit in a free form inside commit message)
> > or make sure you get his SoB tag.  
> 
> Alright, thanks for clarifying that.
> 
> > >  	mutex_init(&data->lock);  
> > 
> > Somewhere (in the previous patch?) you want to switch to devm_mutex_init().  
> 
> Good catch, it looks like that was being leaked before this refactor.
> Yeah that sounds like the right place, I'll include it in v4.
Not really on the leaking.  Take a look at the cleanup for devm_mutex_init().
It's debug only and not all that useful in most cases.

However, it is good to not assume that now we have a devm_mutex_init()
available that is easy to use.

> 
> > > +	ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->supplies),
> > > +				      data->supplies);
> > > +	if (ret)
> > > +		return dev_err_probe(&client->dev, ret, "get regulators failed\n");  
> >   
> > > +		return dev_err_probe(&client->dev, ret,
> > > +				     "regulator enable failed\n");  
> >   
> > > +	ret = devm_add_action_or_reset(&client->dev, stk3310_regulators_disable, data);
> > > +	if (ret)
> > > +		return dev_err_probe(&client->dev, ret,
> > > +				     "failed to register regulator cleanup\n");  
> > 
> > With
> > 
> > 	struct devuce *dev = &client->dev;
> > 
> > at the top of the function makes these and more lines neater.
> >   
> [snip]
> > 
> > While changing to RCT order here, it seems you have inconsistent approach
> > elsewhere (in your own patches!). Please, be consistent with chosen style.  
> 
> Sounds easy enough to fix, I'll include these in v4.
> 
> Thanks taking the time to review
>  - Aren