diff mbox series

[v2,04/15] mux: add mux_chip_resume() function

Message ID 20240102-j7200-pcie-s2r-v2-4-8e4f7d228ec2@bootlin.com
State Superseded
Headers show
Series Add suspend to ram support for PCIe on J7200 | expand

Commit Message

Thomas Richard Jan. 26, 2024, 2:36 p.m. UTC
The mux_chip_resume() function restores a mux_chip using the cached state
of each mux.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mux/core.c         | 27 +++++++++++++++++++++++++++
 include/linux/mux/driver.h |  1 +
 2 files changed, 28 insertions(+)

Comments

Andy Shevchenko Jan. 26, 2024, 9:28 p.m. UTC | #1
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> The mux_chip_resume() function restores a mux_chip using the cached state
> of each mux.

...

> +int mux_chip_resume(struct mux_chip *mux_chip)
> +{
> +       int ret, i;
> +
> +       for (i = 0; i < mux_chip->controllers; ++i) {
> +               struct mux_control *mux = &mux_chip->mux[i];

> +               if (mux->cached_state != MUX_CACHE_UNKNOWN) {

  if (_state == ...)
    continue;

?

> +                       ret = mux_control_set(mux, mux->cached_state);
> +                       if (ret < 0) {
> +                               dev_err(&mux_chip->dev, "unable to restore state\n");
> +                               return ret;
> +                       }
> +               }
> +       }
> +       return 0;
> +}
Andi Shyti Jan. 29, 2024, 10:54 p.m. UTC | #2
Hi Thomas,

...

