The status of each mux is read during suspend and stored in the private
memory of the mux_chip.
Then the state is restored during the resume.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
In this second version, as discussed with Peter, everything is done in the
mmio-mux driver.
A mux_mmio_set() function was added, and used during suspend stage to get
the status of the of the muxes.
This status is stored in the private memory of the mux_chip.
---
Changes in v2:
- Remove all modifications done in the mux subsystem
- Add a mux_mmio_set()
- Read the status of muxes during suspend and store in the private memory
  of the mux_chip.
- Use this status to restore muxes during resume.
- Link to v1: https://lore.kernel.org/r/20240613-mux-mmio-resume-support-v1-0-4525bf56024a@bootlin.com
---
 drivers/mux/mmio.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 9 deletions(-)

diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index 30a952c34365..30b84382637f 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -15,11 +15,25 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 
+struct mux_mmio {
+	struct regmap_field **fields;
+	unsigned int *hardware_states;
+};
+
+static int mux_mmio_get(struct mux_control *mux, int *state)
+{
+	struct mux_mmio *mux_mmio = mux_chip_priv(mux->chip);
+	unsigned int index = mux_control_get_index(mux);
+
+	return regmap_field_read(mux_mmio->fields[index], state);
+}
+
 static int mux_mmio_set(struct mux_control *mux, int state)
 {
-	struct regmap_field **fields = mux_chip_priv(mux->chip);
+	struct mux_mmio *mux_mmio = mux_chip_priv(mux->chip);
+	unsigned int index = mux_control_get_index(mux);
 
-	return regmap_field_write(fields[mux_control_get_index(mux)], state);
+	return regmap_field_write(mux_mmio->fields[index], state);
 }
 
 static const struct mux_control_ops mux_mmio_ops = {
@@ -37,8 +51,8 @@ static int mux_mmio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct regmap_field **fields;
 	struct mux_chip *mux_chip;
+	struct mux_mmio *mux_mmio;
 	struct regmap *regmap;
 	int num_fields;
 	int ret;
@@ -69,12 +83,20 @@ static int mux_mmio_probe(struct platform_device *pdev)
 	}
 	num_fields = ret / 2;
 
-	mux_chip = devm_mux_chip_alloc(dev, num_fields, num_fields *
-				       sizeof(*fields));
+	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(struct mux_mmio));
 	if (IS_ERR(mux_chip))
 		return PTR_ERR(mux_chip);
 
-	fields = mux_chip_priv(mux_chip);
+	mux_mmio = mux_chip_priv(mux_chip);
+
+	mux_mmio->fields = devm_kmalloc(dev, num_fields * sizeof(*mux_mmio->fields), GFP_KERNEL);
+	if (IS_ERR(mux_mmio->fields))
+		return PTR_ERR(mux_mmio->fields);
+
+	mux_mmio->hardware_states = devm_kmalloc(dev, num_fields *
+						 sizeof(*mux_mmio->hardware_states), GFP_KERNEL);
+	if (IS_ERR(mux_mmio->hardware_states))
+		return PTR_ERR(mux_mmio->hardware_states);
 
 	for (i = 0; i < num_fields; i++) {
 		struct mux_control *mux = &mux_chip->mux[i];
@@ -104,9 +126,9 @@ static int mux_mmio_probe(struct platform_device *pdev)
 			return -EINVAL;
 		}
 
