[PATCH v4 0/6] iio: light: stk3310: support powering off during suspend

[PATCH v4 0/6] iio: light: stk3310: support powering off during suspend

[Aren Moynihan]

In the Pine64 PinePhone, the stk3310 chip is powered by a regulator that is
disabled at system boot and can be shut off during suspend. To ensure that
the chip properly initializes, both after boot and suspend, we need to
manage this regulator.

Additionally if the chip is shut off in suspend, we need to make sure that
it gets reinitialized with the same parameters after resume.

Major changes in v4:
 - replace mutex_init with devm_mutex_init
 - code style cleanup

Major changes in v3:
 - Use bulk regulators instead of two individual ones
 - Replace stk3310_remove with devm callbacks

Major changes in v2:
 - Add handling of the IR LED. I was hesitant to include this as it is the
   same as pull-up regulator for the i2c bus on the hardware I have, so I
   can't test it well. I think leaving it out is more likely to cause
   issues than including it.
 - Convert stk3310 to use dev_err_probe for errors.
 - Always enable / disable regulators and rely on dummy devices if they're
   not specified.
 - more listed in individual patches

Aren Moynihan (5):
  dt-bindings: iio: light: stk33xx: add vdd and leda regulators
  iio: light: stk3310: handle all remove logic with devm callbacks
  iio: light: stk3310: Implement vdd and leda supplies
  iio: light: stk3310: use dev_err_probe where possible
  iio: light: stk3310: log error if reading the chip id fails

Ondrej Jirman (1):
  arm64: dts: allwinner: pinephone: Add power supplies to stk3311

 .../bindings/iio/light/stk33xx.yaml           |   4 +
 .../dts/allwinner/sun50i-a64-pinephone.dtsi   |   2 +
 drivers/iio/light/stk3310.c                   | 162 ++++++++++++------
 3 files changed, 120 insertions(+), 48 deletions(-)

-- 
2.47.0

[PATCH v4 1/6] dt-bindings: iio: light: stk33xx: add vdd and leda regulators

[Aren Moynihan]

stk3310 and stk3311 are typically connected to power supplies for the
chip (vdd) and the infrared LED (leda). Add properties so we can power
these up / down appropriately.

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Notes:
    Changes in v2:
     - add leda-supply
     - add supplies to examples

 Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
index e4341fdced98..96ee8ec16463 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -34,6 +34,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  vdd-supply: true
+  leda-supply: true
   proximity-near-level: true
 
 required:
@@ -57,6 +59,8 @@ examples:
                 proximity-near-level = <25>;
                 interrupt-parent = <&gpio1>;
                 interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
+                vdd-supply = <&vdd_regulator>;
+                leda-supply = <&led_regulator>;
         };
     };
 ...
-- 
2.47.0

[PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Aren Moynihan]

Using devm callbacks helps to make the ordering of probe / remove
operations easier to reason about and removes some duplicate code
between the probe error path and driver remove.
---

Notes:
    Changes in v4:
     - also replace mutex_init with devm_mutex_init
    
    Added in v3

 drivers/iio/light/stk3310.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index ed20b6714546..181b7acb3f96 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -484,6 +484,17 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state)
 	return ret;
 }
 
+static void stk3310_set_state_disable(void *private)
+{
+	int ret;
+	struct stk3310_data *data = private;
+	struct device *dev = &data->client->dev;
+
+	ret = stk3310_set_state(data, STK3310_STATE_STANDBY);
+	if (ret)
+		dev_err(dev, "failed to set state to standby: %d\n", ret);
+}
+
 static int stk3310_init(struct iio_dev *indio_dev)
 {
 	int ret;
@@ -491,6 +502,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
 	u8 state;
 	struct stk3310_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 
 	ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
 	if (ret < 0)
@@ -507,6 +519,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register cleanup function\n");
+
 	/* Enable PS interrupts */
 	ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN);
 	if (ret < 0)
@@ -624,7 +640,7 @@ static int stk3310_probe(struct i2c_client *client)
 	device_property_read_u32(&client->dev, "proximity-near-level",
 				 &data->ps_near_level);
 
-	mutex_init(&data->lock);
+	devm_mutex_init(&client->dev, &data->lock);
 
 	ret = stk3310_regmap_init(data);
 	if (ret < 0)
@@ -650,29 +666,17 @@ static int stk3310_probe(struct i2c_client *client)
 		if (ret < 0) {
 			dev_err(&client->dev, "request irq %d failed\n",
 				client->irq);
-			goto err_standby;
+			return ret;
 		}
 	}
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&client->dev, indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "device_register failed\n");
-		goto err_standby;
+		return ret;
 	}
 
 	return 0;
-
-err_standby:
-	stk3310_set_state(data, STK3310_STATE_STANDBY);
-	return ret;
-}
-
-static void stk3310_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	iio_device_unregister(indio_dev);
-	stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
 }
 
 static int stk3310_suspend(struct device *dev)
@@ -736,7 +740,6 @@ static struct i2c_driver stk3310_driver = {
 		.acpi_match_table = stk3310_acpi_id,
 	},
 	.probe =        stk3310_probe,
-	.remove =           stk3310_remove,
 	.id_table =         stk3310_i2c_id,
 };
 
-- 
2.47.0

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Jonathan Cameron]