> +/**
> + * mux_chip_resume() - restores the mux-chip state
> + * @mux_chip: The mux-chip to resume.
> + *
> + * Restores the mux-chip state.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int mux_chip_resume(struct mux_chip *mux_chip)
> +{
> +	int ret, i;

you could put this 'ret' in the innermost indentation part, which
here is inside the 'if (...)' (or inside the 'for (...)' if you
follow Andy's suggestion).

Andi

> +	for (i = 0; i < mux_chip->controllers; ++i) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +
> +		if (mux->cached_state != MUX_CACHE_UNKNOWN) {
> +			ret = mux_control_set(mux, mux->cached_state);
> +			if (ret < 0) {
> +				dev_err(&mux_chip->dev, "unable to restore state\n");
> +				return ret;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_resume);
Peter Rosin Jan. 30, 2024, 8:25 a.m. UTC | #3
Hi!

2024-01-26 at 15:36, Thomas Richard wrote:
> The mux_chip_resume() function restores a mux_chip using the cached state
> of each mux.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/mux/core.c         | 27 +++++++++++++++++++++++++++
>  include/linux/mux/driver.h |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 775816112932..896f74b34eb8 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -215,6 +215,33 @@ void mux_chip_free(struct mux_chip *mux_chip)
>  }
>  EXPORT_SYMBOL_GPL(mux_chip_free);
>  
> +/**
> + * mux_chip_resume() - restores the mux-chip state
> + * @mux_chip: The mux-chip to resume.
> + *
> + * Restores the mux-chip state.
> + *
> + * Return: Zero on success or a negative errno on error.
> + */
> +int mux_chip_resume(struct mux_chip *mux_chip)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < mux_chip->controllers; ++i) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +
> +		if (mux->cached_state != MUX_CACHE_UNKNOWN) {
> +			ret = mux_control_set(mux, mux->cached_state);
> +			if (ret < 0) {
> +				dev_err(&mux_chip->dev, "unable to restore state\n");
> +				return ret;

I'm don't know what is expected of the core resume code on error,
but is it ok to return on first failure? Is it not better to try
to restore all muxes and return zero if all is well or the first
failure when something is up?

But maybe the resume is completely dead anyway if there is any
failure? In that case the above early return is fine, I guess...

Cheers,
Peter

> +			}
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mux_chip_resume);
> +
>  static void devm_mux_chip_release(struct device *dev, void *res)
>  {
>  	struct mux_chip *mux_chip = *(struct mux_chip **)res;
> diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
> index 18824064f8c0..2a7e5ec5d540 100644
> --- a/include/linux/mux/driver.h
> +++ b/include/linux/mux/driver.h
> @@ -88,6 +88,7 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
>  int mux_chip_register(struct mux_chip *mux_chip);
>  void mux_chip_unregister(struct mux_chip *mux_chip);
>  void mux_chip_free(struct mux_chip *mux_chip);
> +int mux_chip_resume(struct mux_chip *mux_chip);
>  
>  struct mux_chip *devm_mux_chip_alloc(struct device *dev,
>  				     unsigned int controllers,
>
Thomas Richard Jan. 30, 2024, 4:24 p.m. UTC | #4
On 1/30/24 09:25, Peter Rosin wrote:
> Hi!
> 
> 2024-01-26 at 15:36, Thomas Richard wrote:
>> The mux_chip_resume() function restores a mux_chip using the cached state
>> of each mux.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/mux/core.c         | 27 +++++++++++++++++++++++++++
>>  include/linux/mux/driver.h |  1 +
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 775816112932..896f74b34eb8 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -215,6 +215,33 @@ void mux_chip_free(struct mux_chip *mux_chip)
>>  }
>>  EXPORT_SYMBOL_GPL(mux_chip_free);
>>  
>> +/**
>> + * mux_chip_resume() - restores the mux-chip state
>> + * @mux_chip: The mux-chip to resume.
>> + *
>> + * Restores the mux-chip state.
>> + *
>> + * Return: Zero on success or a negative errno on error.
>> + */
>> +int mux_chip_resume(struct mux_chip *mux_chip)
>> +{
>> +	int ret, i;
>> +
>> +	for (i = 0; i < mux_chip->controllers; ++i) {
>> +		struct mux_control *mux = &mux_chip->mux[i];
>> +
>> +		if (mux->cached_state != MUX_CACHE_UNKNOWN) {
>> +			ret = mux_control_set(mux, mux->cached_state);
>> +			if (ret < 0) {
>> +				dev_err(&mux_chip->dev, "unable to restore state\n");
>> +				return ret;
> 
> I'm don't know what is expected of the core resume code on error,
> but is it ok to return on first failure? Is it not better to try
> to restore all muxes and return zero if all is well or the first
> failure when something is up?
> 
> But maybe the resume is completely dead anyway if there is any
> failure? In that case the above early return is fine, I guess...
> 

In the first iteration of this series (when it was done in mmio driver),
it restored all muxes and returned zero or the first failure.
I don't know why I changed the behaviour.
For me it's better to try to restores all muxes.
Thomas Richard Jan. 30, 2024, 4:38 p.m. UTC | #5
On 1/26/24 22:28, Andy Shevchenko wrote:
> On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>>
>> The mux_chip_resume() function restores a mux_chip using the cached state
>> of each mux.
> 
> ...
> 
>> +int mux_chip_resume(struct mux_chip *mux_chip)
>> +{
>> +       int ret, i;
>> +
>> +       for (i = 0; i < mux_chip->controllers; ++i) {
>> +               struct mux_control *mux = &mux_chip->mux[i];
> 
>> +               if (mux->cached_state != MUX_CACHE_UNKNOWN) {
> 
>   if (_state == ...)
>     continue;
> 
> ?

Yes it makes the code easier to read.
Fixed in next iteration.
diff mbox series

Patch

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 775816112932..896f74b34eb8 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -215,6 +215,33 @@  void mux_chip_free(struct mux_chip *mux_chip)
 }
 EXPORT_SYMBOL_GPL(mux_chip_free);
 
+/**
+ * mux_chip_resume() - restores the mux-chip state
+ * @mux_chip: The mux-chip to resume.
+ *
+ * Restores the mux-chip state.
+ *
+ * Return: Zero on success or a negative errno on error.
+ */
+int mux_chip_resume(struct mux_chip *mux_chip)
+{
+	int ret, i;
+
+	for (i = 0; i < mux_chip->controllers; ++i) {
+		struct mux_control *mux = &mux_chip->mux[i];
+
+		if (mux->cached_state != MUX_CACHE_UNKNOWN) {
+			ret = mux_control_set(mux, mux->cached_state);
+			if (ret < 0) {
+				dev_err(&mux_chip->dev, "unable to restore state\n");
+				return ret;
+			}
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mux_chip_resume);
+
 static void devm_mux_chip_release(struct device *dev, void *res)
 {
 	struct mux_chip *mux_chip = *(struct mux_chip **)res;
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 18824064f8c0..2a7e5ec5d540 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -88,6 +88,7 @@  struct mux_chip *mux_chip_alloc(struct device *dev,
 int mux_chip_register(struct mux_chip *mux_chip);
 void mux_chip_unregister(struct mux_chip *mux_chip);
 void mux_chip_free(struct mux_chip *mux_chip);
+int mux_chip_resume(struct mux_chip *mux_chip);
 
 struct mux_chip *devm_mux_chip_alloc(struct device *dev,
 				     unsigned int controllers,