1. Patchew
  2. linux
  3. [PATCH v4 0/6] iio: light: stk3310: support powering off during suspend
  4. View patch

[PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies

Aren Moynihan posted 6 patches 3 weeks ago
[PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies
Posted by Aren Moynihan 3 weeks ago 
The vdd and leda supplies must be powered on for the chip to function
and can be powered off during system suspend.

This was originally based on a patch by Ondrej Jirman[1], but has been
rewritten since.

1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82

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

Notes:
    Changes in v4:
     - fix variable declaration order in stk3310_resume to match the rest of
       the driver
    
    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 181b7acb3f96..f93689c61f44 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)
 
 	devm_mutex_init(&client->dev, &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;
+	int ret;
 	struct stk3310_data *data;
+	u8 state = 0;
 
 	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
Re: [PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies
Posted by Andy Shevchenko 2 weeks, 6 days ago 
On Sat, Nov 02, 2024 at 03:50:39PM -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.
> 
> This was originally based on a patch by Ondrej Jirman[1], but has been
> rewritten since.
> 
> 1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82

Make it a Link tag...

> 

...here

Link: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82 [1]
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>

...

> +	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");

With previously introduced temporary 'dev' variable these become:

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

...

> +	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");

So do these...

...

> +	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);

Is array_size.h included?

> +	if (ret) {
> +		dev_err(dev, "failed to disable regulators: %d\n", ret);
> +		return ret;
> +	}

...

> -	u8 state = 0;
> +	int ret;
>  	struct stk3310_data *data;
> +	u8 state = 0;

Can we try to make it RCT ordered?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies
Posted by Aren 1 week, 6 days ago 
On Mon, Nov 04, 2024 at 10:35:42AM +0200, Andy Shevchenko wrote:
> On Sat, Nov 02, 2024 at 03:50:39PM -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.
> > 
> > This was originally based on a patch by Ondrej Jirman[1], but has been
> > rewritten since.
> > 
> > 1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82
> 
> Make it a Link tag...
> 
> > 
> 
> ...here
> 
> Link: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82 [1]

Makes sense

> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> 
> ...
> 
> > +	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");
> 
> With previously introduced temporary 'dev' variable these become:
> 
> 	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies), data->supplies);
> 	if (ret)
> 		return dev_err_probe(dev, ret, "get regulators failed\n");
> 
> ...
> 
> > +	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");
> 
> So do these...
> 

Cleaning all of these up

> > +	ret = regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
> 
> Is array_size.h included?

It's not, it looks like it came one of the headers that's already
included. I'll add it explicitly.

Thanks
 - Aren
Re: [PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies
Posted by Jonathan Cameron 3 weeks ago 
On Sat,  2 Nov 2024 15:50:39 -0400
Aren Moynihan <aren@peacevolution.org> wrote:

> The vdd and leda supplies must be powered on for the chip to function
> and can be powered off during system suspend.
> 
> This was originally based on a patch by Ondrej Jirman[1], but has been
> rewritten since.
> 
> 1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82
> 
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> 
> Notes:
>     Changes in v4:
>      - fix variable declaration order in stk3310_resume to match the rest of
>        the driver

For this Andy was asking for consistency.  Generally we don't insist on a
particular ordering in IIO drivers, but we do prefer them to be the same.
Your new ordering is inconsistent between resume and suspend.  Whilst
existing code may be inconsistent, you can still pick most common ordering
and use that for your new code.

If the existing driver is inconsistent then feel free to tidy that up but
do it in a precursor patch so there is a consistent style for you to then
carry on.

>     
>     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 181b7acb3f96..f93689c61f44 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)
>  
>  	devm_mutex_init(&client->dev, &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;
> +	int ret;
>  	struct stk3310_data *data;
> +	u8 state = 0;
>  
>  	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)
Re: [PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies
Posted by Aren 3 weeks ago 
On Sun, Nov 03, 2024 at 11:31:03AM +0000, Jonathan Cameron wrote:
> On Sat,  2 Nov 2024 15:50:39 -0400
> Aren Moynihan <aren@peacevolution.org> wrote:
> 
> > The vdd and leda supplies must be powered on for the chip to function
> > and can be powered off during system suspend.
> > 
> > This was originally based on a patch by Ondrej Jirman[1], but has been
> > rewritten since.
> > 
> > 1: https://codeberg.org/megi/linux/commit/a933aff8b7a0e6e3c9cf1d832dcba07022bbfa82
> > 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> > ---
> > 
> > Notes:
> >     Changes in v4:
> >      - fix variable declaration order in stk3310_resume to match the rest of
> >        the driver
> 
> For this Andy was asking for consistency.  Generally we don't insist on a
> particular ordering in IIO drivers, but we do prefer them to be the same.
> Your new ordering is inconsistent between resume and suspend.  Whilst
> existing code may be inconsistent, you can still pick most common ordering
> and use that for your new code.
> 
> If the existing driver is inconsistent then feel free to tidy that up but
> do it in a precursor patch so there is a consistent style for you to then
> carry on.

Oh right, the order of declarations in stk3310_suspend also needs to be
flipped. Is that simple enough that you can fix it when applying this?

Apparently I was being dense, I checked the rest of the driver to see
what it did (it's consistent about putting shorter lines & ones without
an assignment first), and fixed the case Andy pointed out to match that,
but failed to check the rest of the patch.

Thanks
 - Aren
Re: [PATCH v4 3/6] iio: light: stk3310: Implement vdd and leda supplies
Posted by Andy Shevchenko 2 weeks, 6 days ago 
On Sun, Nov 03, 2024 at 11:11:13AM -0500, Aren wrote:
> On Sun, Nov 03, 2024 at 11:31:03AM +0000, Jonathan Cameron wrote:
> > On Sat,  2 Nov 2024 15:50:39 -0400
> > Aren Moynihan <aren@peacevolution.org> wrote:

...

> > For this Andy was asking for consistency.  Generally we don't insist on a
> > particular ordering in IIO drivers, but we do prefer them to be the same.
> > Your new ordering is inconsistent between resume and suspend.  Whilst
> > existing code may be inconsistent, you can still pick most common ordering
> > and use that for your new code.
> > 
> > If the existing driver is inconsistent then feel free to tidy that up but
> > do it in a precursor patch so there is a consistent style for you to then
> > carry on.
> 
> Oh right, the order of declarations in stk3310_suspend also needs to be
> flipped. Is that simple enough that you can fix it when applying this?
> 
> Apparently I was being dense, I checked the rest of the driver to see
> what it did (it's consistent about putting shorter lines & ones without
> an assignment first), and fixed the case Andy pointed out to match that,
> but failed to check the rest of the patch.

Thanks!

You may ignore my comment about RCT order if it's not that one that being
commonly used in the driver.

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.