Hi Aren,

> @@ -624,7 +640,7 @@ static int stk3310_probe(struct i2c_client *client)
>  	device_property_read_u32(&client->dev, "proximity-near-level",
>  				 &data->ps_near_level);
>  
> -	mutex_init(&data->lock);
> +	devm_mutex_init(&client->dev, &data->lock);
ret = devm_mutex_init()
if (ret)
	return ret;

It is very unlikely to fail but technically it can.  Andy has been fixing
this up across the kernel (including IIO) so let's not introduce another
case that doesn't check it!

If nothing else comes up I can probably tidy that up whilst applying.

Jonathan

>  
>  	ret = stk3310_regmap_init(data);
>  	if (ret < 0)
> @@ -650,29 +666,17 @@ static int stk3310_probe(struct i2c_client *client)
>  		if (ret < 0) {
>  			dev_err(&client->dev, "request irq %d failed\n",
>  				client->irq);
> -			goto err_standby;
> +			return ret;
>  		}
>  	}
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "device_register failed\n");
> -		goto err_standby;
> +		return ret;
>  	}
>  
>  	return 0;
> -
> -err_standby:
> -	stk3310_set_state(data, STK3310_STATE_STANDBY);
> -	return ret;
> -}
> -
> -static void stk3310_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -
> -	iio_device_unregister(indio_dev);
> -	stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
>  }
>  
>  static int stk3310_suspend(struct device *dev)
> @@ -736,7 +740,6 @@ static struct i2c_driver stk3310_driver = {
>  		.acpi_match_table = stk3310_acpi_id,
>  	},
>  	.probe =        stk3310_probe,
> -	.remove =           stk3310_remove,
>  	.id_table =         stk3310_i2c_id,
>  };
>  

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Aren]

On Sun, Nov 03, 2024 at 11:22:08AM +0000, Jonathan Cameron wrote:
> Hi Aren,
> 
> > @@ -624,7 +640,7 @@ static int stk3310_probe(struct i2c_client *client)
> >  	device_property_read_u32(&client->dev, "proximity-near-level",
> >  				 &data->ps_near_level);
> >  
> > -	mutex_init(&data->lock);
> > +	devm_mutex_init(&client->dev, &data->lock);
> ret = devm_mutex_init()
> if (ret)
> 	return ret;
> 
> It is very unlikely to fail but technically it can.  Andy has been fixing
> this up across the kernel (including IIO) so let's not introduce another
> case that doesn't check it!

Right, I'll take this as my periodic reminder to read the docs / types
more carefully :)

> If nothing else comes up I can probably tidy that up whilst applying.

That would be great

Thanks
 - Aren

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Andy Shevchenko]

On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote:
> Using devm callbacks helps to make the ordering of probe / remove
> operations easier to reason about and removes some duplicate code
> between the probe error path and driver remove.

Where is SoB?

...

> +	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);

Why not simply 'dev' as in below call?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register cleanup function\n");

...

> -	mutex_init(&data->lock);
> +	devm_mutex_init(&client->dev, &data->lock);

Missed error check, otherwise what's the point?


Also can add a temporary variable for 'dev'.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Aren]

On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote:
> On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote:
> > Using devm callbacks helps to make the ordering of probe / remove
> > operations easier to reason about and removes some duplicate code
> > between the probe error path and driver remove.
> 
> Where is SoB?

Oops that got lost in a rebase

> ...
> 
> > +	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
> 
> Why not simply 'dev' as in below call?

I was trying to avoid refactoring the entire function to replace
&client->dev with dev, I'll add a patch for that to the next revision.

> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register cleanup function\n");
> 
> ...
> 
> > -	mutex_init(&data->lock);
> > +	devm_mutex_init(&client->dev, &data->lock);
> 
> Missed error check, otherwise what's the point?
> 
> 
> Also can add a temporary variable for 'dev'.

Yup, fixing... I need to read the docs / function type more carefully
sometimes.

Thanks
 - Aren

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Andy Shevchenko]

Sun, Nov 10, 2024 at 01:38:39PM -0500, Aren kirjoitti:
> On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote:
> > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote:

...

> > > +	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
> > 
> > Why not simply 'dev' as in below call?
> 
> I was trying to avoid refactoring the entire function to replace
> &client->dev with dev, I'll add a patch for that to the next revision.

I'm not talking about refactoring, I'm talking only about the lines that you
have touched / added.

> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to register cleanup function\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Aren]

On Sun, Nov 10, 2024 at 09:51:04PM +0200, Andy Shevchenko wrote:
> Sun, Nov 10, 2024 at 01:38:39PM -0500, Aren kirjoitti:
> > On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote:
> > > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote:
> 
> ...
> 
> > > > +	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
> > > 
> > > Why not simply 'dev' as in below call?
> > 
> > I was trying to avoid refactoring the entire function to replace
> > &client->dev with dev, I'll add a patch for that to the next revision.
> 
> I'm not talking about refactoring, I'm talking only about the lines that you
> have touched / added.

Ah right, this one makes sense, my comment should have been on the next
patch in this series which is a little more complex. For that patch it
seemed inconsistent to use dev only in new code and mix it with calls
using &client->dev.

 - Aren

Re: [PATCH v4 2/6] iio: light: stk3310: handle all remove logic with devm callbacks

