[PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds some cleanups for the RZ/G2L ADC driver after the support
for the RZ/G3S SoC.

Thank you,
Claudiu Beznea

Changes in v4:
- open the devres group in its own function and rename the
  rzg2l_adc_probe() to rzg2l_adc_probe_helper() to have simpler code
- collected tags

Changes in v3:
- in patch 2/2 use a devres group for all the devm resources
  acquired in the driver's probe

Changes in v2:
- updated cover letter
- collected tags
- updated patch 1/2 to drop devres APIs from the point the
  runtime PM is enabled

Claudiu Beznea (2):
  iio: adc: rzg2l_adc: Open a devres group
  iio: adc: rzg2l: Cleanup suspend/resume path

 drivers/iio/adc/rzg2l_adc.c | 67 +++++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] iio: adc: rzg2l_adc: Open a devres group

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On all systems where the rzg2l_adc driver is used, the ADC clocks are part
of a PM domain. The code that implements the PM domains support is in
drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
domains support is registered with GENPD_FLAG_PM_CLK which, according to
the documentation, instructs genpd to use the PM clk framework while
powering on/off attached devices.

During probe, the ADC device is attached to the PM domain
controlling the ADC clocks. Similarly, during removal, the ADC device is
detached from the PM domain.

The detachment call stack is as follows:

device_driver_detach() ->
  device_release_driver_internal() ->
    __device_release_driver() ->
      device_remove() ->
        platform_remove() ->
          dev_pm_domain_detach()

During driver unbind, after the ADC device is detached from its PM domain,
the device_unbind_cleanup() function is called, which subsequently invokes
devres_release_all(). This function handles devres resource cleanup.

If runtime PM is enabled via devm_pm_runtime_enable(), the cleanup process
triggers the action or reset function for disabling runtime PM. This
function is pm_runtime_disable_action(), which leads to the following call
stack of interest when called:

pm_runtime_disable_action() ->
  pm_runtime_dont_use_autosuspend() ->
    __pm_runtime_use_autosuspend() ->
      update_autosuspend() ->
        rpm_idle()

The rpm_idle() function attempts to runtime resume the ADC device. However,
at the point it is called, the ADC device is no longer part of the PM
domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
APIs directly modifies hardware registers, the
rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
being enabled. This is because the PM domain no longer resumes along with
the ADC device. As a result, this leads to system aborts.

Open a devres group in the driver probe and release it in the driver
remove. This ensures the runtime PM is disabled (though the devres group)
after the rzg2l_adc_remove() finishes its execution avoiding the described
scenario.

Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- open the devres group in its own function and rename the rzg2l_adc_probe()
  to rzg2l_adc_probe_helper() to have simpler code
- @Prabhakar: for this ^^ I haven't yet collected your tag
- @Jonathan: as there is no outcome yet from [1] I only addressed your review
  comments on v3; please let me know how you consider this version.
  Thank you!

[1] https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com

Changes in v3:
- open a devres group in probe and release it in remove; the failure
  path of probe() was also updated to close the devres group
- dropped Ulf's Rb tag as the patch is different now
- updated the patch description to match the new approach

Note: a generic approach was proposed in [1] to have this in the platform
bus itself but wasn't seen acceptable.

[1] https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/

Changes in v2:
- collected Ulf's tag
- add a comment above pm_runtime_enable() explaining the reason
  it shouldn't be converted to devres
- drop devres calls that request IRQ and register IIO device
  as proposed in the review process: Ulf, I still kept you Rb
  tag; please let me know otherwise


 drivers/iio/adc/rzg2l_adc.c | 38 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 883c167c0670..761ba19b83a7 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -85,6 +85,7 @@ struct rzg2l_adc {
 	struct reset_control *adrstn;
 	const struct rzg2l_adc_data *data;
 	const struct rzg2l_adc_hw_params *hw_params;
+	void *devres_group_id;
 	struct completion completion;
 	struct mutex lock;
 	u16 last_val[RZG2L_ADC_MAX_CHANNELS];
@@ -424,7 +425,7 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 	return ret;
 }
 
