* [PATCH 0/4] iio: light: stk3310: support powering off during suspend
@ 2024-04-14 17:53 ` Aren Moynihan
0 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:53 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
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.
Aren Moynihan (2):
dt-bindings: iio: light: stk33xx: add regulator for vdd supply
iio: light: stk3310: log error if reading the chip id fails
Ondrej Jirman (2):
iio: light: stk3310: Implement vdd supply and power it off during
suspend
arm64: dts: allwinner: pinephone: Add power supply to stk3311
.../bindings/iio/light/stk33xx.yaml | 1 +
.../dts/allwinner/sun50i-a64-pinephone.dtsi | 1 +
drivers/iio/light/stk3310.c | 60 +++++++++++++++++--
3 files changed, 58 insertions(+), 4 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 0/4] iio: light: stk3310: support powering off during suspend
@ 2024-04-14 17:53 ` Aren Moynihan
0 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:53 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
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.
Aren Moynihan (2):
dt-bindings: iio: light: stk33xx: add regulator for vdd supply
iio: light: stk3310: log error if reading the chip id fails
Ondrej Jirman (2):
iio: light: stk3310: Implement vdd supply and power it off during
suspend
arm64: dts: allwinner: pinephone: Add power supply to stk3311
.../bindings/iio/light/stk33xx.yaml | 1 +
.../dts/allwinner/sun50i-a64-pinephone.dtsi | 1 +
drivers/iio/light/stk3310.c | 60 +++++++++++++++++--
3 files changed, 58 insertions(+), 4 deletions(-)
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
2024-04-14 17:53 ` Aren Moynihan
@ 2024-04-14 17:57 ` Aren Moynihan
-1 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
index f6e22dc9814a..db35e239d4a8 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -29,6 +29,7 @@ properties:
interrupts:
maxItems: 1
+ vdd-supply: true
proximity-near-level: true
required:
--
2.44.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
@ 2024-04-14 17:57 ` Aren Moynihan
0 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
index f6e22dc9814a..db35e239d4a8 100644
--- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
+++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
@@ -29,6 +29,7 @@ properties:
interrupts:
maxItems: 1
+ vdd-supply: true
proximity-near-level: true
required:
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-14 17:57 ` Aren Moynihan
-1 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
From: Ondrej Jirman <megi@xff.cz>
VDD power input can be used to completely power off the chip during
system suspend. Do so if available.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
drivers/iio/light/stk3310.c | 56 +++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 7b71ad71d78d..bfa090538df7 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -16,6 +16,7 @@
#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
#define STK3310_REG_STATE 0x00
#define STK3310_REG_PSCTRL 0x01
@@ -117,6 +118,7 @@ struct stk3310_data {
struct regmap_field *reg_int_ps;
struct regmap_field *reg_flag_psint;
struct regmap_field *reg_flag_nf;
+ struct regulator *vdd_reg;
};
static const struct iio_event_spec stk3310_events[] = {
@@ -607,6 +609,16 @@ static int stk3310_probe(struct i2c_client *client)
mutex_init(&data->lock);
+ data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
+ if (IS_ERR(data->vdd_reg)) {
+ ret = PTR_ERR(data->vdd_reg);
+ if (ret == -ENODEV)
+ data->vdd_reg = NULL;
+ else
+ return dev_err_probe(&client->dev, ret,
+ "get regulator vdd failed\n");
+ }
+
ret = stk3310_regmap_init(data);
if (ret < 0)
return ret;
@@ -617,9 +629,18 @@ static int stk3310_probe(struct i2c_client *client)
indio_dev->channels = stk3310_channels;
indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
+ if (data->vdd_reg) {
+ ret = regulator_enable(data->vdd_reg);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "regulator vdd enable failed\n");
+
+ usleep_range(1000, 2000);
+ }
+
ret = stk3310_init(indio_dev);
if (ret < 0)
- return ret;
+ goto err_vdd_disable;
if (client->irq > 0) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -645,32 +666,61 @@ static int stk3310_probe(struct i2c_client *client)
err_standby:
stk3310_set_state(data, STK3310_STATE_STANDBY);
+err_vdd_disable:
+ if (data->vdd_reg)
+ regulator_disable(data->vdd_reg);
return ret;
}
static void stk3310_remove(struct i2c_client *client)
{
struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct stk3310_data *data = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
+ if (data->vdd_reg)
+ regulator_disable(data->vdd_reg);
}
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;
+
+ if (data->vdd_reg) {
+ regcache_mark_dirty(data->regmap);
+ regulator_disable(data->vdd_reg);
+ }
+
+ return 0;
}
static int stk3310_resume(struct device *dev)
{
- u8 state = 0;
struct stk3310_data *data;
+ u8 state = 0;
+ int ret;
data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
+
+ if (data->vdd_reg) {
+ ret = regulator_enable(data->vdd_reg);
+ if (ret) {
+ dev_err(dev, "Failed to re-enable regulator vdd\n");
+ return ret;
+ }
+
+ usleep_range(1000, 2000);
+ regcache_sync(data->regmap);
+ }
+
if (data->ps_enabled)
state |= STK3310_STATE_EN_PS;
if (data->als_enabled)
--
2.44.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-14 17:57 ` Aren Moynihan
0 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
From: Ondrej Jirman <megi@xff.cz>
VDD power input can be used to completely power off the chip during
system suspend. Do so if available.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
drivers/iio/light/stk3310.c | 56 +++++++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 7b71ad71d78d..bfa090538df7 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -16,6 +16,7 @@
#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
#define STK3310_REG_STATE 0x00
#define STK3310_REG_PSCTRL 0x01
@@ -117,6 +118,7 @@ struct stk3310_data {
struct regmap_field *reg_int_ps;
struct regmap_field *reg_flag_psint;
struct regmap_field *reg_flag_nf;
+ struct regulator *vdd_reg;
};
static const struct iio_event_spec stk3310_events[] = {
@@ -607,6 +609,16 @@ static int stk3310_probe(struct i2c_client *client)
mutex_init(&data->lock);
+ data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
+ if (IS_ERR(data->vdd_reg)) {
+ ret = PTR_ERR(data->vdd_reg);
+ if (ret == -ENODEV)
+ data->vdd_reg = NULL;
+ else
+ return dev_err_probe(&client->dev, ret,
+ "get regulator vdd failed\n");
+ }
+
ret = stk3310_regmap_init(data);
if (ret < 0)
return ret;
@@ -617,9 +629,18 @@ static int stk3310_probe(struct i2c_client *client)
indio_dev->channels = stk3310_channels;
indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
+ if (data->vdd_reg) {
+ ret = regulator_enable(data->vdd_reg);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "regulator vdd enable failed\n");
+
+ usleep_range(1000, 2000);
+ }
+
ret = stk3310_init(indio_dev);
if (ret < 0)
- return ret;
+ goto err_vdd_disable;
if (client->irq > 0) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -645,32 +666,61 @@ static int stk3310_probe(struct i2c_client *client)
err_standby:
stk3310_set_state(data, STK3310_STATE_STANDBY);
+err_vdd_disable:
+ if (data->vdd_reg)
+ regulator_disable(data->vdd_reg);
return ret;
}
static void stk3310_remove(struct i2c_client *client)
{
struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct stk3310_data *data = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
+ if (data->vdd_reg)
+ regulator_disable(data->vdd_reg);
}
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;
+
+ if (data->vdd_reg) {
+ regcache_mark_dirty(data->regmap);
+ regulator_disable(data->vdd_reg);
+ }
+
+ return 0;
}
static int stk3310_resume(struct device *dev)
{
- u8 state = 0;
struct stk3310_data *data;
+ u8 state = 0;
+ int ret;
data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
+
+ if (data->vdd_reg) {
+ ret = regulator_enable(data->vdd_reg);
+ if (ret) {
+ dev_err(dev, "Failed to re-enable regulator vdd\n");
+ return ret;
+ }
+
+ usleep_range(1000, 2000);
+ regcache_sync(data->regmap);
+ }
+
if (data->ps_enabled)
state |= STK3310_STATE_EN_PS;
if (data->als_enabled)
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-14 17:57 ` Aren Moynihan
-1 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
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. Common errors
are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
isn't powered).
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
drivers/iio/light/stk3310.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index bfa090538df7..c0954a63a143 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -472,8 +472,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
struct i2c_client *client = data->client;
ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(&client->dev, "failed to read chip id: %d", ret);
return ret;
+ }
if (chipid != STK3310_CHIP_ID_VAL &&
chipid != STK3311_CHIP_ID_VAL &&
--
2.44.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
@ 2024-04-14 17:57 ` Aren Moynihan
0 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
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. Common errors
are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
isn't powered).
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
drivers/iio/light/stk3310.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index bfa090538df7..c0954a63a143 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -472,8 +472,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
struct i2c_client *client = data->client;
ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(&client->dev, "failed to read chip id: %d", ret);
return ret;
+ }
if (chipid != STK3310_CHIP_ID_VAL &&
chipid != STK3311_CHIP_ID_VAL &&
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/4] arm64: dts: allwinner: pinephone: Add power supply to stk3311
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-14 17:57 ` Aren Moynihan
-1 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
From: Ondrej Jirman <megi@xff.cz>
This makes the driver disable the supply during sleep.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index b5a232209f2b..e87bc21db316 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -250,6 +250,7 @@ light-sensor@48 {
reg = <0x48>;
interrupt-parent = <&pio>;
interrupts = <1 0 IRQ_TYPE_EDGE_FALLING>; /* PB0 */
+ vdd-supply = <®_ldo_io0>;
};
/* Accelerometer/gyroscope */
--
2.44.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/4] arm64: dts: allwinner: pinephone: Add power supply to stk3311
@ 2024-04-14 17:57 ` Aren Moynihan
0 siblings, 0 replies; 32+ messages in thread
From: Aren Moynihan @ 2024-04-14 17:57 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
From: Ondrej Jirman <megi@xff.cz>
This makes the driver disable the supply during sleep.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index b5a232209f2b..e87bc21db316 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -250,6 +250,7 @@ light-sensor@48 {
reg = <0x48>;
interrupt-parent = <&pio>;
interrupts = <1 0 IRQ_TYPE_EDGE_FALLING>; /* PB0 */
+ vdd-supply = <®_ldo_io0>;
};
/* Accelerometer/gyroscope */
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-14 18:39 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2024-04-14 18:39 UTC (permalink / raw)
To: Aren Moynihan, Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Andy Shevchenko, Ondrej Jirman, Uwe Kleine-König, linux-iio,
phone-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-sunxi, Willow Barraco
On 14/04/2024 19:57, Aren Moynihan wrote:
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
Why? Please provide proper commit msg which will explain why you are
doing it (e.g. say about hardware).
Anyway empty commit msgs cannot be accepted.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
@ 2024-04-14 18:39 ` Krzysztof Kozlowski
0 siblings, 0 replies; 32+ messages in thread
From: Krzysztof Kozlowski @ 2024-04-14 18:39 UTC (permalink / raw)
To: Aren Moynihan, Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown
Cc: Andy Shevchenko, Ondrej Jirman, Uwe Kleine-König, linux-iio,
phone-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-sunxi, Willow Barraco
On 14/04/2024 19:57, Aren Moynihan wrote:
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
Why? Please provide proper commit msg which will explain why you are
doing it (e.g. say about hardware).
Anyway empty commit msgs cannot be accepted.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-15 14:04 ` Andy Shevchenko
-1 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-15 14:04 UTC (permalink / raw)
To: Aren Moynihan
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
>
> From: Ondrej Jirman <megi@xff.cz>
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> #include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
Move it to be ordered and add a blank line to separate iio/*.h group.
...
> + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret == -ENODEV)
> + data->vdd_reg = NULL;
> + else
Redundant 'else' when you follow the pattern "check for error condition first".
> + return dev_err_probe(&client->dev, ret,
> + "get regulator vdd failed\n");
> + }
...
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "regulator vdd enable failed\n");
> +
> + usleep_range(1000, 2000);
fsleep()
> + }
...
> stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
I forgot to check the order of freeing resources, be sure you have no
devm_*() releases happening before this call.
...
> + usleep_range(1000, 2000);
fsleep()
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-15 14:04 ` Andy Shevchenko
0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-15 14:04 UTC (permalink / raw)
To: Aren Moynihan
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
>
> From: Ondrej Jirman <megi@xff.cz>
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
...
> #include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
Move it to be ordered and add a blank line to separate iio/*.h group.
...
> + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret == -ENODEV)
> + data->vdd_reg = NULL;
> + else
Redundant 'else' when you follow the pattern "check for error condition first".
> + return dev_err_probe(&client->dev, ret,
> + "get regulator vdd failed\n");
> + }
...
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "regulator vdd enable failed\n");
> +
> + usleep_range(1000, 2000);
fsleep()
> + }
...
> stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
I forgot to check the order of freeing resources, be sure you have no
devm_*() releases happening before this call.
...
> + usleep_range(1000, 2000);
fsleep()
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-15 15:05 ` Andy Shevchenko
-1 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-15 15:05 UTC (permalink / raw)
To: Aren Moynihan
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> 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. Common errors
> are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
> isn't powered).
> ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to read chip id: %d", ret);
> return ret;
> + }
Briefly looking at the code it seems that this one is strictly part of
the probe phase, which means we may use
return dev_err_probe(...);
pattern. Yet, you may add another patch to clean up all of them:
_probe(), _init(), _regmap_init() to use the same pattern everywhere.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
@ 2024-04-15 15:05 ` Andy Shevchenko
0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-15 15:05 UTC (permalink / raw)
To: Aren Moynihan
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> 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. Common errors
> are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
> isn't powered).
> ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to read chip id: %d", ret);
> return ret;
> + }
Briefly looking at the code it seems that this one is strictly part of
the probe phase, which means we may use
return dev_err_probe(...);
pattern. Yet, you may add another patch to clean up all of them:
_probe(), _init(), _regmap_init() to use the same pattern everywhere.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-15 20:46 ` Jernej Škrabec
-1 siblings, 0 replies; 32+ messages in thread
From: Jernej Škrabec @ 2024-04-15 20:46 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Samuel Holland,
Liam Girdwood, Mark Brown, Aren Moynihan
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
Dne nedelja, 14. april 2024 ob 19:57:13 GMT +2 je Aren Moynihan napisal(a):
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
Commit message cannot be empty.
Best regards,
Jernej
> ---
> Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> index f6e22dc9814a..db35e239d4a8 100644
> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> @@ -29,6 +29,7 @@ properties:
> interrupts:
> maxItems: 1
>
> + vdd-supply: true
> proximity-near-level: true
>
> required:
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
@ 2024-04-15 20:46 ` Jernej Škrabec
0 siblings, 0 replies; 32+ messages in thread
From: Jernej Škrabec @ 2024-04-15 20:46 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Samuel Holland,
Liam Girdwood, Mark Brown, Aren Moynihan
Cc: Aren Moynihan, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
Dne nedelja, 14. april 2024 ob 19:57:13 GMT +2 je Aren Moynihan napisal(a):
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
Commit message cannot be empty.
Best regards,
Jernej
> ---
> Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> index f6e22dc9814a..db35e239d4a8 100644
> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> @@ -29,6 +29,7 @@ properties:
> interrupts:
> maxItems: 1
>
> + vdd-supply: true
> proximity-near-level: true
>
> required:
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-15 14:04 ` Andy Shevchenko
@ 2024-04-18 15:06 ` Aren
-1 siblings, 0 replies; 32+ messages in thread
From: Aren @ 2024-04-18 15:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
> >
> > From: Ondrej Jirman <megi@xff.cz>
> >
> > VDD power input can be used to completely power off the chip during
> > system suspend. Do so if available.
>
> ...
>
> > #include <linux/iio/events.h>
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
>
> > +#include <linux/regulator/consumer.h>
>
> Move it to be ordered and add a blank line to separate iio/*.h group.
>
> ...
>
> > + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + ret = PTR_ERR(data->vdd_reg);
> > + if (ret == -ENODEV)
> > + data->vdd_reg = NULL;
>
> > + else
>
> Redundant 'else' when you follow the pattern "check for error condition first".
>
> > + return dev_err_probe(&client->dev, ret,
> > + "get regulator vdd failed\n");
> > + }
>
> ...
>
> > + if (data->vdd_reg) {
> > + ret = regulator_enable(data->vdd_reg);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "regulator vdd enable failed\n");
> > +
> > + usleep_range(1000, 2000);
>
> fsleep()
>
> > + }
>
> ...
>
> > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> > + if (data->vdd_reg)
> > + regulator_disable(data->vdd_reg);
>
> I forgot to check the order of freeing resources, be sure you have no
> devm_*() releases happening before this call.
If I understand what you're saying, this should be fine. The driver just
uses devm to clean up acquired resources after remove is called. Or am I
missing something and resources could be freed before calling
stk3310_remove?
> ...
>
> > + usleep_range(1000, 2000);
>
> fsleep()
Everything else makes sense, I'll include those in v2 along with a patch
to switch stk3310_init to dev_err_probe.
Thanks for taking the time to review
- Aren
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-18 15:06 ` Aren
0 siblings, 0 replies; 32+ messages in thread
From: Aren @ 2024-04-18 15:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
> >
> > From: Ondrej Jirman <megi@xff.cz>
> >
> > VDD power input can be used to completely power off the chip during
> > system suspend. Do so if available.
>
> ...
>
> > #include <linux/iio/events.h>
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
>
> > +#include <linux/regulator/consumer.h>
>
> Move it to be ordered and add a blank line to separate iio/*.h group.
>
> ...
>
> > + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
> > + if (IS_ERR(data->vdd_reg)) {
> > + ret = PTR_ERR(data->vdd_reg);
> > + if (ret == -ENODEV)
> > + data->vdd_reg = NULL;
>
> > + else
>
> Redundant 'else' when you follow the pattern "check for error condition first".
>
> > + return dev_err_probe(&client->dev, ret,
> > + "get regulator vdd failed\n");
> > + }
>
> ...
>
> > + if (data->vdd_reg) {
> > + ret = regulator_enable(data->vdd_reg);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "regulator vdd enable failed\n");
> > +
> > + usleep_range(1000, 2000);
>
> fsleep()
>
> > + }
>
> ...
>
> > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> > + if (data->vdd_reg)
> > + regulator_disable(data->vdd_reg);
>
> I forgot to check the order of freeing resources, be sure you have no
> devm_*() releases happening before this call.
If I understand what you're saying, this should be fine. The driver just
uses devm to clean up acquired resources after remove is called. Or am I
missing something and resources could be freed before calling
stk3310_remove?
> ...
>
> > + usleep_range(1000, 2000);
>
> fsleep()
Everything else makes sense, I'll include those in v2 along with a patch
to switch stk3310_init to dev_err_probe.
Thanks for taking the time to review
- Aren
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-18 15:06 ` Aren
@ 2024-04-18 15:56 ` Andy Shevchenko
-1 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-18 15:56 UTC (permalink / raw)
To: Aren
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote:
> On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
...
> > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> > > + if (data->vdd_reg)
> > > + regulator_disable(data->vdd_reg);
> >
> > I forgot to check the order of freeing resources, be sure you have no
> > devm_*() releases happening before this call.
>
> If I understand what you're saying, this should be fine. The driver just
> uses devm to clean up acquired resources after remove is called. Or am I
> missing something and resources could be freed before calling
> stk3310_remove?
I'm not objecting to that. The point here is that the resources should
be freed in the reversed order. devm-allocated resources are deferred
to be freed after the explicit driver ->remove() callback. At the end
it should not interleave with each other, i.o.w. it should be
probe: devm followed by non-devm
remove: non-devm only.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-18 15:56 ` Andy Shevchenko
0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-18 15:56 UTC (permalink / raw)
To: Aren
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote:
> On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
...
> > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> > > + if (data->vdd_reg)
> > > + regulator_disable(data->vdd_reg);
> >
> > I forgot to check the order of freeing resources, be sure you have no
> > devm_*() releases happening before this call.
>
> If I understand what you're saying, this should be fine. The driver just
> uses devm to clean up acquired resources after remove is called. Or am I
> missing something and resources could be freed before calling
> stk3310_remove?
I'm not objecting to that. The point here is that the resources should
be freed in the reversed order. devm-allocated resources are deferred
to be freed after the explicit driver ->remove() callback. At the end
it should not interleave with each other, i.o.w. it should be
probe: devm followed by non-devm
remove: non-devm only.
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-18 15:56 ` Andy Shevchenko
@ 2024-04-18 17:50 ` Aren
-1 siblings, 0 replies; 32+ messages in thread
From: Aren @ 2024-04-18 17:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote:
> > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
>
> ...
>
> > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> > > > + if (data->vdd_reg)
> > > > + regulator_disable(data->vdd_reg);
> > >
> > > I forgot to check the order of freeing resources, be sure you have no
> > > devm_*() releases happening before this call.
> >
> > If I understand what you're saying, this should be fine. The driver just
> > uses devm to clean up acquired resources after remove is called. Or am I
> > missing something and resources could be freed before calling
> > stk3310_remove?
>
> I'm not objecting to that. The point here is that the resources should
> be freed in the reversed order. devm-allocated resources are deferred
> to be freed after the explicit driver ->remove() callback. At the end
> it should not interleave with each other, i.o.w. it should be
> probe: devm followed by non-devm
> remove: non-devm only.
I think what you're describing is already the case, with the exception
of parts of the probe function not changed in this patch mixing
acquiring resources through devm with configuring the device.
I hope I'm not being dense, thanks for the clarification
- Aren
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-18 17:50 ` Aren
0 siblings, 0 replies; 32+ messages in thread
From: Aren @ 2024-04-18 17:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote:
> > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
>
> ...
>
> > > > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> > > > + if (data->vdd_reg)
> > > > + regulator_disable(data->vdd_reg);
> > >
> > > I forgot to check the order of freeing resources, be sure you have no
> > > devm_*() releases happening before this call.
> >
> > If I understand what you're saying, this should be fine. The driver just
> > uses devm to clean up acquired resources after remove is called. Or am I
> > missing something and resources could be freed before calling
> > stk3310_remove?
>
> I'm not objecting to that. The point here is that the resources should
> be freed in the reversed order. devm-allocated resources are deferred
> to be freed after the explicit driver ->remove() callback. At the end
> it should not interleave with each other, i.o.w. it should be
> probe: devm followed by non-devm
> remove: non-devm only.
I think what you're describing is already the case, with the exception
of parts of the probe function not changed in this patch mixing
acquiring resources through devm with configuring the device.
I hope I'm not being dense, thanks for the clarification
- Aren
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-18 17:50 ` Aren
@ 2024-04-18 18:19 ` Andy Shevchenko
-1 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-18 18:19 UTC (permalink / raw)
To: Aren
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Thu, Apr 18, 2024 at 8:50 PM Aren <aren@peacevolution.org> wrote:
> On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote:
> > > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
...
> > > > I forgot to check the order of freeing resources, be sure you have no
> > > > devm_*() releases happening before this call.
> > >
> > > If I understand what you're saying, this should be fine. The driver just
> > > uses devm to clean up acquired resources after remove is called. Or am I
> > > missing something and resources could be freed before calling
> > > stk3310_remove?
> >
> > I'm not objecting to that. The point here is that the resources should
> > be freed in the reversed order. devm-allocated resources are deferred
> > to be freed after the explicit driver ->remove() callback. At the end
> > it should not interleave with each other, i.o.w. it should be
> > probe: devm followed by non-devm
> > remove: non-devm only.
>
> I think what you're describing is already the case, with the exception
> of parts of the probe function not changed in this patch mixing
> acquiring resources through devm with configuring the device.
Okay, then we are fine!
> I hope I'm not being dense, thanks for the clarification
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-18 18:19 ` Andy Shevchenko
0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-04-18 18:19 UTC (permalink / raw)
To: Aren
Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Thu, Apr 18, 2024 at 8:50 PM Aren <aren@peacevolution.org> wrote:
> On Thu, Apr 18, 2024 at 06:56:09PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 18, 2024 at 6:06 PM Aren <aren@peacevolution.org> wrote:
> > > On Mon, Apr 15, 2024 at 05:04:53PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> wrote:
...
> > > > I forgot to check the order of freeing resources, be sure you have no
> > > > devm_*() releases happening before this call.
> > >
> > > If I understand what you're saying, this should be fine. The driver just
> > > uses devm to clean up acquired resources after remove is called. Or am I
> > > missing something and resources could be freed before calling
> > > stk3310_remove?
> >
> > I'm not objecting to that. The point here is that the resources should
> > be freed in the reversed order. devm-allocated resources are deferred
> > to be freed after the explicit driver ->remove() callback. At the end
> > it should not interleave with each other, i.o.w. it should be
> > probe: devm followed by non-devm
> > remove: non-devm only.
>
> I think what you're describing is already the case, with the exception
> of parts of the probe function not changed in this patch mixing
> acquiring resources through devm with configuring the device.
Okay, then we are fine!
> I hope I'm not being dense, thanks for the clarification
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-20 13:04 ` Jonathan Cameron
-1 siblings, 0 replies; 32+ messages in thread
From: Jonathan Cameron @ 2024-04-20 13:04 UTC (permalink / raw)
To: Aren Moynihan
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Liam Girdwood, Mark Brown, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, 14 Apr 2024 13:57:14 -0400
Aren Moynihan <aren@peacevolution.org> wrote:
> From: Ondrej Jirman <megi@xff.cz>
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
I'd make this non optional (relying on regulator framework providing
us a stub for old DT etc) and pay the minor cost of potentially restoring
registers when it was never powered down.
Simpler code and likely anyone who is doing suspend / resume will have
power control anyway.
Jonathan
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> drivers/iio/light/stk3310.c | 56 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 7b71ad71d78d..bfa090538df7 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -16,6 +16,7 @@
> #include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
>
> #define STK3310_REG_STATE 0x00
> #define STK3310_REG_PSCTRL 0x01
> @@ -117,6 +118,7 @@ struct stk3310_data {
> struct regmap_field *reg_int_ps;
> struct regmap_field *reg_flag_psint;
> struct regmap_field *reg_flag_nf;
> + struct regulator *vdd_reg;
> };
>
> static const struct iio_event_spec stk3310_events[] = {
> @@ -607,6 +609,16 @@ static int stk3310_probe(struct i2c_client *client)
>
> mutex_init(&data->lock);
>
> + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
This needs a comment on why it is optional.
Generally power supply regulators are not, but I think the point here
is to avoid restoring the registers if the chip wasn't powered down?
This feels like an interesting gap in the regulator framework.
For most cases we can rely on stub / fake regulator being created
for always on supplies, but that doesn't let us elide the register writes.
My gut feeling is do them unconditionally. Suspend / resume isn't
that common that it will matter much.
That would allow you to have this as devm_regulator_get() and
drop handling of it not being provided.
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret == -ENODEV)
> + data->vdd_reg = NULL;
> + else
> + return dev_err_probe(&client->dev, ret,
> + "get regulator vdd failed\n");
> + }
> +
> ret = stk3310_regmap_init(data);
> if (ret < 0)
> return ret;
> @@ -617,9 +629,18 @@ static int stk3310_probe(struct i2c_client *client)
> indio_dev->channels = stk3310_channels;
> indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
>
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "regulator vdd enable failed\n");
> +
> + usleep_range(1000, 2000);
> + }
> +
> ret = stk3310_init(indio_dev);
> if (ret < 0)
> - return ret;
> + goto err_vdd_disable;
>
> if (client->irq > 0) {
> ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -645,32 +666,61 @@ static int stk3310_probe(struct i2c_client *client)
>
> err_standby:
> stk3310_set_state(data, STK3310_STATE_STANDBY);
> +err_vdd_disable:
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
> return ret;
> }
>
> static void stk3310_remove(struct i2c_client *client)
> {
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct stk3310_data *data = iio_priv(indio_dev);
>
> iio_device_unregister(indio_dev);
> stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
> }
>
> 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;
> +
> + if (data->vdd_reg) {
As above, I don't think we care enough about overhead on
boards where there isn't a vdd regulator. Just do this
unconditionally.
> + regcache_mark_dirty(data->regmap);
> + regulator_disable(data->vdd_reg);
> + }
> +
> + return 0;
> }
>
> static int stk3310_resume(struct device *dev)
> {
> - u8 state = 0;
> struct stk3310_data *data;
> + u8 state = 0;
> + int ret;
>
> data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> +
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret) {
> + dev_err(dev, "Failed to re-enable regulator vdd\n");
> + return ret;
> + }
> +
> + usleep_range(1000, 2000);
> + regcache_sync(data->regmap);
> + }
> +
> if (data->ps_enabled)
> state |= STK3310_STATE_EN_PS;
> if (data->als_enabled)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend
@ 2024-04-20 13:04 ` Jonathan Cameron
0 siblings, 0 replies; 32+ messages in thread
From: Jonathan Cameron @ 2024-04-20 13:04 UTC (permalink / raw)
To: Aren Moynihan
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Liam Girdwood, Mark Brown, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, 14 Apr 2024 13:57:14 -0400
Aren Moynihan <aren@peacevolution.org> wrote:
> From: Ondrej Jirman <megi@xff.cz>
>
> VDD power input can be used to completely power off the chip during
> system suspend. Do so if available.
I'd make this non optional (relying on regulator framework providing
us a stub for old DT etc) and pay the minor cost of potentially restoring
registers when it was never powered down.
Simpler code and likely anyone who is doing suspend / resume will have
power control anyway.
Jonathan
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> drivers/iio/light/stk3310.c | 56 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 7b71ad71d78d..bfa090538df7 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -16,6 +16,7 @@
> #include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
>
> #define STK3310_REG_STATE 0x00
> #define STK3310_REG_PSCTRL 0x01
> @@ -117,6 +118,7 @@ struct stk3310_data {
> struct regmap_field *reg_int_ps;
> struct regmap_field *reg_flag_psint;
> struct regmap_field *reg_flag_nf;
> + struct regulator *vdd_reg;
> };
>
> static const struct iio_event_spec stk3310_events[] = {
> @@ -607,6 +609,16 @@ static int stk3310_probe(struct i2c_client *client)
>
> mutex_init(&data->lock);
>
> + data->vdd_reg = devm_regulator_get_optional(&client->dev, "vdd");
This needs a comment on why it is optional.
Generally power supply regulators are not, but I think the point here
is to avoid restoring the registers if the chip wasn't powered down?
This feels like an interesting gap in the regulator framework.
For most cases we can rely on stub / fake regulator being created
for always on supplies, but that doesn't let us elide the register writes.
My gut feeling is do them unconditionally. Suspend / resume isn't
that common that it will matter much.
That would allow you to have this as devm_regulator_get() and
drop handling of it not being provided.
> + if (IS_ERR(data->vdd_reg)) {
> + ret = PTR_ERR(data->vdd_reg);
> + if (ret == -ENODEV)
> + data->vdd_reg = NULL;
> + else
> + return dev_err_probe(&client->dev, ret,
> + "get regulator vdd failed\n");
> + }
> +
> ret = stk3310_regmap_init(data);
> if (ret < 0)
> return ret;
> @@ -617,9 +629,18 @@ static int stk3310_probe(struct i2c_client *client)
> indio_dev->channels = stk3310_channels;
> indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
>
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "regulator vdd enable failed\n");
> +
> + usleep_range(1000, 2000);
> + }
> +
> ret = stk3310_init(indio_dev);
> if (ret < 0)
> - return ret;
> + goto err_vdd_disable;
>
> if (client->irq > 0) {
> ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -645,32 +666,61 @@ static int stk3310_probe(struct i2c_client *client)
>
> err_standby:
> stk3310_set_state(data, STK3310_STATE_STANDBY);
> +err_vdd_disable:
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
> return ret;
> }
>
> static void stk3310_remove(struct i2c_client *client)
> {
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct stk3310_data *data = iio_priv(indio_dev);
>
> iio_device_unregister(indio_dev);
> stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY);
> + if (data->vdd_reg)
> + regulator_disable(data->vdd_reg);
> }
>
> 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;
> +
> + if (data->vdd_reg) {
As above, I don't think we care enough about overhead on
boards where there isn't a vdd regulator. Just do this
unconditionally.
> + regcache_mark_dirty(data->regmap);
> + regulator_disable(data->vdd_reg);
> + }
> +
> + return 0;
> }
>
> static int stk3310_resume(struct device *dev)
> {
> - u8 state = 0;
> struct stk3310_data *data;
> + u8 state = 0;
> + int ret;
>
> data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> +
> + if (data->vdd_reg) {
> + ret = regulator_enable(data->vdd_reg);
> + if (ret) {
> + dev_err(dev, "Failed to re-enable regulator vdd\n");
> + return ret;
> + }
> +
> + usleep_range(1000, 2000);
> + regcache_sync(data->regmap);
> + }
> +
> if (data->ps_enabled)
> state |= STK3310_STATE_EN_PS;
> if (data->als_enabled)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
2024-04-14 17:57 ` Aren Moynihan
@ 2024-04-20 13:05 ` Jonathan Cameron
-1 siblings, 0 replies; 32+ messages in thread
From: Jonathan Cameron @ 2024-04-20 13:05 UTC (permalink / raw)
To: Aren Moynihan
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Liam Girdwood, Mark Brown, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, 14 Apr 2024 13:57:13 -0400
Aren Moynihan <aren@peacevolution.org> wrote:
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> index f6e22dc9814a..db35e239d4a8 100644
> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> @@ -29,6 +29,7 @@ properties:
> interrupts:
> maxItems: 1
>
> + vdd-supply: true
As per review of patch 2, make it required and add one to the example.
That doesn't mean it will be in every dts file, just that we expect
it to exist given chips tend to work poorly without power.
> proximity-near-level: true
>
> required:
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply
@ 2024-04-20 13:05 ` Jonathan Cameron
0 siblings, 0 replies; 32+ messages in thread
From: Jonathan Cameron @ 2024-04-20 13:05 UTC (permalink / raw)
To: Aren Moynihan
Cc: Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Liam Girdwood, Mark Brown, Andy Shevchenko, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Sun, 14 Apr 2024 13:57:13 -0400
Aren Moynihan <aren@peacevolution.org> wrote:
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> Documentation/devicetree/bindings/iio/light/stk33xx.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> index f6e22dc9814a..db35e239d4a8 100644
> --- a/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/stk33xx.yaml
> @@ -29,6 +29,7 @@ properties:
> interrupts:
> maxItems: 1
>
> + vdd-supply: true
As per review of patch 2, make it required and add one to the example.
That doesn't mean it will be in every dts file, just that we expect
it to exist given chips tend to work poorly without power.
> proximity-near-level: true
>
> required:
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
2024-04-15 15:05 ` Andy Shevchenko
@ 2024-04-20 13:06 ` Jonathan Cameron
-1 siblings, 0 replies; 32+ messages in thread
From: Jonathan Cameron @ 2024-04-20 13:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Aren Moynihan, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Mon, 15 Apr 2024 18:05:54 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> 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. Common errors
> > are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
> > isn't powered).
>
> > ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to read chip id: %d", ret);
> > return ret;
> > + }
>
> Briefly looking at the code it seems that this one is strictly part of
> the probe phase, which means we may use
>
> return dev_err_probe(...);
>
> pattern. Yet, you may add another patch to clean up all of them:
> _probe(), _init(), _regmap_init() to use the same pattern everywhere.
>
Yes, a precursor patch to use dev_err_probe() throughout the probe only
functions in this driver would be excellent.
Jonathan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails
@ 2024-04-20 13:06 ` Jonathan Cameron
0 siblings, 0 replies; 32+ messages in thread
From: Jonathan Cameron @ 2024-04-20 13:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Aren Moynihan, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chen-Yu Tsai, Jernej Skrabec,
Samuel Holland, Liam Girdwood, Mark Brown, Ondrej Jirman,
Uwe Kleine-König, linux-iio, phone-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-sunxi, Willow Barraco
On Mon, 15 Apr 2024 18:05:54 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sun, Apr 14, 2024 at 8:57 PM Aren Moynihan <aren@peacevolution.org> 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. Common errors
> > are ENXIO (when the chip isn't powered) and ETIMEDOUT (when the i2c bus
> > isn't powered).
>
> > ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to read chip id: %d", ret);
> > return ret;
> > + }
>
> Briefly looking at the code it seems that this one is strictly part of
> the probe phase, which means we may use
>
> return dev_err_probe(...);
>
> pattern. Yet, you may add another patch to clean up all of them:
> _probe(), _init(), _regmap_init() to use the same pattern everywhere.
>
Yes, a precursor patch to use dev_err_probe() throughout the probe only
functions in this driver would be excellent.
Jonathan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2024-04-20 13:07 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-14 17:53 [PATCH 0/4] iio: light: stk3310: support powering off during suspend Aren Moynihan
2024-04-14 17:53 ` Aren Moynihan
2024-04-14 17:57 ` [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply Aren Moynihan
2024-04-14 17:57 ` Aren Moynihan
2024-04-14 17:57 ` [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend Aren Moynihan
2024-04-14 17:57 ` Aren Moynihan
2024-04-15 14:04 ` Andy Shevchenko
2024-04-15 14:04 ` Andy Shevchenko
2024-04-18 15:06 ` Aren
2024-04-18 15:06 ` Aren
2024-04-18 15:56 ` Andy Shevchenko
2024-04-18 15:56 ` Andy Shevchenko
2024-04-18 17:50 ` Aren
2024-04-18 17:50 ` Aren
2024-04-18 18:19 ` Andy Shevchenko
2024-04-18 18:19 ` Andy Shevchenko
2024-04-20 13:04 ` Jonathan Cameron
2024-04-20 13:04 ` Jonathan Cameron
2024-04-14 17:57 ` [PATCH 3/4] iio: light: stk3310: log error if reading the chip id fails Aren Moynihan
2024-04-14 17:57 ` Aren Moynihan
2024-04-15 15:05 ` Andy Shevchenko
2024-04-15 15:05 ` Andy Shevchenko
2024-04-20 13:06 ` Jonathan Cameron
2024-04-20 13:06 ` Jonathan Cameron
2024-04-14 17:57 ` [PATCH 4/4] arm64: dts: allwinner: pinephone: Add power supply to stk3311 Aren Moynihan
2024-04-14 17:57 ` Aren Moynihan
2024-04-14 18:39 ` [PATCH 1/4] dt-bindings: iio: light: stk33xx: add regulator for vdd supply Krzysztof Kozlowski
2024-04-14 18:39 ` Krzysztof Kozlowski
2024-04-15 20:46 ` Jernej Škrabec
2024-04-15 20:46 ` Jernej Škrabec
2024-04-20 13:05 ` Jonathan Cameron
2024-04-20 13:05 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.