From: Jonathan Cameron <jic23@kernel.org> To: Aren Moynihan <aren@peacevolution.org> Cc: "Lars-Peter Clausen" <lars@metafoo.de>, "Rob Herring" <robh@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "Chen-Yu Tsai" <wens@csie.org>, "Jernej Skrabec" <jernej.skrabec@gmail.com>, "Samuel Holland" <samuel@sholland.org>, "Liam Girdwood" <lgirdwood@gmail.com>, "Mark Brown" <broonie@kernel.org>, "Andy Shevchenko" <andy.shevchenko@gmail.com>, "Ondrej Jirman" <megi@xff.cz>, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, linux-iio@vger.kernel.org, phone-devel@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, "Willow Barraco" <contact@willowbarraco.fr> Subject: Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend Date: Sat, 20 Apr 2024 14:04:17 +0100 [thread overview] Message-ID: <20240420140417.12b2bf8e@jic23-huawei> (raw) In-Reply-To: <20240414175716.958831-2-aren@peacevolution.org> 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)
WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron <jic23@kernel.org> To: Aren Moynihan <aren@peacevolution.org> Cc: "Lars-Peter Clausen" <lars@metafoo.de>, "Rob Herring" <robh@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>, "Chen-Yu Tsai" <wens@csie.org>, "Jernej Skrabec" <jernej.skrabec@gmail.com>, "Samuel Holland" <samuel@sholland.org>, "Liam Girdwood" <lgirdwood@gmail.com>, "Mark Brown" <broonie@kernel.org>, "Andy Shevchenko" <andy.shevchenko@gmail.com>, "Ondrej Jirman" <megi@xff.cz>, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, linux-iio@vger.kernel.org, phone-devel@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, "Willow Barraco" <contact@willowbarraco.fr> Subject: Re: [PATCH 2/4] iio: light: stk3310: Implement vdd supply and power it off during suspend Date: Sat, 20 Apr 2024 14:04:17 +0100 [thread overview] Message-ID: <20240420140417.12b2bf8e@jic23-huawei> (raw) In-Reply-To: <20240414175716.958831-2-aren@peacevolution.org> 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
next prev parent reply other threads:[~2024-04-20 13:04 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20240420140417.12b2bf8e@jic23-huawei \ --to=jic23@kernel.org \ --cc=andy.shevchenko@gmail.com \ --cc=aren@peacevolution.org \ --cc=broonie@kernel.org \ --cc=conor+dt@kernel.org \ --cc=contact@willowbarraco.fr \ --cc=devicetree@vger.kernel.org \ --cc=jernej.skrabec@gmail.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=lars@metafoo.de \ --cc=lgirdwood@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sunxi@lists.linux.dev \ --cc=megi@xff.cz \ --cc=phone-devel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=samuel@sholland.org \ --cc=u.kleine-koenig@pengutronix.de \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.