-static int rzg2l_adc_probe(struct platform_device *pdev)
+static int rzg2l_adc_probe_helper(struct platform_device *pdev, void *devres_group_id)
 {
 	struct device *dev = &pdev->dev;
 	struct iio_dev *indio_dev;
@@ -438,6 +439,7 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 
 	adc = iio_priv(indio_dev);
 
+	adc->devres_group_id = devres_group_id;
 	adc->hw_params = device_get_match_data(dev);
 	if (!adc->hw_params || adc->hw_params->num_channels > RZG2L_ADC_MAX_CHANNELS)
 		return -EINVAL;
@@ -495,6 +497,39 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 	return devm_iio_device_register(dev, indio_dev);
 }
 
+static int rzg2l_adc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	void *devres_group_id;
+	int ret;
+
+	/*
+	 * Open a devres group to allow using devm_pm_runtime_enable()
+	 * w/o interfeering with dev_pm_genpd_detach() in the platform bus
+	 * remove. Otherwise, durring repeated unbind/bind operations,
+	 * the ADC may be runtime resumed when it is not part of its power
+	 * domain, leading to accessing ADC registers without its clocks
+	 * being enabled and its PM domain being turned on.
+	 */
+	devres_group_id = devres_open_group(dev, NULL, GFP_KERNEL);
+	if (!devres_group_id)
+		return -ENOMEM;
+
+	ret = rzg2l_adc_probe_helper(pdev, devres_group_id);
+	if (ret)
+		devres_release_group(dev, devres_group_id);
+
+	return ret;
+}
+
+static void rzg2l_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct rzg2l_adc *adc = iio_priv(indio_dev);
+
+	devres_release_group(&pdev->dev, adc->devres_group_id);
+}
+
 static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
 	.num_channels = 8,
 	.default_adcmp = 0xe,