-		fields[i] = devm_regmap_field_alloc(dev, regmap, field);
-		if (IS_ERR(fields[i])) {
-			ret = PTR_ERR(fields[i]);
+		mux_mmio->fields[i] = devm_regmap_field_alloc(dev, regmap, field);
+		if (IS_ERR(mux_mmio->fields[i])) {
+			ret = PTR_ERR(mux_mmio->fields[i]);
 			dev_err(dev, "bitfield %d: failed allocate: %d\n",
 				i, ret);
 			return ret;
@@ -130,13 +152,55 @@ static int mux_mmio_probe(struct platform_device *pdev)
 
 	mux_chip->ops = &mux_mmio_ops;
 
+	dev_set_drvdata(dev, mux_chip);
+
 	return devm_mux_chip_register(dev, mux_chip);
 }
 
+static int mux_mmio_suspend_noirq(struct device *dev)
+{
+	struct mux_chip *mux_chip = dev_get_drvdata(dev);
+	struct mux_mmio *mux_mmio = mux_chip_priv(mux_chip);
+	unsigned int state;
+	int ret, i;
+
+	for (i = 0; i < mux_chip->controllers; i++) {
+		ret = mux_mmio_get(&mux_chip->mux[i], &state);
+		if (ret) {
+			dev_err(dev, "control %u: error saving mux: %d\n", i, ret);
+			return ret;
+		}
+
+		mux_mmio->hardware_states[i] = state;
+	}
+
+	return 0;
+}
+
+static int mux_mmio_resume_noirq(struct device *dev)
+{
+	struct mux_chip *mux_chip = dev_get_drvdata(dev);
+	struct mux_mmio *mux_mmio = mux_chip_priv(mux_chip);
+	int ret, i;
+
+	for (i = 0; i < mux_chip->controllers; i++) {
+		ret = mux_mmio_set(&mux_chip->mux[i], mux_mmio->hardware_states[i]);
+		if (ret) {
+			dev_err(dev, "control %u: error restoring mux: %d\n", i, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(mux_mmio_pm_ops, mux_mmio_suspend_noirq, mux_mmio_resume_noirq);
+
 static struct platform_driver mux_mmio_driver = {
 	.driver = {
 		.name = "mmio-mux",
 		.of_match_table	= mux_mmio_dt_ids,
+		.pm = pm_sleep_ptr(&mux_mmio_pm_ops),
 	},
 	.probe = mux_mmio_probe,
 };

---
base-commit: caf614bf68351c7e9e38dd37e07539417c757813
change-id: 20240613-mux-mmio-resume-support-4f3b2a34a32a

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

On 9/11/24 3:53 AM, Thomas Richard wrote:
> The status of each mux is read during suspend and stored in the private
> memory of the mux_chip.
> Then the state is restored during the resume.
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
> In this second version, as discussed with Peter, everything is done in the
> mmio-mux driver.
> A mux_mmio_set() function was added, and used during suspend stage to get
> the status of the of the muxes.
> This status is stored in the private memory of the mux_chip.
> ---
> Changes in v2:
> - Remove all modifications done in the mux subsystem
> - Add a mux_mmio_set()
> - Read the status of muxes during suspend and store in the private memory
>    of the mux_chip.

Do you need this private memory? Since this is already using regmap, why
not use the regmap cache, then you can restore all the values on resume
with a simple call to regcache_sync().

Andrew

> - Use this status to restore muxes during resume.
> - Link to v1: https://lore.kernel.org/r/20240613-mux-mmio-resume-support-v1-0-4525bf56024a@bootlin.com
> ---
>   drivers/mux/mmio.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 73 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
> index 30a952c34365..30b84382637f 100644
> --- a/drivers/mux/mmio.c
> +++ b/drivers/mux/mmio.c
> @@ -15,11 +15,25 @@
>   #include <linux/property.h>
>   #include <linux/regmap.h>
>   
> +struct mux_mmio {
> +	struct regmap_field **fields;
> +	unsigned int *hardware_states;
> +};
> +
> +static int mux_mmio_get(struct mux_control *mux, int *state)
> +{
> +	struct mux_mmio *mux_mmio = mux_chip_priv(mux->chip);
> +	unsigned int index = mux_control_get_index(mux);
> +
> +	return regmap_field_read(mux_mmio->fields[index], state);
> +}
> +
>   static int mux_mmio_set(struct mux_control *mux, int state)
>   {
> -	struct regmap_field **fields = mux_chip_priv(mux->chip);
> +	struct mux_mmio *mux_mmio = mux_chip_priv(mux->chip);
> +	unsigned int index = mux_control_get_index(mux);
>   
> -	return regmap_field_write(fields[mux_control_get_index(mux)], state);
> +	return regmap_field_write(mux_mmio->fields[index], state);
>   }
>   
>   static const struct mux_control_ops mux_mmio_ops = {
> @@ -37,8 +51,8 @@ static int mux_mmio_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct device_node *np = dev->of_node;
> -	struct regmap_field **fields;
>   	struct mux_chip *mux_chip;
> +	struct mux_mmio *mux_mmio;
>   	struct regmap *regmap;
>   	int num_fields;
>   	int ret;
> @@ -69,12 +83,20 @@ static int mux_mmio_probe(struct platform_device *pdev)
>   	}
>   	num_fields = ret / 2;
>   
> -	mux_chip = devm_mux_chip_alloc(dev, num_fields, num_fields *
> -				       sizeof(*fields));
> +	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(struct mux_mmio));
>   	if (IS_ERR(mux_chip))
>   		return PTR_ERR(mux_chip);
>   
> -	fields = mux_chip_priv(mux_chip);
> +	mux_mmio = mux_chip_priv(mux_chip);
> +
> +	mux_mmio->fields = devm_kmalloc(dev, num_fields * sizeof(*mux_mmio->fields), GFP_KERNEL);
> +	if (IS_ERR(mux_mmio->fields))
> +		return PTR_ERR(mux_mmio->fields);
> +
> +	mux_mmio->hardware_states = devm_kmalloc(dev, num_fields *
> +						 sizeof(*mux_mmio->hardware_states), GFP_KERNEL);
> +	if (IS_ERR(mux_mmio->hardware_states))
> +		return PTR_ERR(mux_mmio->hardware_states);
>   
>   	for (i = 0; i < num_fields; i++) {
>   		struct mux_control *mux = &mux_chip->mux[i];
> @@ -104,9 +126,9 @@ static int mux_mmio_probe(struct platform_device *pdev)
>   			return -EINVAL;
>   		}
>   
> -		fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> -		if (IS_ERR(fields[i])) {
> -			ret = PTR_ERR(fields[i]);
> +		mux_mmio->fields[i] = devm_regmap_field_alloc(dev, regmap, field);
> +		if (IS_ERR(mux_mmio->fields[i])) {
> +			ret = PTR_ERR(mux_mmio->fields[i]);
>   			dev_err(dev, "bitfield %d: failed allocate: %d\n",
>   				i, ret);
>   			return ret;
> @@ -130,13 +152,55 @@ static int mux_mmio_probe(struct platform_device *pdev)
>   
>   	mux_chip->ops = &mux_mmio_ops;
>   
> +	dev_set_drvdata(dev, mux_chip);
> +
>   	return devm_mux_chip_register(dev, mux_chip);
>   }
>   
> +static int mux_mmio_suspend_noirq(struct device *dev)
> +{
> +	struct mux_chip *mux_chip = dev_get_drvdata(dev);
> +	struct mux_mmio *mux_mmio = mux_chip_priv(mux_chip);
> +	unsigned int state;
> +	int ret, i;
> +
> +	for (i = 0; i < mux_chip->controllers; i++) {
> +		ret = mux_mmio_get(&mux_chip->mux[i], &state);
> +		if (ret) {
> +			dev_err(dev, "control %u: error saving mux: %d\n", i, ret);
> +			return ret;
> +		}
> +
> +		mux_mmio->hardware_states[i] = state;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mux_mmio_resume_noirq(struct device *dev)
> +{
> +	struct mux_chip *mux_chip = dev_get_drvdata(dev);
> +	struct mux_mmio *mux_mmio = mux_chip_priv(mux_chip);
> +	int ret, i;
> +
> +	for (i = 0; i < mux_chip->controllers; i++) {
> +		ret = mux_mmio_set(&mux_chip->mux[i], mux_mmio->hardware_states[i]);
> +		if (ret) {
> +			dev_err(dev, "control %u: error restoring mux: %d\n", i, ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_NOIRQ_DEV_PM_OPS(mux_mmio_pm_ops, mux_mmio_suspend_noirq, mux_mmio_resume_noirq);
> +
>   static struct platform_driver mux_mmio_driver = {
>   	.driver = {
>   		.name = "mmio-mux",
>   		.of_match_table	= mux_mmio_dt_ids,
> +		.pm = pm_sleep_ptr(&mux_mmio_pm_ops),
>   	},
>   	.probe = mux_mmio_probe,
>   };
> 
> ---
> base-commit: caf614bf68351c7e9e38dd37e07539417c757813
> change-id: 20240613-mux-mmio-resume-support-4f3b2a34a32a
> 
> Best regards,

On 12/4/24 14:53, Andrew Davis wrote:
> On 9/11/24 3:53 AM, Thomas Richard wrote:
>> The status of each mux is read during suspend and stored in the private
>> memory of the mux_chip.
>> Then the state is restored during the resume.
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>> In this second version, as discussed with Peter, everything is done in
>> the
>> mmio-mux driver.
>> A mux_mmio_set() function was added, and used during suspend stage to get
>> the status of the of the muxes.
>> This status is stored in the private memory of the mux_chip.
>> ---
>> Changes in v2:
>> - Remove all modifications done in the mux subsystem
>> - Add a mux_mmio_set()
>> - Read the status of muxes during suspend and store in the private memory
>>    of the mux_chip.
> 
> Do you need this private memory? Since this is already using regmap, why
> not use the regmap cache, then you can restore all the values on resume
> with a simple call to regcache_sync().
The regmap can be set without cache.
As you inherit the regmap, you cannot assume that the cache is enabled.

Thomas

On 12/4/24 8:14 AM, Thomas Richard wrote:
> On 12/4/24 14:53, Andrew Davis wrote:
>> On 9/11/24 3:53 AM, Thomas Richard wrote:
>>> The status of each mux is read during suspend and stored in the private
>>> memory of the mux_chip.
>>> Then the state is restored during the resume.
>>>
>>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>>> ---
>>> In this second version, as discussed with Peter, everything is done in
>>> the
>>> mmio-mux driver.
>>> A mux_mmio_set() function was added, and used during suspend stage to get
>>> the status of the of the muxes.
>>> This status is stored in the private memory of the mux_chip.
>>> ---
>>> Changes in v2:
>>> - Remove all modifications done in the mux subsystem
>>> - Add a mux_mmio_set()
>>> - Read the status of muxes during suspend and store in the private memory
>>>     of the mux_chip.
>>
>> Do you need this private memory? Since this is already using regmap, why
>> not use the regmap cache, then you can restore all the values on resume
>> with a simple call to regcache_sync().
> The regmap can be set without cache.
> As you inherit the regmap, you cannot assume that the cache is enabled.
> 

Fair point, we don't really know much about the underlying registers.
One thing we don't know is if they are retained during suspend, so
saving and restoring might be unneeded in some cases. Maybe that isn't
something we can solve at this point, could be something that should
be done in the regmap framework instead..

For now, the rest of this patch seems good to me,

Reviewed-by: Andrew Davis <afd@ti.com>

On 9/11/24 10:53, Thomas Richard wrote:
> The status of each mux is read during suspend and stored in the private
> memory of the mux_chip.
> Then the state is restored during the resume.

Hello again Peter,

Do you have any comments ?
If not, is it possible to get this patch merged ?

Best Regards,

Thomas

On 9/11/24 10:53, Thomas Richard wrote:
> The status of each mux is read during suspend and stored in the private
> memory of the mux_chip.
> Then the state is restored during the resume.

Hello Peter,

Did you have time to look at this patch ?

Regards,

Thomas