[Andy Shevchenko]

On Sun, Nov 10, 2024 at 05:37:56PM -0500, Aren wrote:
> On Sun, Nov 10, 2024 at 09:51:04PM +0200, Andy Shevchenko wrote:
> > Sun, Nov 10, 2024 at 01:38:39PM -0500, Aren kirjoitti:
> > > On Mon, Nov 04, 2024 at 10:32:08AM +0200, Andy Shevchenko wrote:
> > > > On Sat, Nov 02, 2024 at 03:50:37PM -0400, Aren Moynihan wrote:

...

> > > > > +	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
> > > > 
> > > > Why not simply 'dev' as in below call?
> > > 
> > > I was trying to avoid refactoring the entire function to replace
> > > &client->dev with dev, I'll add a patch for that to the next revision.
> > 
> > I'm not talking about refactoring, I'm talking only about the lines that you
> > have touched / added.
> 
> Ah right, this one makes sense, my comment should have been on the next
> patch in this series which is a little more complex. For that patch it
> seemed inconsistent to use dev only in new code and mix it with calls
> using &client->dev.

It's fine, you can add a new cleanup patch later on.

-- 
With Best Regards,
Andy Shevchenko

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

[Aren Moynihan]

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

[Jonathan Cameron]

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

[Aren]

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

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

[Aren]

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

[PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Aren Moynihan]

Using dev_err_probe instead of dev_err and return makes the errors
easier to understand by including the error name, and saves a little
code.

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