@@ -614,6 +649,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
 
 static struct platform_driver rzg2l_adc_driver = {
 	.probe		= rzg2l_adc_probe,
+	.remove		= rzg2l_adc_remove,
 	.driver		= {
 		.name		= DRIVER_NAME,
 		.of_match_table = rzg2l_adc_match,
-- 
2.43.0

[PATCH v4 2/2] iio: adc: rzg2l: Cleanup suspend/resume path

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

There is no need to manually track the runtime PM status in the driver.
The pm_runtime_force_suspend() and pm_runtime_force_resume() functions
already call pm_runtime_status_suspended() to check the runtime PM state.

Additionally, avoid calling pm_runtime_put_autosuspend() during the
suspend/resume path, as this would decrease the usage counter of a
potential user that had the ADC open before the suspend/resume cycle.

Fixes: cb164d7c1526 ("iio: adc: rzg2l_adc: Add suspend/resume support")
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v4:
- collected tags

Changes in v3:
- collected tags

Changes in v2:
- none

 drivers/iio/adc/rzg2l_adc.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 761ba19b83a7..3c96684fff68 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -89,7 +89,6 @@ struct rzg2l_adc {
 	struct completion completion;
 	struct mutex lock;
 	u16 last_val[RZG2L_ADC_MAX_CHANNELS];
-	bool was_rpm_active;
 };
 
 /**
@@ -584,14 +583,9 @@ static int rzg2l_adc_suspend(struct device *dev)
 	};
 	int ret;
 
-	if (pm_runtime_suspended(dev)) {
-		adc->was_rpm_active = false;
-	} else {
-		ret = pm_runtime_force_suspend(dev);
-		if (ret)
-			return ret;
-		adc->was_rpm_active = true;
-	}
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
 
 	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	if (ret)
@@ -600,9 +594,7 @@ static int rzg2l_adc_suspend(struct device *dev)
 	return 0;
 
 rpm_restore:
-	if (adc->was_rpm_active)
-		pm_runtime_force_resume(dev);
-
+	pm_runtime_force_resume(dev);
 	return ret;
 }
 
@@ -620,11 +612,9 @@ static int rzg2l_adc_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (adc->was_rpm_active) {
-		ret = pm_runtime_force_resume(dev);
-		if (ret)
-			goto resets_restore;
-	}
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		goto resets_restore;
 
 	ret = rzg2l_adc_hw_init(dev, adc);
 	if (ret)
@@ -633,10 +623,7 @@ static int rzg2l_adc_resume(struct device *dev)
 	return 0;
 
 rpm_restore:
-	if (adc->was_rpm_active) {
-		pm_runtime_mark_last_busy(dev);
-		pm_runtime_put_autosuspend(dev);
-	}
+	pm_runtime_force_suspend(dev);
 resets_restore:
 	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	return ret;
-- 
2.43.0

Re: [PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[Jonathan Cameron]

On Mon, 24 Mar 2025 14:26:25 +0200
Claudiu <claudiu.beznea@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hi,
> 
> Series adds some cleanups for the RZ/G2L ADC driver after the support
> for the RZ/G3S SoC.

This doesn't address Dmitry's comment or highlight the outstanding
question he had to Greg KH on v3.  
I appreciate you want to get this fixed but I'd rather we got
it 'right' first time!

Also, please make sure to +CC anyone who engaged with an earlier version.

For reference of Greg if he sees this, Dmitry was expressing view that
the fix belongs in the bus layer not the individual drivers.
FWIW that feels like the right layer to me as well.

https://lore.kernel.org/all/Z8k8lDxA53gUJa0n@google.com/#t

Jonathan



> 
> Thank you,
> Claudiu Beznea
> 
> Changes in v4:
> - open the devres group in its own function and rename the
>   rzg2l_adc_probe() to rzg2l_adc_probe_helper() to have simpler code
> - collected tags
> 
> Changes in v3:
> - in patch 2/2 use a devres group for all the devm resources
>   acquired in the driver's probe
> 
> Changes in v2:
> - updated cover letter
> - collected tags
> - updated patch 1/2 to drop devres APIs from the point the
>   runtime PM is enabled
> 
> Claudiu Beznea (2):
>   iio: adc: rzg2l_adc: Open a devres group
>   iio: adc: rzg2l: Cleanup suspend/resume path
> 
>  drivers/iio/adc/rzg2l_adc.c | 67 +++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 22 deletions(-)
> 

Re: [PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[Greg KH]

On Thu, Mar 27, 2025 at 03:38:45PM +0000, Jonathan Cameron wrote:
> On Mon, 24 Mar 2025 14:26:25 +0200
> Claudiu <claudiu.beznea@tuxon.dev> wrote:
> 
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > 
> > Hi,
> > 
> > Series adds some cleanups for the RZ/G2L ADC driver after the support
> > for the RZ/G3S SoC.
> 
> This doesn't address Dmitry's comment or highlight the outstanding
> question he had to Greg KH on v3.  
> I appreciate you want to get this fixed but I'd rather we got
> it 'right' first time!
> 
> Also, please make sure to +CC anyone who engaged with an earlier version.
> 
> For reference of Greg if he sees this, Dmitry was expressing view that
> the fix belongs in the bus layer not the individual drivers.
> FWIW that feels like the right layer to me as well.
> 
> https://lore.kernel.org/all/Z8k8lDxA53gUJa0n@google.com/#t

As this is a PM question, Rafael would be the best to ask.

thanks,

greg k-h

Re: [PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[Claudiu Beznea]

Hi, Jonathan,

On 27.03.2025 17:38, Jonathan Cameron wrote:
> On Mon, 24 Mar 2025 14:26:25 +0200
> Claudiu <claudiu.beznea@tuxon.dev> wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Hi,
>>
>> Series adds some cleanups for the RZ/G2L ADC driver after the support
>> for the RZ/G3S SoC.
> 
> This doesn't address Dmitry's comment or highlight the outstanding
> question he had to Greg KH on v3.  
> I appreciate you want to get this fixed but I'd rather we got
> it 'right' first time!

My bad. As there was no input on platform bus patch I though this is not
the desired way of going forward. Sorry for that.

> 
> Also, please make sure to +CC anyone who engaged with an earlier version.

Ok, will do it.

> 
> For reference of Greg if he sees this, Dmitry was expressing view that
> the fix belongs in the bus layer not the individual drivers.
> FWIW that feels like the right layer to me as well.

To me, too.

Thank you,
Claudiu

> 
> https://lore.kernel.org/all/Z8k8lDxA53gUJa0n@google.com/#t
> 
> Jonathan
> 
> 
> 
>>
>> Thank you,
>> Claudiu Beznea
>>
>> Changes in v4:
>> - open the devres group in its own function and rename the
>>   rzg2l_adc_probe() to rzg2l_adc_probe_helper() to have simpler code
>> - collected tags
>>
>> Changes in v3:
>> - in patch 2/2 use a devres group for all the devm resources
>>   acquired in the driver's probe
>>
>> Changes in v2:
>> - updated cover letter
>> - collected tags
>> - updated patch 1/2 to drop devres APIs from the point the
>>   runtime PM is enabled
>>
>> Claudiu Beznea (2):
>>   iio: adc: rzg2l_adc: Open a devres group
>>   iio: adc: rzg2l: Cleanup suspend/resume path
>>
>>  drivers/iio/adc/rzg2l_adc.c | 67 +++++++++++++++++++++++++------------
>>  1 file changed, 45 insertions(+), 22 deletions(-)
>>
> 

Re: [PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[Jonathan Cameron]

On Thu, 27 Mar 2025 17:22:20 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Mar 27, 2025 at 03:38:45PM +0000, Jonathan Cameron wrote:
> > On Mon, 24 Mar 2025 14:26:25 +0200
> > Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >   
> > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > 
> > > Hi,
> > > 
> > > Series adds some cleanups for the RZ/G2L ADC driver after the support
> > > for the RZ/G3S SoC.  
> > 
> > This doesn't address Dmitry's comment or highlight the outstanding
> > question he had to Greg KH on v3.  
> > I appreciate you want to get this fixed but I'd rather we got
> > it 'right' first time!
> > 
> > Also, please make sure to +CC anyone who engaged with an earlier version.
> > 
> > For reference of Greg if he sees this, Dmitry was expressing view that
> > the fix belongs in the bus layer not the individual drivers.
> > FWIW that feels like the right layer to me as well.
> > 
> > https://lore.kernel.org/all/Z8k8lDxA53gUJa0n@google.com/#t  
> 
> As this is a PM question, Rafael would be the best to ask.

Sure. Perhaps Rafael missed previous discussion, so I've messaged
him directly to draw his attention to the series.

Claudiu, please include all relevant people in +CC.  Don't trim
it down to those effected by a particular solution as has happened
here. +CC Rafael, Daniel and Ulf.

Thanks,

Jonathan


> 
> thanks,
> 
> greg k-h

Re: [PATCH v4 0/2] iio: rzg2l_adc: Cleanups for rzg2l_adc driver

[Claudiu Beznea]

Hi, Jonathan, Daniel,

On 30.03.2025 18:36, Jonathan Cameron wrote:
> On Thu, 27 Mar 2025 17:22:20 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
>> On Thu, Mar 27, 2025 at 03:38:45PM +0000, Jonathan Cameron wrote:
>>> On Mon, 24 Mar 2025 14:26:25 +0200
>>> Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>   
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> Hi,
>>>>
>>>> Series adds some cleanups for the RZ/G2L ADC driver after the support
>>>> for the RZ/G3S SoC.  
>>>
>>> This doesn't address Dmitry's comment or highlight the outstanding
>>> question he had to Greg KH on v3.  
>>> I appreciate you want to get this fixed but I'd rather we got
>>> it 'right' first time!
>>>
>>> Also, please make sure to +CC anyone who engaged with an earlier version.
>>>
>>> For reference of Greg if he sees this, Dmitry was expressing view that
>>> the fix belongs in the bus layer not the individual drivers.
>>> FWIW that feels like the right layer to me as well.
>>>
>>> https://lore.kernel.org/all/Z8k8lDxA53gUJa0n@google.com/#t  
>>
>> As this is a PM question, Rafael would be the best to ask.
> 
> Sure. Perhaps Rafael missed previous discussion, so I've messaged
> him directly to draw his attention to the series.
> 
> Claudiu, please include all relevant people in +CC.  Don't trim
> it down to those effected by a particular solution as has happened
> here. +CC Rafael, Daniel and Ulf.

As the discussion [1] is progressing very slowly:

Jonathan: do you consider having this series as a temporary solution?

Daniel: do you consider having the fix in [2] as a temporary solution (of
course, with the adjustments suggested by Geert)?

There is also [3] for which a similar approach was proposed. From what I
understood from the discussions on [3], Bjorn is OK with the current solution.

I am taking the responsibility to do the necessary adjustments to all these
drivers once there is a resolution for [1].

Thank you,
Claudiu

[1]
https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
[2]
https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/
[3]
https://lore.kernel.org/all/20250430103236.3511989-6-claudiu.beznea.uj@bp.renesas.com/