Notes:
    Changes in v4:
     - Get a struct device ahead of time so it can be passed as "dev"
       instead of "&client->dev"
    
    No changes in v3
    
    Added in v2

 drivers/iio/light/stk3310.c | 61 ++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index f93689c61f44..c9a3f02bdd80 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -61,12 +61,12 @@
 #define STK3310_REGFIELD(name)						    \
 	do {								    \
 		data->reg_##name =					    \
-			devm_regmap_field_alloc(&client->dev, regmap,	    \
+			devm_regmap_field_alloc(dev, regmap,		    \
 				stk3310_reg_field_##name);		    \
-		if (IS_ERR(data->reg_##name)) {				    \
-			dev_err(&client->dev, "reg field alloc failed.\n"); \
-			return PTR_ERR(data->reg_##name);		    \
-		}							    \
+		if (IS_ERR(data->reg_##name))				    \
+			return dev_err_probe(dev,			    \
+				PTR_ERR(data->reg_##name),		    \
+				"reg field alloc failed.\n");		    \
 	} while (0)
 
 static const struct reg_field stk3310_reg_field_state =
@@ -517,10 +517,8 @@ static int stk3310_init(struct iio_dev *indio_dev)
 
 	state = STK3310_STATE_EN_ALS | STK3310_STATE_EN_PS;
 	ret = stk3310_set_state(data, state);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to enable sensor");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to enable sensor\n");
 
 	ret = devm_add_action_or_reset(&client->dev, stk3310_set_state_disable, data);
 	if (ret)
@@ -529,9 +527,9 @@ static int stk3310_init(struct iio_dev *indio_dev)
 	/* Enable PS interrupts */
 	ret = regmap_field_write(data->reg_int_ps, STK3310_PSINT_EN);
 	if (ret < 0)
-		dev_err(&client->dev, "failed to enable interrupts!\n");
+		return dev_err_probe(dev, ret, "failed to enable interrupts!\n");
 
-	return ret;
+	return 0;
 }
 
 static bool stk3310_is_volatile_reg(struct device *dev, unsigned int reg)
@@ -560,14 +558,14 @@ static const struct regmap_config stk3310_regmap_config = {
 static int stk3310_regmap_init(struct stk3310_data *data)
 {
 	struct regmap *regmap;
-	struct i2c_client *client;
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
 
-	client = data->client;
 	regmap = devm_regmap_init_i2c(client, &stk3310_regmap_config);
-	if (IS_ERR(regmap)) {
-		dev_err(&client->dev, "regmap initialization failed.\n");
-		return PTR_ERR(regmap);
-	}
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap initialization failed.\n");
+
 	data->regmap = regmap;
 
 	STK3310_REGFIELD(state);
@@ -654,12 +652,11 @@ static int stk3310_probe(struct i2c_client *client)
 	int ret;
 	struct iio_dev *indio_dev;
 	struct stk3310_data *data;
+	struct device *dev = &client->dev;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
-	if (!indio_dev) {
-		dev_err(&client->dev, "iio allocation failed!\n");
-		return -ENOMEM;
-	}
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM, "iio allocation failed!\n");
 
 	data = iio_priv(indio_dev);
 	data->client = client;
@@ -675,7 +672,7 @@ static int stk3310_probe(struct i2c_client *client)
 	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(dev, ret, "get regulators failed\n");
 
 	ret = stk3310_regmap_init(data);
 	if (ret < 0)
@@ -689,13 +686,11 @@ static int stk3310_probe(struct i2c_client *client)
 
 	ret = stk3310_regulators_enable(data);
 	if (ret)
-		return dev_err_probe(&client->dev, ret,
-				     "regulator enable failed\n");
+		return dev_err_probe(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");
+		return dev_err_probe(dev, ret, "failed to register regulator cleanup\n");
 
 	ret = stk3310_init(indio_dev);
 	if (ret < 0)
@@ -708,18 +703,14 @@ static int stk3310_probe(struct i2c_client *client)
 						IRQF_TRIGGER_FALLING |
 						IRQF_ONESHOT,
 						STK3310_EVENT, indio_dev);
-		if (ret < 0) {
-			dev_err(&client->dev, "request irq %d failed\n",
-				client->irq);
-			return ret;
-		}
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "request irq %d failed\n",
+					     client->irq);
 	}
 
 	ret = devm_iio_device_register(&client->dev, indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "device_register failed\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "device_register failed\n");
 
 	return 0;
 }
-- 
2.47.0

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko]

On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote:
> Using dev_err_probe instead of dev_err and return makes the errors
> easier to understand by including the error name, and saves a little
> code.

...

>  #define STK3310_REGFIELD(name)						    \
>  	do {								    \
>  		data->reg_##name =					    \
> -			devm_regmap_field_alloc(&client->dev, regmap,	    \
> +			devm_regmap_field_alloc(dev, regmap,		    \
>  				stk3310_reg_field_##name);		    \
> -		if (IS_ERR(data->reg_##name)) {				    \
> -			dev_err(&client->dev, "reg field alloc failed.\n"); \
> -			return PTR_ERR(data->reg_##name);		    \
> -		}							    \
> +		if (IS_ERR(data->reg_##name))				    \

> +			return dev_err_probe(dev,			    \
> +				PTR_ERR(data->reg_##name),		    \

AFAICS these two can be put on one.

> +				"reg field alloc failed.\n");		    \
>  	} while (0)


...

> @@ -654,12 +652,11 @@ static int stk3310_probe(struct i2c_client *client)
>  	int ret;
>  	struct iio_dev *indio_dev;
>  	struct stk3310_data *data;
> +	struct device *dev = &client->dev;

This should has been done a few patches earlier...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Aren]

On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote:
> On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote:
> > Using dev_err_probe instead of dev_err and return makes the errors
> > easier to understand by including the error name, and saves a little
> > code.
> 
> ...
> 
> >  #define STK3310_REGFIELD(name)						    \
> >  	do {								    \
> >  		data->reg_##name =					    \
> > -			devm_regmap_field_alloc(&client->dev, regmap,	    \
> > +			devm_regmap_field_alloc(dev, regmap,		    \
> >  				stk3310_reg_field_##name);		    \
> > -		if (IS_ERR(data->reg_##name)) {				    \
> > -			dev_err(&client->dev, "reg field alloc failed.\n"); \
> > -			return PTR_ERR(data->reg_##name);		    \
> > -		}							    \
> > +		if (IS_ERR(data->reg_##name))				    \
> 
> > +			return dev_err_probe(dev,			    \
> > +				PTR_ERR(data->reg_##name),		    \
> 
> AFAICS these two can be put on one.

This doesn't leave room for whitespace between the end of line and "\",
replacing "do { } while (0)" with "({ })" and deindenting could make
enough room to clean this up the formatting of this macro though.

> > +				"reg field alloc failed.\n");		    \
> >  	} while (0)
> 
> 
> ...
> 
> > @@ -654,12 +652,11 @@ static int stk3310_probe(struct i2c_client *client)
> >  	int ret;
> >  	struct iio_dev *indio_dev;
> >  	struct stk3310_data *data;
> > +	struct device *dev = &client->dev;
> 
> This should has been done a few patches earlier...

Moving it there now

 - Aren

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko]

Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote:
> > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote:

...

> > >  #define STK3310_REGFIELD(name)						    \
> > >  	do {								    \
> > >  		data->reg_##name =					    \
> > > -			devm_regmap_field_alloc(&client->dev, regmap,	    \
> > > +			devm_regmap_field_alloc(dev, regmap,		    \
> > >  				stk3310_reg_field_##name);		    \
> > > -		if (IS_ERR(data->reg_##name)) {				    \
> > > -			dev_err(&client->dev, "reg field alloc failed.\n"); \
> > > -			return PTR_ERR(data->reg_##name);		    \
> > > -		}							    \
> > > +		if (IS_ERR(data->reg_##name))				    \
> > 
> > > +			return dev_err_probe(dev,			    \
> > > +				PTR_ERR(data->reg_##name),		    \
> > 
> > AFAICS these two can be put on one.
> 
> This doesn't leave room for whitespace between the end of line and "\",

Is it a problem?

> replacing "do { } while (0)" with "({ })" and deindenting could make
> enough room to clean this up the formatting of this macro though.

do {} while (0) is C standard, ({}) is not.

> > > +				"reg field alloc failed.\n");		    \
> > >  	} while (0)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Aren]

On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> > On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote:
> > > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote:
> 
> ...
> 
> > > >  #define STK3310_REGFIELD(name)						    \
> > > >  	do {								    \
> > > >  		data->reg_##name =					    \
> > > > -			devm_regmap_field_alloc(&client->dev, regmap,	    \
> > > > +			devm_regmap_field_alloc(dev, regmap,		    \
> > > >  				stk3310_reg_field_##name);		    \
> > > > -		if (IS_ERR(data->reg_##name)) {				    \
> > > > -			dev_err(&client->dev, "reg field alloc failed.\n"); \
> > > > -			return PTR_ERR(data->reg_##name);		    \
> > > > -		}							    \
> > > > +		if (IS_ERR(data->reg_##name))				    \
> > > 
> > > > +			return dev_err_probe(dev,			    \
> > > > +				PTR_ERR(data->reg_##name),		    \
> > > 
> > > AFAICS these two can be put on one.
> > 
> > This doesn't leave room for whitespace between the end of line and "\",
> 
> Is it a problem?

It feels a bit camped and not as readable to me:

#define STK3310_REGFIELD(name)						    \
	do {								    \
		data->reg_##name =					    \
			devm_regmap_field_alloc(dev, regmap,		    \
				stk3310_reg_field_##name);		    \
		if (IS_ERR(data->reg_##name))				    \
			return dev_err_probe(dev, PTR_ERR(data->reg_##name),\
					     "reg field alloc failed.\n");  \
	} while (0)

Removing a level of indentation makes it much better

#define STK3310_REGFIELD(name) ({						\
	data->reg_##name = devm_regmap_field_alloc(dev, regmap,			\
						   stk3310_reg_field_##name);   \
	if (IS_ERR(data->reg_##name))						\
		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
				     "reg field alloc failed\n");		\
})

> > replacing "do { } while (0)" with "({ })" and deindenting could make
> > enough room to clean this up the formatting of this macro though.
> 
> do {} while (0) is C standard, ({}) is not.

({ }) is used throughout the kernel, and is documented as such[1]. I
don't see a reason to avoid it, if it helps readability.

1: the "GNU Extensions" section of Documentation/kernel-hacking/hacking.rst

 - Aren

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko]

On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> > > On Mon, Nov 04, 2024 at 10:40:16AM +0200, Andy Shevchenko wrote:
> > > > On Sat, Nov 02, 2024 at 03:50:41PM -0400, Aren Moynihan wrote:

...

> > > > >  #define STK3310_REGFIELD(name)						    \
> > > > >  	do {								    \
> > > > >  		data->reg_##name =					    \
> > > > > -			devm_regmap_field_alloc(&client->dev, regmap,	    \
> > > > > +			devm_regmap_field_alloc(dev, regmap,		    \
> > > > >  				stk3310_reg_field_##name);		    \
> > > > > -		if (IS_ERR(data->reg_##name)) {				    \
> > > > > -			dev_err(&client->dev, "reg field alloc failed.\n"); \
> > > > > -			return PTR_ERR(data->reg_##name);		    \
> > > > > -		}							    \
> > > > > +		if (IS_ERR(data->reg_##name))				    \
> > > > 
> > > > > +			return dev_err_probe(dev,			    \
> > > > > +				PTR_ERR(data->reg_##name),		    \
> > > > 
> > > > AFAICS these two can be put on one.
> > > 
> > > This doesn't leave room for whitespace between the end of line and "\",
> > 
> > Is it a problem?
> 
> It feels a bit camped and not as readable to me:
> 
> #define STK3310_REGFIELD(name)						    \
> 	do {								    \
> 		data->reg_##name =					    \
> 			devm_regmap_field_alloc(dev, regmap,		    \
> 				stk3310_reg_field_##name);		    \
> 		if (IS_ERR(data->reg_##name))				    \
> 			return dev_err_probe(dev, PTR_ERR(data->reg_##name),\
> 					     "reg field alloc failed.\n");  \
> 	} while (0)

Rather this way (besides the fact of having spaces instead of TABs for
the last formatting, in such case you even can simply add yet another
column with spaces):

#define STK3310_REGFIELD(name)								\
	do {										\
		data->reg_##name =							\
			devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name);	\
		if (IS_ERR(data->reg_##name))						\
			return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
					     "reg field alloc failed.\n");		\
	} while (0)

> Removing a level of indentation makes it much better

You can do it differently

#define STK3310_REGFIELD(name)							\
do {										\
	data->reg_##name =							\
		devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name);	\
	if (IS_ERR(data->reg_##name))						\
		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
				     "reg field alloc failed.\n");		\
} while (0)

> #define STK3310_REGFIELD(name) ({						\
> 	data->reg_##name = devm_regmap_field_alloc(dev, regmap,			\
> 						   stk3310_reg_field_##name);   \
> 	if (IS_ERR(data->reg_##name))						\
> 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> 				     "reg field alloc failed\n");		\
> })

I am against unneeded use of GNU extensions.

> > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > enough room to clean this up the formatting of this macro though.
> > 
> > do {} while (0) is C standard, ({}) is not.
> 
> ({ }) is used throughout the kernel, and is documented as such[1]. I
> don't see a reason to avoid it, if it helps readability.

I don't see how it makes things better here, and not everybody is familiar with
the concept even if it's used in the kernel here and there. Also if a tool is
being used in one case it doesn't mean it's suitable for another.

> 1: the "GNU Extensions" section of Documentation/kernel-hacking/hacking.rst

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

Hello Andy, hello Aren,

On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:
> On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> 
> You can do it differently
> 
> #define STK3310_REGFIELD(name)							\
> do {										\
> 	data->reg_##name =							\
> 		devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name);	\
> 	if (IS_ERR(data->reg_##name))						\
> 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> 				     "reg field alloc failed.\n");		\
> } while (0)
> 
> > #define STK3310_REGFIELD(name) ({						\
> > 	data->reg_##name = devm_regmap_field_alloc(dev, regmap,			\
> > 						   stk3310_reg_field_##name);   \
> > 	if (IS_ERR(data->reg_##name))						\
> > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > 				     "reg field alloc failed\n");		\
> > })
> 
> I am against unneeded use of GNU extensions.
> 
> > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > enough room to clean this up the formatting of this macro though.
> > > 
> > > do {} while (0) is C standard, ({}) is not.
> > 
> > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > don't see a reason to avoid it, if it helps readability.
> 
> I don't see how it makes things better here, and not everybody is familiar with
> the concept even if it's used in the kernel here and there. Also if a tool is
> being used in one case it doesn't mean it's suitable for another.

Just to throw in my subjective view here: I don't expect anyone with
some base level knowledge of C will have doubts about the semantics of
({ ... }) and compared to that I find do { ... } while (0) less optimal,
because it's more verbose and when spotting the "do {" part, the
semantic only gets clear when you also see the "while (0)". Having said
that I also dislike the "do" starting on column 0, IMHO the RHS of the
#define should be intended.

So if you ask me, this is not an unneeded use of an extension. The
extension is used to improve readabilty and I blame the C standard to
not support this syntax.

While I'm in critics mode: I consider hiding a return in a macro bad
style.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Andy Shevchenko]

On Tue, Nov 12, 2024 at 12:15 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> >
> > You can do it differently
> >
> > #define STK3310_REGFIELD(name)                                                        \
> > do {                                                                          \
> >       data->reg_##name =                                                      \
> >               devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name); \
> >       if (IS_ERR(data->reg_##name))                                           \
> >               return dev_err_probe(dev, PTR_ERR(data->reg_##name),            \
> >                                    "reg field alloc failed.\n");              \
> > } while (0)
> >
> > > #define STK3310_REGFIELD(name) ({                                           \
> > >     data->reg_##name = devm_regmap_field_alloc(dev, regmap,                 \
> > >                                                stk3310_reg_field_##name);   \
> > >     if (IS_ERR(data->reg_##name))                                           \
> > >             return dev_err_probe(dev, PTR_ERR(data->reg_##name),            \
> > >                                  "reg field alloc failed\n");               \
> > > })
> >
> > I am against unneeded use of GNU extensions.
> >
> > > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > > enough room to clean this up the formatting of this macro though.
> > > >
> > > > do {} while (0) is C standard, ({}) is not.
> > >
> > > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > > don't see a reason to avoid it, if it helps readability.
> >
> > I don't see how it makes things better here, and not everybody is familiar with
> > the concept even if it's used in the kernel here and there. Also if a tool is
> > being used in one case it doesn't mean it's suitable for another.
>
> Just to throw in my subjective view here: I don't expect anyone with
> some base level knowledge of C will have doubts about the semantics of
> ({ ... }) and compared to that I find do { ... } while (0) less optimal,
> because it's more verbose and when spotting the "do {" part, the
> semantic only gets clear when you also see the "while (0)".

Seems we have to agree on a disagreement.

> Having said
> that I also dislike the "do" starting on column 0, IMHO the RHS of the
> #define should be intended.

This argument I kinda accept.

> So if you ask me, this is not an unneeded use of an extension. The
> extension is used to improve readabilty and I blame the C standard to
> not support this syntax.

Here I agree with you.

> While I'm in critics mode: I consider hiding a return in a macro bad
> style.

So, summarizing the discussion we have a split, hence Jonathan is our
arbiter here to judge.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Nuno Sá]

On Tue, 2024-11-12 at 11:15 +0100, Uwe Kleine-König wrote:
> Hello Andy, hello Aren,
> 
> On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> > 
> > You can do it differently
> > 
> > #define
> > STK3310_REGFIELD(name)							\
> > do
> > {										\
> > 	data->reg_##name
> > =							\
> > 		devm_regmap_field_alloc(dev, regmap,
> > stk3310_reg_field_##name);	\
> > 	if (IS_ERR(data-
> > >reg_##name))						\
> > 		return dev_err_probe(dev, PTR_ERR(data-
> > >reg_##name),		\
> > 				     "reg field alloc
> > failed.\n");		\
> > } while (0)
> > 
> > > #define STK3310_REGFIELD(name)
> > > ({						\
> > > 	data->reg_##name = devm_regmap_field_alloc(dev,
> > > regmap,			\
> > > 						  
> > > stk3310_reg_field_##name);   \
> > > 	if (IS_ERR(data-
> > > >reg_##name))						\
> > > 		return dev_err_probe(dev, PTR_ERR(data-
> > > >reg_##name),		\
> > > 				     "reg field alloc
> > > failed\n");		\
> > > })
> > 
> > I am against unneeded use of GNU extensions.
> > 
> > > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > > enough room to clean this up the formatting of this macro though.
> > > > 
> > > > do {} while (0) is C standard, ({}) is not.
> > > 
> > > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > > don't see a reason to avoid it, if it helps readability.
> > 
> > I don't see how it makes things better here, and not everybody is familiar with
> > the concept even if it's used in the kernel here and there. Also if a tool is
> > being used in one case it doesn't mean it's suitable for another.
> 
> Just to throw in my subjective view here: I don't expect anyone with
> some base level knowledge of C will have doubts about the semantics of
> ({ ... }) and compared to that I find do { ... } while (0) less optimal,
> because it's more verbose and when spotting the "do {" part, the
> semantic only gets clear when you also see the "while (0)". Having said
> that I also dislike the "do" starting on column 0, IMHO the RHS of the
> #define should be intended.
> 
> So if you ask me, this is not an unneeded use of an extension. The
> extension is used to improve readabilty and I blame the C standard to
> not support this syntax.
> 
> While I'm in critics mode: I consider hiding a return in a macro bad
> style.
> 

Not commenting on the debate between using the extension or not but I totally agree
with Uwe about hiding the return in the macro.

- Nuno Sá
> 

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Aren]

On Tue, Nov 12, 2024 at 11:15:54AM +0100, Uwe Kleine-König wrote:
> Hello Andy, hello Aren,
> 
> On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:
> > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:
> > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:
> > 
> > You can do it differently
> > 
> > #define STK3310_REGFIELD(name)							\
> > do {										\
> > 	data->reg_##name =							\
> > 		devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name);	\
> > 	if (IS_ERR(data->reg_##name))						\
> > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > 				     "reg field alloc failed.\n");		\
> > } while (0)
> > 
> > > #define STK3310_REGFIELD(name) ({						\
> > > 	data->reg_##name = devm_regmap_field_alloc(dev, regmap,			\
> > > 						   stk3310_reg_field_##name);   \
> > > 	if (IS_ERR(data->reg_##name))						\
> > > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > > 				     "reg field alloc failed\n");		\
> > > })
> > 
> > I am against unneeded use of GNU extensions.
> > 
> > > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > > enough room to clean this up the formatting of this macro though.
> > > > 
> > > > do {} while (0) is C standard, ({}) is not.
> > > 
> > > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > > don't see a reason to avoid it, if it helps readability.
> > 
> > I don't see how it makes things better here, and not everybody is familiar with
> > the concept even if it's used in the kernel here and there. Also if a tool is
> > being used in one case it doesn't mean it's suitable for another.
> 
> Just to throw in my subjective view here: I don't expect anyone with
> some base level knowledge of C will have doubts about the semantics of
> ({ ... }) and compared to that I find do { ... } while (0) less optimal,
> because it's more verbose and when spotting the "do {" part, the
> semantic only gets clear when you also see the "while (0)". Having said
> that I also dislike the "do" starting on column 0, IMHO the RHS of the
> #define should be intended.

Thank you, this sums up my opinion on this better than I could have (and
some bits I hadn't considered).

> So if you ask me, this is not an unneeded use of an extension. The
> extension is used to improve readabilty and I blame the C standard to
> not support this syntax.
> 
> While I'm in critics mode: I consider hiding a return in a macro bad
> style.

Yeah... probably worse than any of the formatting options here. I guess
the proper way would be to use devm_regmap_field_bulk_alloc, but that's
well outside the scope of this series. Perhaps it would make sense to
move the macro definition to just before the function it's used in so
it's at least a little easier to spot?

 - Aren

Re: [PATCH v4 4/6] iio: light: stk3310: use dev_err_probe where possible

[Jonathan Cameron]

On Tue, 12 Nov 2024 18:11:37 -0500
Aren <aren@peacevolution.org> wrote:

> On Tue, Nov 12, 2024 at 11:15:54AM +0100, Uwe Kleine-König wrote:
> > Hello Andy, hello Aren,
> > 
> > On Mon, Nov 11, 2024 at 11:44:51AM +0200, Andy Shevchenko wrote:  
> > > On Sun, Nov 10, 2024 at 04:34:30PM -0500, Aren wrote:  
> > > > On Sun, Nov 10, 2024 at 09:52:32PM +0200, Andy Shevchenko wrote:  
> > > > > Sun, Nov 10, 2024 at 02:14:24PM -0500, Aren kirjoitti:  
> > > 
> > > You can do it differently
> > > 
> > > #define STK3310_REGFIELD(name)							\
> > > do {										\
> > > 	data->reg_##name =							\
> > > 		devm_regmap_field_alloc(dev, regmap, stk3310_reg_field_##name);	\
> > > 	if (IS_ERR(data->reg_##name))						\
> > > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > > 				     "reg field alloc failed.\n");		\
> > > } while (0)
> > >   
> > > > #define STK3310_REGFIELD(name) ({						\
> > > > 	data->reg_##name = devm_regmap_field_alloc(dev, regmap,			\
> > > > 						   stk3310_reg_field_##name);   \
> > > > 	if (IS_ERR(data->reg_##name))						\
> > > > 		return dev_err_probe(dev, PTR_ERR(data->reg_##name),		\
> > > > 				     "reg field alloc failed\n");		\
> > > > })  
> > > 
> > > I am against unneeded use of GNU extensions.
> > >   
> > > > > > replacing "do { } while (0)" with "({ })" and deindenting could make
> > > > > > enough room to clean this up the formatting of this macro though.  
> > > > > 
> > > > > do {} while (0) is C standard, ({}) is not.  
> > > > 
> > > > ({ }) is used throughout the kernel, and is documented as such[1]. I
> > > > don't see a reason to avoid it, if it helps readability.  
> > > 
> > > I don't see how it makes things better here, and not everybody is familiar with
> > > the concept even if it's used in the kernel here and there. Also if a tool is
> > > being used in one case it doesn't mean it's suitable for another.  
> > 
> > Just to throw in my subjective view here: I don't expect anyone with
> > some base level knowledge of C will have doubts about the semantics of
> > ({ ... }) and compared to that I find do { ... } while (0) less optimal,
> > because it's more verbose and when spotting the "do {" part, the
> > semantic only gets clear when you also see the "while (0)". Having said
> > that I also dislike the "do" starting on column 0, IMHO the RHS of the
> > #define should be intended.  
> 
> Thank you, this sums up my opinion on this better than I could have (and
> some bits I hadn't considered).
> 
> > So if you ask me, this is not an unneeded use of an extension. The
> > extension is used to improve readabilty and I blame the C standard to
> > not support this syntax.
> > 
> > While I'm in critics mode: I consider hiding a return in a macro bad
> > style.  
> 
> Yeah... probably worse than any of the formatting options here. I guess
> the proper way would be to use devm_regmap_field_bulk_alloc, but that's
> well outside the scope of this series. Perhaps it would make sense to
> move the macro definition to just before the function it's used in so
> it's at least a little easier to spot?

It's only used 8 times.  I'd just get rid of the macro - which now
has even less advantage as the change here reduces the length of the
macro.

Normally I'd argue it should be a precursor patch, but here I think it is
fine to just do it in this patch to avoid a lot of churn.

No macro, no disagreement on formatting ;)

I'm not really sure why I let this macro in to begin with. I normally
push back on this sort of thing. Must have been a low caffeine day :(

Jonathan


> 
>  - Aren

[PATCH v4 5/6] iio: light: stk3310: log error if reading the chip id fails

[Aren Moynihan]

If the chip isn't powered, this call is likely to return an error.
Without a log here the driver will silently fail to probe. Potential
errors include ENXIO (when the chip isn't powered) and ETIMEDOUT (when
the i2c bus isn't powered).

This function is only called from stk3310_probe, and this condition
should return an error, which fits what dev_err_probe is designed for.

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

Notes:
    Changes in v4:
     - get a struct device ahead of time so it can be passed as "dev"
       instead of "&client->dev"
    
    Changes in v2:
     - use dev_err_probe

 drivers/iio/light/stk3310.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index c9a3f02bdd80..becd6901dfef 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -509,7 +509,7 @@ static int stk3310_init(struct iio_dev *indio_dev)
 
 	ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "failed to read chip id\n");
 
 	ret = stk3310_check_chip_id(chipid);
 	if (ret < 0)
-- 
2.47.0

Re: [PATCH v4 5/6] iio: light: stk3310: log error if reading the chip id fails

[Andy Shevchenko]

On Sat, Nov 02, 2024 at 03:50:43PM -0400, Aren Moynihan wrote:
> If the chip isn't powered, this call is likely to return an error.
> Without a log here the driver will silently fail to probe. Potential
> errors include ENXIO (when the chip isn't powered) and ETIMEDOUT (when
> the i2c bus isn't powered).
> 
> This function is only called from stk3310_probe, and this condition
> should return an error, which fits what dev_err_probe is designed for.

...

> +		return dev_err_probe(dev, ret, "failed to read chip id\n");

Please, make sure you have consistent style in the messages. Most of what
I have seen use period at the end. This one doesn't.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/6] iio: light: stk3310: log error if reading the chip id fails

[Aren]

On Mon, Nov 04, 2024 at 10:41:11AM +0200, Andy Shevchenko wrote:
> On Sat, Nov 02, 2024 at 03:50:43PM -0400, Aren Moynihan wrote:
> > If the chip isn't powered, this call is likely to return an error.
> > Without a log here the driver will silently fail to probe. Potential
> > errors include ENXIO (when the chip isn't powered) and ETIMEDOUT (when
> > the i2c bus isn't powered).
> > 
> > This function is only called from stk3310_probe, and this condition
> > should return an error, which fits what dev_err_probe is designed for.
> 
> ...
> 
> > +		return dev_err_probe(dev, ret, "failed to read chip id\n");
> 
> Please, make sure you have consistent style in the messages. Most of what
> I have seen use period at the end. This one doesn't.

All but two log messages in this driver don't have a period at the end.
I'll correct those two in the next revision.

 - Aren

[PATCH v4 6/6] arm64: dts: allwinner: pinephone: Add power supplies to stk3311

[Aren Moynihan]

From: Ondrej Jirman <megi@xff.cz>

This allows the driver to properly handle powering this device, and
disable power during suspend.

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

Notes:
    Changes in v2:
     - add leda-supply

 arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index 6d152066a59a..618341b63db2 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -260,6 +260,8 @@ light-sensor@48 {
 		reg = <0x48>;
 		interrupt-parent = <&pio>;
 		interrupts = <1 0 IRQ_TYPE_EDGE_FALLING>; /* PB0 */
+		vdd-supply = <&reg_ldo_io0>;
+		leda-supply = <&reg_dldo1>;
 	};
 
 	/* Accelerometer/gyroscope */
-- 
2.47.0