[PATCH 00/14] iio: adc: rzg2l_adc: Add support for RZ/G3S

[PATCH 00/14] iio: adc: rzg2l_adc: Add support for RZ/G3S

[Claudiu]

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

Hi,

This series adds ADC support for the Renesas RZ/G3S SoC.

Series is organized as follows:
- patch 01/14:		adds clocks, reset and power domain support
			for ADC
- patches 02-06/14:	cleanup patches to ease the addition of RZ/G3S
			support
- patches 07/14:	enables runtime PM autosuspend support
- patches 08-12/14:	add RZ/G3S support, including suspend-to-RAM
			functionality
- patches 13-14/14:	add device tree support

Merge strategy, if any:
- patch 01/14 can go through the Renesas tree
- patches 02-12/14 can go through the IIO tree
- patch 13-14/14 can go through the Renesas tree

Thank you,
Claudiu Beznea

Claudiu Beznea (14):
  clk: renesas: r9a08g045: Add clocks, resets and power domain support
    for the ADC IP
  iio: adc: rzg2l_adc: Use devres helpers to request pre-deasserted
    reset controls
  iio: adc: rzg2l_adc: Simplify the runtime PM code
  iio: adc: rzg2l_adc: Switch to RUNTIME_PM_OPS() and pm_ptr()
  iio: adc: rzg2l_adc: Use read_poll_timeout()
  iio: adc: rzg2l_adc: Simplify the locking scheme in
    rzg2l_adc_read_raw()
  iio: adc: rzg2l_adc: Enable runtime PM autosuspend support
  iio: adc: rzg2l_adc: Prepare for the addition of RZ/G3S support
  iio: adc: rzg2l_adc: Add support for channel 8
  iio: adc: rzg2l_adc: Add suspend/resume support
  dt-bindings: iio: adc: renesas,rzg2l-adc: Document RZ/G3S SoC
  iio: adc: rzg2l_adc: Add support for Renesas RZ/G3S
  arm64: dts: renesas: r9a08g045: Add ADC node
  arm64: dts: renesas: rzg3s-smarc-som: Enable ADC

 .../bindings/iio/adc/renesas,rzg2l-adc.yaml   |  37 +-
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    |  53 +++
 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     |   4 +
 drivers/clk/renesas/r9a08g045-cpg.c           |   7 +
 drivers/iio/adc/rzg2l_adc.c                   | 414 ++++++++++--------
 5 files changed, 323 insertions(+), 192 deletions(-)

-- 
2.39.2

[PATCH 01/14] clk: renesas: r9a08g045: Add clocks, resets and power domain support for the ADC IP

[Claudiu]

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

Add clocks, resets and power domains for ADC IP available on the Renesas
RZ/G3S SoC.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/clk/renesas/r9a08g045-cpg.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index b2ae8cdc4723..a124e3339d84 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -187,6 +187,7 @@ static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = {
 	DEF_FIXED("OSC", R9A08G045_OSCCLK, CLK_EXTAL, 1, 1),
 	DEF_FIXED("OSC2", R9A08G045_OSCCLK2, CLK_EXTAL, 1, 3),
 	DEF_FIXED("HP", R9A08G045_CLK_HP, CLK_PLL6, 1, 2),
+	DEF_FIXED("TSU", R9A08G045_CLK_TSU, CLK_PLL2_DIV2, 1, 8),
 };
 
 static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
@@ -225,6 +226,8 @@ static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
 	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3),
 	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0),
 	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0),
+	DEF_MOD("adc_adclk",		R9A08G045_ADC_ADCLK, R9A08G045_CLK_TSU, 0x5a8, 0),
+	DEF_MOD("adc_pclk",		R9A08G045_ADC_PCLK, R9A08G045_CLK_TSU, 0x5a8, 1),
 	DEF_MOD("vbat_bclk",		R9A08G045_VBAT_BCLK, R9A08G045_OSCCLK, 0x614, 0),
 };
 
@@ -252,6 +255,8 @@ static const struct rzg2l_reset r9a08g045_resets[] = {
 	DEF_RST(R9A08G045_GPIO_RSTN, 0x898, 0),
 	DEF_RST(R9A08G045_GPIO_PORT_RESETN, 0x898, 1),
 	DEF_RST(R9A08G045_GPIO_SPARE_RESETN, 0x898, 2),
+	DEF_RST(R9A08G045_ADC_PRESETN, 0x8a8, 0),
+	DEF_RST(R9A08G045_ADC_ADRST_N, 0x8a8, 1),
 	DEF_RST(R9A08G045_VBAT_BRESETN, 0x914, 0),
 };
 
@@ -306,6 +311,8 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(13)), 0),
 	DEF_PD("scif0",		R9A08G045_PD_SCIF0,
 				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(1)), 0),
+	DEF_PD("adc",		R9A08G045_PD_ADC,
+				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(14)), 0),
 	DEF_PD("vbat",		R9A08G045_PD_VBAT,
 				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(8)),
 				GENPD_FLAG_ALWAYS_ON),
-- 
2.39.2

[PATCH 02/14] iio: adc: rzg2l_adc: Use devres helpers to request pre-deasserted reset controls

[Claudiu]

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

Starting with commit d872bed85036 ("reset: Add devres helpers to request
pre-deasserted reset controls"), devres helpers are available to simplify
the process of requesting pre-deasserted reset controls. Update the
rzg2l_adc driver to utilize these helpers, reducing complexity in this
way.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 37 ++-----------------------------------
 1 file changed, 2 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index cd3a7e46ea53..7039949a7554 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -415,11 +415,6 @@ static void rzg2l_adc_pm_runtime_set_suspended(void *data)
 	pm_runtime_set_suspended(dev->parent);
 }
 
-static void rzg2l_adc_reset_assert(void *data)
-{
-	reset_control_assert(data);
-}
-
 static int rzg2l_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -456,46 +451,18 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(adc->adclk);
 	}
 
-	adc->adrstn = devm_reset_control_get_exclusive(dev, "adrst-n");
+	adc->adrstn = devm_reset_control_get_exclusive_deasserted(dev, "adrst-n");
 	if (IS_ERR(adc->adrstn)) {
 		dev_err(dev, "failed to get adrstn\n");
 		return PTR_ERR(adc->adrstn);
 	}
 
-	adc->presetn = devm_reset_control_get_exclusive(dev, "presetn");
+	adc->presetn = devm_reset_control_get_exclusive_deasserted(dev, "presetn");
 	if (IS_ERR(adc->presetn)) {
 		dev_err(dev, "failed to get presetn\n");
 		return PTR_ERR(adc->presetn);
 	}
 
-	ret = reset_control_deassert(adc->adrstn);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret);
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_adc_reset_assert, adc->adrstn);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n",
-			ret);
-		return ret;
-	}
-
-	ret = reset_control_deassert(adc->presetn);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret);
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_adc_reset_assert, adc->presetn);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n",
-			ret);
-		return ret;
-	}
-
 	ret = rzg2l_adc_hw_init(adc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret);
-- 
2.39.2

Re: [PATCH 02/14] iio: adc: rzg2l_adc: Use devres helpers to request pre-deasserted reset controls

[Jonathan Cameron]

On Tue,  3 Dec 2024 13:13:02 +0200
Claudiu <claudiu.beznea@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Starting with commit d872bed85036 ("reset: Add devres helpers to request
> pre-deasserted reset controls"), devres helpers are available to simplify
> the process of requesting pre-deasserted reset controls. Update the
> rzg2l_adc driver to utilize these helpers, reducing complexity in this
> way.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Hi Claudia,

Minor comments below.

Thanks

Jonathan

> ---
>  drivers/iio/adc/rzg2l_adc.c | 37 ++-----------------------------------
>  1 file changed, 2 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index cd3a7e46ea53..7039949a7554 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -415,11 +415,6 @@ static void rzg2l_adc_pm_runtime_set_suspended(void *data)
>  	pm_runtime_set_suspended(dev->parent);
>  }
>  
> -static void rzg2l_adc_reset_assert(void *data)
> -{
> -	reset_control_assert(data);
> -}
> -
>  static int rzg2l_adc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -456,46 +451,18 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  		return PTR_ERR(adc->adclk);
>  	}
>  
> -	adc->adrstn = devm_reset_control_get_exclusive(dev, "adrst-n");
> +	adc->adrstn = devm_reset_control_get_exclusive_deasserted(dev, "adrst-n");
>  	if (IS_ERR(adc->adrstn)) {
>  		dev_err(dev, "failed to get adrstn\n");
I'd be tempted to keep the error message from below rather than this one.
As we can only conclude the deassert failed to happen, not whether it was the
get step or the deassert itself.

Also, use dev_err_probe() throughout and definitely for anything you are touching
in this series. Ideally add a patch converting all the other places where it
is useful in things only called from probe.
	return dev_err_probe(dev, PTR_ERR(adc->adrstn),
			     "failed to deassert adrstn pin\n");

>  		return PTR_ERR(adc->adrstn);
>  	}
>  
> -	adc->presetn = devm_reset_control_get_exclusive(dev, "presetn");
> +	adc->presetn = devm_reset_control_get_exclusive_deasserted(dev, "presetn");
>  	if (IS_ERR(adc->presetn)) {
>  		dev_err(dev, "failed to get presetn\n");
>  		return PTR_ERR(adc->presetn);
>  	}
>  
> -	ret = reset_control_deassert(adc->adrstn);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to deassert adrstn pin, %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = devm_add_action_or_reset(&pdev->dev,
> -				       rzg2l_adc_reset_assert, adc->adrstn);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register adrstn assert devm action, %d\n",
> -			ret);
> -		return ret;
> -	}
> -
> -	ret = reset_control_deassert(adc->presetn);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to deassert presetn pin, %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = devm_add_action_or_reset(&pdev->dev,
> -				       rzg2l_adc_reset_assert, adc->presetn);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to register presetn assert devm action, %d\n",
> -			ret);
> -		return ret;
> -	}
> -
>  	ret = rzg2l_adc_hw_init(adc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret);

[PATCH 03/14] iio: adc: rzg2l_adc: Simplify the runtime PM code

[Claudiu]

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

All Renesas SoCs using the rzg2l_adc driver manage ADC clocks through PM
domains. Calling pm_runtime_{resume_and_get, put_sync}() implicitly sets
the state of the clocks. As a result, the code in the rzg2l_adc driver that
explicitly manages ADC clocks can be removed, leading to simpler and
cleaner implementation.

Additionally, replace the use of rzg2l_adc_set_power() with direct PM
runtime API calls to further simplify and clean up the code.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 100 ++++++++----------------------------
 1 file changed, 20 insertions(+), 80 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 7039949a7554..a17690ecbdc3 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -8,7 +8,6 @@
  */
 
 #include <linux/bitfield.h>
-#include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/iio/iio.h>
@@ -69,8 +68,6 @@ struct rzg2l_adc_data {
 
 struct rzg2l_adc {
 	void __iomem *base;
-	struct clk *pclk;
-	struct clk *adclk;
 	struct reset_control *presetn;
 	struct reset_control *adrstn;
 	struct completion completion;
@@ -188,29 +185,18 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 	return 0;
 }
 
-static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
-{
-	struct device *dev = indio_dev->dev.parent;
-
-	if (on)
-		return pm_runtime_resume_and_get(dev);
-
-	return pm_runtime_put_sync(dev);
-}
-
 static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
 {
+	struct device *dev = indio_dev->dev.parent;
 	int ret;
 
-	ret = rzg2l_adc_set_power(indio_dev, true);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;
 
 	ret = rzg2l_adc_conversion_setup(adc, ch);
-	if (ret) {
-		rzg2l_adc_set_power(indio_dev, false);
-		return ret;
-	}
+	if (ret)
+		goto rpm_put;
 
 	reinit_completion(&adc->completion);
 
@@ -219,12 +205,14 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
 	if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
 		rzg2l_adc_writel(adc, RZG2L_ADINT,
 				 rzg2l_adc_readl(adc, RZG2L_ADINT) & ~RZG2L_ADINT_INTEN_MASK);
-		rzg2l_adc_start_stop(adc, false);
-		rzg2l_adc_set_power(indio_dev, false);
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
 	}
 
-	return rzg2l_adc_set_power(indio_dev, false);
+	rzg2l_adc_start_stop(adc, false);
+
+rpm_put:
+	pm_runtime_put_sync(dev);
+	return ret;
 }
 
 static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
@@ -352,13 +340,13 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 	return 0;
 }
 
-static int rzg2l_adc_hw_init(struct rzg2l_adc *adc)
+static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 {
 	int timeout = 5;
 	u32 reg;
 	int ret;
 
-	ret = clk_prepare_enable(adc->pclk);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;
 
@@ -396,25 +384,10 @@ static int rzg2l_adc_hw_init(struct rzg2l_adc *adc)
 	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
 
 exit_hw_init:
-	clk_disable_unprepare(adc->pclk);
-
+	pm_runtime_put_sync(dev);
 	return ret;
 }
 
-static void rzg2l_adc_pm_runtime_disable(void *data)
-{
-	struct device *dev = data;
-
-	pm_runtime_disable(dev->parent);
-}
-
-static void rzg2l_adc_pm_runtime_set_suspended(void *data)
-{
-	struct device *dev = data;
-
-	pm_runtime_set_suspended(dev->parent);
-}
-
 static int rzg2l_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -439,18 +412,6 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 	if (IS_ERR(adc->base))
 		return PTR_ERR(adc->base);
 
-	adc->pclk = devm_clk_get(dev, "pclk");
-	if (IS_ERR(adc->pclk)) {
-		dev_err(dev, "Failed to get pclk");
-		return PTR_ERR(adc->pclk);
-	}
-
-	adc->adclk = devm_clk_get(dev, "adclk");
-	if (IS_ERR(adc->adclk)) {
-		dev_err(dev, "Failed to get adclk");
-		return PTR_ERR(adc->adclk);
-	}
-
 	adc->adrstn = devm_reset_control_get_exclusive_deasserted(dev, "adrst-n");
 	if (IS_ERR(adc->adrstn)) {
 		dev_err(dev, "failed to get adrstn\n");
@@ -463,7 +424,13 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(adc->presetn);
 	}
 
-	ret = rzg2l_adc_hw_init(adc);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = rzg2l_adc_hw_init(dev, adc);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to initialize ADC HW, %d\n", ret);
 		return ret;
@@ -480,26 +447,12 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 
 	init_completion(&adc->completion);
 
-	platform_set_drvdata(pdev, indio_dev);
-
 	indio_dev->name = DRIVER_NAME;
 	indio_dev->info = &rzg2l_adc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adc->data->channels;
 	indio_dev->num_channels = adc->data->num_channels;
 
-	pm_runtime_set_suspended(dev);
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_adc_pm_runtime_set_suspended, &indio_dev->dev);
-	if (ret)
-		return ret;
-
-	pm_runtime_enable(dev);
-	ret = devm_add_action_or_reset(&pdev->dev,
-				       rzg2l_adc_pm_runtime_disable, &indio_dev->dev);
-	if (ret)
-		return ret;
-
 	return devm_iio_device_register(dev, indio_dev);
 }
 
@@ -515,8 +468,6 @@ static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
 
 	rzg2l_adc_pwr(adc, false);
-	clk_disable_unprepare(adc->adclk);
-	clk_disable_unprepare(adc->pclk);
 
 	return 0;
 }
@@ -525,17 +476,6 @@ static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
-	int ret;
-
-	ret = clk_prepare_enable(adc->pclk);
-	if (ret)
-		return ret;
-
-	ret = clk_prepare_enable(adc->adclk);
-	if (ret) {
-		clk_disable_unprepare(adc->pclk);
-		return ret;
-	}
 
 	rzg2l_adc_pwr(adc, true);
 
-- 
2.39.2

Re: [PATCH 03/14] iio: adc: rzg2l_adc: Simplify the runtime PM code

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 2356 bytes --]

Hi Claudiu,

On 03/12/2024 11:13, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> All Renesas SoCs using the rzg2l_adc driver manage ADC clocks through PM
> domains. Calling pm_runtime_{resume_and_get, put_sync}() implicitly sets
> the state of the clocks. As a result, the code in the rzg2l_adc driver that
> explicitly manages ADC clocks can be removed, leading to simpler and
> cleaner implementation.
> 
> Additionally, replace the use of rzg2l_adc_set_power() with direct PM
> runtime API calls to further simplify and clean up the code.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/iio/adc/rzg2l_adc.c | 100 ++++++++----------------------------
>  1 file changed, 20 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 7039949a7554..a17690ecbdc3 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -8,7 +8,6 @@
>   */
>  
>  #include <linux/bitfield.h>
> -#include <linux/clk.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/iio/iio.h>
> @@ -69,8 +68,6 @@ struct rzg2l_adc_data {
>  
>  struct rzg2l_adc {
>  	void __iomem *base;
> -	struct clk *pclk;
> -	struct clk *adclk;
>  	struct reset_control *presetn;
>  	struct reset_control *adrstn;
>  	struct completion completion;
> @@ -188,29 +185,18 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>  	return 0;
>  }
>  
> -static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
> -{
> -	struct device *dev = indio_dev->dev.parent;
> -
> -	if (on)
> -		return pm_runtime_resume_and_get(dev);
> -
> -	return pm_runtime_put_sync(dev);
> -}
> -
>  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
>  {
> +	struct device *dev = indio_dev->dev.parent;
>  	int ret;
>  
> -	ret = rzg2l_adc_set_power(indio_dev, true);
> +	ret = pm_runtime_resume_and_get(dev);
>  	if (ret)
>  		return ret;

Should we check (ret < 0) here instead of just (ret)? According to the
docs [1], pm_runtime_resume_and_get() can return 1 if the device is
already active.

[1]: https://docs.kernel.org/power/runtime_pm.html#runtime-pm-device-helper-functions

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 03/14] iio: adc: rzg2l_adc: Simplify the runtime PM code

[Claudiu Beznea]

Hi, Paul,

On 03.12.2024 14:53, Paul Barker wrote:
> Hi Claudiu,
> 
> On 03/12/2024 11:13, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> All Renesas SoCs using the rzg2l_adc driver manage ADC clocks through PM
>> domains. Calling pm_runtime_{resume_and_get, put_sync}() implicitly sets
>> the state of the clocks. As a result, the code in the rzg2l_adc driver that
>> explicitly manages ADC clocks can be removed, leading to simpler and
>> cleaner implementation.
>>
>> Additionally, replace the use of rzg2l_adc_set_power() with direct PM
>> runtime API calls to further simplify and clean up the code.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/iio/adc/rzg2l_adc.c | 100 ++++++++----------------------------
>>  1 file changed, 20 insertions(+), 80 deletions(-)
>>
>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
>> index 7039949a7554..a17690ecbdc3 100644
>> --- a/drivers/iio/adc/rzg2l_adc.c
>> +++ b/drivers/iio/adc/rzg2l_adc.c
>> @@ -8,7 +8,6 @@
>>   */
>>  
>>  #include <linux/bitfield.h>
>> -#include <linux/clk.h>
>>  #include <linux/completion.h>
>>  #include <linux/delay.h>
>>  #include <linux/iio/iio.h>
>> @@ -69,8 +68,6 @@ struct rzg2l_adc_data {
>>  
>>  struct rzg2l_adc {
>>  	void __iomem *base;
>> -	struct clk *pclk;
>> -	struct clk *adclk;
>>  	struct reset_control *presetn;
>>  	struct reset_control *adrstn;
>>  	struct completion completion;
>> @@ -188,29 +185,18 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>>  	return 0;
>>  }
>>  
>> -static int rzg2l_adc_set_power(struct iio_dev *indio_dev, bool on)
>> -{
>> -	struct device *dev = indio_dev->dev.parent;
>> -
>> -	if (on)
>> -		return pm_runtime_resume_and_get(dev);
>> -
>> -	return pm_runtime_put_sync(dev);
>> -}
>> -
>>  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
>>  {
>> +	struct device *dev = indio_dev->dev.parent;
>>  	int ret;
>>  
>> -	ret = rzg2l_adc_set_power(indio_dev, true);
>> +	ret = pm_runtime_resume_and_get(dev);
>>  	if (ret)
>>  		return ret;
> 
> Should we check (ret < 0) here instead of just (ret)? According to the
> docs [1], pm_runtime_resume_and_get() can return 1 if the device is
> already active.

The v6.13-rc1 implementation of pm_runtime_resume_and_get() is:

static inline int pm_runtime_resume_and_get(struct device *dev)
{
	int ret;

	ret = __pm_runtime_resume(dev, RPM_GET_PUT);
	if (ret < 0) {
		pm_runtime_put_noidle(dev);
		return ret;
	}

	return 0;
}

It can return zero or negative error number.

Thank you,
Claudiu


> 
> [1]: https://docs.kernel.org/power/runtime_pm.html#runtime-pm-device-helper-functions
> 
> Thanks,
> 

Re: [PATCH 03/14] iio: adc: rzg2l_adc: Simplify the runtime PM code

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1203 bytes --]

Hi Claudiu,

On 03/12/2024 13:40, Claudiu Beznea wrote:
> On 03.12.2024 14:53, Paul Barker wrote:
>> On 03/12/2024 11:13, Claudiu wrote:
>>>  static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
>>>  {
>>> +	struct device *dev = indio_dev->dev.parent;
>>>  	int ret;
>>>  
>>> -	ret = rzg2l_adc_set_power(indio_dev, true);
>>> +	ret = pm_runtime_resume_and_get(dev);
>>>  	if (ret)
>>>  		return ret;
>>
>> Should we check (ret < 0) here instead of just (ret)? According to the
>> docs [1], pm_runtime_resume_and_get() can return 1 if the device is
>> already active.
> 
> The v6.13-rc1 implementation of pm_runtime_resume_and_get() is:
> 
> static inline int pm_runtime_resume_and_get(struct device *dev)
> {
> 	int ret;
> 
> 	ret = __pm_runtime_resume(dev, RPM_GET_PUT);
> 	if (ret < 0) {
> 		pm_runtime_put_noidle(dev);
> 		return ret;
> 	}
> 
> 	return 0;
> }
> 
> It can return zero or negative error number.

Ah, ok. The docs say that pm_runtime_resume_and_get() will "return the
result of pm_runtime_resume" (which can return 1), but it doesn't do
that exactly. I'll send a fix for the docs.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[PATCH 04/14] iio: adc: rzg2l_adc: Switch to RUNTIME_PM_OPS() and pm_ptr()

[Claudiu]

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

The use of SET_RUNTIME_PM_OPS() is now deprecated and requires
__maybe_unused annotations to avoid warnings about unused functions.
Switching to RUNTIME_PM_OPS() and pm_ptr() eliminates the need for such
annotations because the compiler can directly reference the runtime PM
functions, thereby suppressing the warnings. As a result, the
__maybe_unused markings can be removed.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index a17690ecbdc3..5437b21c4e70 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -462,7 +462,7 @@ static const struct of_device_id rzg2l_adc_match[] = {
 };
 MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
 
-static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
+static int rzg2l_adc_pm_runtime_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
@@ -472,7 +472,7 @@ static int __maybe_unused rzg2l_adc_pm_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
+static int rzg2l_adc_pm_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
@@ -483,9 +483,7 @@ static int __maybe_unused rzg2l_adc_pm_runtime_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops rzg2l_adc_pm_ops = {
-	SET_RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend,
-			   rzg2l_adc_pm_runtime_resume,
-			   NULL)
+	RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, rzg2l_adc_pm_runtime_resume, NULL)
 };
 
 static struct platform_driver rzg2l_adc_driver = {
@@ -493,7 +491,7 @@ static struct platform_driver rzg2l_adc_driver = {
 	.driver		= {
 		.name		= DRIVER_NAME,
 		.of_match_table = rzg2l_adc_match,
-		.pm		= &rzg2l_adc_pm_ops,
+		.pm		= pm_ptr(&rzg2l_adc_pm_ops),
 	},
 };
 
-- 
2.39.2

[PATCH 05/14] iio: adc: rzg2l_adc: Use read_poll_timeout()

[Claudiu]

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

Replace the driver-specific implementation with the read_poll_timeout()
function. This change simplifies the code and improves maintainability by
leveraging the standardized helper.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 5437b21c4e70..62932f9295b6 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -13,6 +13,7 @@
 #include <linux/iio/iio.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -112,7 +113,7 @@ static void rzg2l_adc_pwr(struct rzg2l_adc *adc, bool on)
 
 static void rzg2l_adc_start_stop(struct rzg2l_adc *adc, bool start)
 {
-	int timeout = 5;
+	int ret;
 	u32 reg;
 
 	reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
@@ -125,15 +126,10 @@ static void rzg2l_adc_start_stop(struct rzg2l_adc *adc, bool start)
 	if (start)
 		return;
 
-	do {
-		usleep_range(100, 200);
-		reg = rzg2l_adc_readl(adc, RZG2L_ADM(0));
-		timeout--;
-		if (!timeout) {
-			pr_err("%s stopping ADC timed out\n", __func__);
-			break;
-		}
-	} while (((reg & RZG2L_ADM0_ADBSY) || (reg & RZG2L_ADM0_ADCE)));
+	ret = read_poll_timeout(rzg2l_adc_readl, reg, !(reg & (RZG2L_ADM0_ADBSY | RZG2L_ADM0_ADCE)),
+				200, 1000, true, adc, RZG2L_ADM(0));
+	if (ret)
+		pr_err("%s stopping ADC timed out\n", __func__);
 }
 
 static void rzg2l_set_trigger(struct rzg2l_adc *adc)
@@ -342,7 +338,6 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 
 static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 {
-	int timeout = 5;
 	u32 reg;
 	int ret;
 
@@ -355,14 +350,10 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 	reg |= RZG2L_ADM0_SRESB;
 	rzg2l_adc_writel(adc, RZG2L_ADM(0), reg);
 
-	while (!(rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_SRESB)) {
-		if (!timeout) {
-			ret = -EBUSY;
-			goto exit_hw_init;
-		}
-		timeout--;
-		usleep_range(100, 200);
-	}
+	ret = read_poll_timeout(rzg2l_adc_readl, reg, reg & RZG2L_ADM0_SRESB,
+				200, 1000, false, adc, RZG2L_ADM(0));
+	if (ret)
+		goto exit_hw_init;
 
 	/* Only division by 4 can be set */
 	reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
-- 
2.39.2

[PATCH 06/14] iio: adc: rzg2l_adc: Simplify the locking scheme in rzg2l_adc_read_raw()

[Claudiu]

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

Simplify the locking scheme in rzg2l_adc_read_raw() by saving the converted
value only if the rzg2l_adc_conversion() returns success. The approach
simplifies the addition of thermal sensor support (that will be done in the
next commits). The downside is that the ret variable need to be checked
twice.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 62932f9295b6..eed2944bd98d 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -227,14 +227,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
 		mutex_lock(&adc->lock);
 		ch = chan->channel & RZG2L_ADC_CHN_MASK;
 		ret = rzg2l_adc_conversion(indio_dev, adc, ch);
-		if (ret) {
-			mutex_unlock(&adc->lock);
-			return ret;
-		}
-		*val = adc->last_val[ch];
+		if (!ret)
+			*val = adc->last_val[ch];
 		mutex_unlock(&adc->lock);
 
-		return IIO_VAL_INT;
+		return ret ? ret : IIO_VAL_INT;
 
 	default:
 		return -EINVAL;
-- 
2.39.2

Re: [PATCH 06/14] iio: adc: rzg2l_adc: Simplify the locking scheme in rzg2l_adc_read_raw()

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1465 bytes --]

Hi Claudiu,

On 03/12/2024 11:13, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Simplify the locking scheme in rzg2l_adc_read_raw() by saving the converted
> value only if the rzg2l_adc_conversion() returns success. The approach
> simplifies the addition of thermal sensor support (that will be done in the
> next commits). The downside is that the ret variable need to be checked
> twice.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/iio/adc/rzg2l_adc.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 62932f9295b6..eed2944bd98d 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -227,14 +227,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
>  		mutex_lock(&adc->lock);
>  		ch = chan->channel & RZG2L_ADC_CHN_MASK;
>  		ret = rzg2l_adc_conversion(indio_dev, adc, ch);
> -		if (ret) {
> -			mutex_unlock(&adc->lock);
> -			return ret;
> -		}
> -		*val = adc->last_val[ch];
> +		if (!ret)
> +			*val = adc->last_val[ch];
>  		mutex_unlock(&adc->lock);
>  
> -		return IIO_VAL_INT;
> +		return ret ? ret : IIO_VAL_INT;

It would be maybe slightly neater to use:

	if (!ret) {
		*val = adc->last_val[ch];
		ret = IIO_VAL_INT;
	}
	mutex_unlock(&adc->lock);

	return ret;

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 06/14] iio: adc: rzg2l_adc: Simplify the locking scheme in rzg2l_adc_read_raw()

[Jonathan Cameron]

On Tue, 3 Dec 2024 13:03:29 +0000
Paul Barker <paul.barker.ct@bp.renesas.com> wrote:

> Hi Claudiu,
> 
> On 03/12/2024 11:13, Claudiu wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > 
> > Simplify the locking scheme in rzg2l_adc_read_raw() by saving the converted
> > value only if the rzg2l_adc_conversion() returns success. The approach
> > simplifies the addition of thermal sensor support (that will be done in the
> > next commits). The downside is that the ret variable need to be checked
> > twice.
> > 
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> >  drivers/iio/adc/rzg2l_adc.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > index 62932f9295b6..eed2944bd98d 100644
> > --- a/drivers/iio/adc/rzg2l_adc.c
> > +++ b/drivers/iio/adc/rzg2l_adc.c
> > @@ -227,14 +227,11 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
> >  		mutex_lock(&adc->lock);
> >  		ch = chan->channel & RZG2L_ADC_CHN_MASK;
> >  		ret = rzg2l_adc_conversion(indio_dev, adc, ch);
> > -		if (ret) {
> > -			mutex_unlock(&adc->lock);
> > -			return ret;
> > -		}
> > -		*val = adc->last_val[ch];
> > +		if (!ret)
> > +			*val = adc->last_val[ch];
> >  		mutex_unlock(&adc->lock);
> >  
> > -		return IIO_VAL_INT;
> > +		return ret ? ret : IIO_VAL_INT;  
> 
> It would be maybe slightly neater to use:
> 
> 	if (!ret) {
> 		*val = adc->last_val[ch];
> 		ret = IIO_VAL_INT;
> 	}
> 	mutex_unlock(&adc->lock);
> 
> 	return ret;
> 
Better I think to use {} for scope and
guard(mutex)()
...
if (ret)
	return ret;

*val = adc->last_val[ch];

Where possible keeping the error path as the out of line element is
easier to follow on basis that is most common pattern so what a reviewers
eye is 'trained' to see.

Jonathan

> Thanks,
> 

[PATCH 07/14] iio: adc: rzg2l_adc: Enable runtime PM autosuspend support

[Claudiu]

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

Enable runtime PM autosuspend support for the rzg2l_adc driver. With this
change, consecutive conversion requests will no longer cause the device to
be runtime-enabled/disabled after each request. Instead, the device will
transition based on the delay configured by the user.

This approach reduces the frequency of hardware register access during
runtime PM suspend/resume cycles, thereby saving CPU cycles. The default
autosuspend delay is set to zero to maintain the previous driver behavior.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index eed2944bd98d..fda8b42ded81 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -207,7 +207,8 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
 	rzg2l_adc_start_stop(adc, false);
 
 rpm_put:
-	pm_runtime_put_sync(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 	return ret;
 }
 
@@ -372,7 +373,8 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
 
 exit_hw_init:
-	pm_runtime_put_sync(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 	return ret;
 }
 
@@ -412,6 +414,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 		return PTR_ERR(adc->presetn);
 	}
 
+	/* Default 0 for power saving. Can be overridden via sysfs. */
+	pm_runtime_set_autosuspend_delay(dev, 0);
+	pm_runtime_use_autosuspend(dev);
 	ret = devm_pm_runtime_enable(dev);
 	if (ret)
 		return ret;
-- 
2.39.2

Re: [PATCH 07/14] iio: adc: rzg2l_adc: Enable runtime PM autosuspend support

[Jonathan Cameron]

On Tue,  3 Dec 2024 13:13:07 +0200
Claudiu <claudiu.beznea@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Enable runtime PM autosuspend support for the rzg2l_adc driver. With this
> change, consecutive conversion requests will no longer cause the device to
> be runtime-enabled/disabled after each request. Instead, the device will
> transition based on the delay configured by the user.
> 
> This approach reduces the frequency of hardware register access during
> runtime PM suspend/resume cycles, thereby saving CPU cycles. The default
> autosuspend delay is set to zero to maintain the previous driver behavior.

Unless you have a weird user who is polling slow enough to not trigger
autosuspend with a non zero period, but is still saving power I'm not convinced
anyone will notice if you just enable this for a sensible autosuspend delay.
There will of course be a small increase in power usage for each read but
hopefully that is trivial.

So I'd not go with a default of 0, though what value makes sense depends
on the likely usecase + how much power is saved by going to sleep.

If you really want to keep 0 I don't mind that much, just seems odd!

Jonathan

> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/iio/adc/rzg2l_adc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index eed2944bd98d..fda8b42ded81 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -207,7 +207,8 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
>  	rzg2l_adc_start_stop(adc, false);
>  
>  rpm_put:
> -	pm_runtime_put_sync(dev);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
>  	return ret;
>  }
>  
> @@ -372,7 +373,8 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
>  	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
>  
>  exit_hw_init:
> -	pm_runtime_put_sync(dev);
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
>  	return ret;
>  }
>  
> @@ -412,6 +414,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  		return PTR_ERR(adc->presetn);
>  	}
>  
> +	/* Default 0 for power saving. Can be overridden via sysfs. */
> +	pm_runtime_set_autosuspend_delay(dev, 0);
> +	pm_runtime_use_autosuspend(dev);
>  	ret = devm_pm_runtime_enable(dev);
>  	if (ret)
>  		return ret;

Re: [PATCH 07/14] iio: adc: rzg2l_adc: Enable runtime PM autosuspend support

[Claudiu Beznea]

Hi, Jonathan,

On 03.12.2024 22:00, Jonathan Cameron wrote:
> On Tue,  3 Dec 2024 13:13:07 +0200
> Claudiu <claudiu.beznea@tuxon.dev> wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Enable runtime PM autosuspend support for the rzg2l_adc driver. With this
>> change, consecutive conversion requests will no longer cause the device to
>> be runtime-enabled/disabled after each request. Instead, the device will
>> transition based on the delay configured by the user.
>>
>> This approach reduces the frequency of hardware register access during
>> runtime PM suspend/resume cycles, thereby saving CPU cycles. The default
>> autosuspend delay is set to zero to maintain the previous driver behavior.
> 
> Unless you have a weird user who is polling slow enough to not trigger
> autosuspend with a non zero period, but is still saving power I'm not convinced
> anyone will notice if you just enable this for a sensible autosuspend delay.
> There will of course be a small increase in power usage for each read but
> hopefully that is trivial.
> 
> So I'd not go with a default of 0, though what value makes sense depends
> on the likely usecase + how much power is saved by going to sleep.
> 
> If you really want to keep 0 I don't mind that much, just seems odd!

I agree with you. I chose it like this as I got internal request (on other
drivers enabling autosuspend support) to keep the previous behavior in place.

Thank you for your review,
Claudiu

> 
> Jonathan
> 
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/iio/adc/rzg2l_adc.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
>> index eed2944bd98d..fda8b42ded81 100644
>> --- a/drivers/iio/adc/rzg2l_adc.c
>> +++ b/drivers/iio/adc/rzg2l_adc.c
>> @@ -207,7 +207,8 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
>>  	rzg2l_adc_start_stop(adc, false);
>>  
>>  rpm_put:
>> -	pm_runtime_put_sync(dev);
>> +	pm_runtime_mark_last_busy(dev);
>> +	pm_runtime_put_autosuspend(dev);
>>  	return ret;
>>  }
>>  
>> @@ -372,7 +373,8 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
>>  	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
>>  
>>  exit_hw_init:
>> -	pm_runtime_put_sync(dev);
>> +	pm_runtime_mark_last_busy(dev);
>> +	pm_runtime_put_autosuspend(dev);
>>  	return ret;
>>  }
>>  
>> @@ -412,6 +414,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>  		return PTR_ERR(adc->presetn);
>>  	}
>>  
>> +	/* Default 0 for power saving. Can be overridden via sysfs. */
>> +	pm_runtime_set_autosuspend_delay(dev, 0);
>> +	pm_runtime_use_autosuspend(dev);
>>  	ret = devm_pm_runtime_enable(dev);
>>  	if (ret)
>>  		return ret;
> 

RE: [PATCH 07/14] iio: adc: rzg2l_adc: Enable runtime PM autosuspend support

[Biju Das]

Hi All,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Sent: 04 December 2024 08:32
> Subject: Re: [PATCH 07/14] iio: adc: rzg2l_adc: Enable runtime PM autosuspend support
> 
> Hi, Jonathan,
> 
> On 03.12.2024 22:00, Jonathan Cameron wrote:
> > On Tue,  3 Dec 2024 13:13:07 +0200
> > Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> Enable runtime PM autosuspend support for the rzg2l_adc driver. With
> >> this change, consecutive conversion requests will no longer cause the
> >> device to be runtime-enabled/disabled after each request. Instead,
> >> the device will transition based on the delay configured by the user.
> >>
> >> This approach reduces the frequency of hardware register access
> >> during runtime PM suspend/resume cycles, thereby saving CPU cycles.
> >> The default autosuspend delay is set to zero to maintain the previous driver behavior.
> >
> > Unless you have a weird user who is polling slow enough to not trigger
> > autosuspend with a non zero period, but is still saving power I'm not
> > convinced anyone will notice if you just enable this for a sensible autosuspend delay.
> > There will of course be a small increase in power usage for each read
> > but hopefully that is trivial.
> >
> > So I'd not go with a default of 0, though what value makes sense
> > depends on the likely usecase + how much power is saved by going to sleep.
> >
> > If you really want to keep 0 I don't mind that much, just seems odd!
> 
> I agree with you. I chose it like this as I got internal request (on other drivers enabling
> autosuspend support) to keep the previous behavior in place.


On I2C driver after every transfer we turn off the clocks to save power(transaction based).

If we introduce autosupend with 100msec, we are consuming power for 100msec. So, to keep
previous behaviour, we are setting the value to 0, when auto suspend feature is introduced there.

ADC case maybe different, you can have sensible delay between reading the temperature/voltage
and next request as it uses polling.

Cheers,
Biju

[PATCH 08/14] iio: adc: rzg2l_adc: Prepare for the addition of RZ/G3S support

[Claudiu]

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

The ADC IP available on the RZ/G3S differs slightly from the one found on
the RZ/G2L. The identified differences are as follows:
- different number of channels (one being used for temperature conversion);
  consequently, various registers differ
- different default sampling periods
- the RZ/G3S variant lacks the ADVIC register.

To accommodate these differences, the rzg2l_adc driver has been updated by
introducing the struct rzg2l_adc_hw_params, which encapsulates the
hardware-specific differences between the IP variants. A pointer to an
object of type struct rzg2l_adc_hw_params is embedded in
struct rzg2l_adc_data.

Additionally, the completion member of struct rzg2l_adc_data was relocated
to avoid potential padding, if any.

The code has been adjusted to utilize hardware-specific parameters stored
in the new structure instead of relying on plain macros.

The check of chan->channel in rzg2l_adc_read_raw() function, against the
driver specific mask was removed as the subsystem should have already
been done this before reaching the rzg2l_adc_read_raw() function.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 92 ++++++++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index fda8b42ded81..aff41152ebf8 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -32,20 +32,15 @@
 #define RZG2L_ADM1_MS			BIT(2)
 #define RZG2L_ADM1_BS			BIT(4)
 #define RZG2L_ADM1_EGA_MASK		GENMASK(13, 12)
-#define RZG2L_ADM2_CHSEL_MASK		GENMASK(7, 0)
 #define RZG2L_ADM3_ADIL_MASK		GENMASK(31, 24)
 #define RZG2L_ADM3_ADCMP_MASK		GENMASK(23, 16)
-#define RZG2L_ADM3_ADCMP_E		FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe)
-#define RZG2L_ADM3_ADSMP_MASK		GENMASK(15, 0)
 
 #define RZG2L_ADINT			0x20
-#define RZG2L_ADINT_INTEN_MASK		GENMASK(7, 0)
 #define RZG2L_ADINT_CSEEN		BIT(16)
 #define RZG2L_ADINT_INTS		BIT(31)
 
 #define RZG2L_ADSTS			0x24
 #define RZG2L_ADSTS_CSEST		BIT(16)
-#define RZG2L_ADSTS_INTST_MASK		GENMASK(7, 0)
 
 #define RZG2L_ADIVC			0x28
 #define RZG2L_ADIVC_DIVADC_MASK		GENMASK(8, 0)
@@ -56,12 +51,26 @@
 #define RZG2L_ADCR(n)			(0x30 + ((n) * 0x4))
 #define RZG2L_ADCR_AD_MASK		GENMASK(11, 0)
 
-#define RZG2L_ADSMP_DEFAULT_SAMPLING	0x578
-
-#define RZG2L_ADC_MAX_CHANNELS		8
-#define RZG2L_ADC_CHN_MASK		0x7
 #define RZG2L_ADC_TIMEOUT		usecs_to_jiffies(1 * 4)
 
+/**
+ * struct rzg2l_adc_hw_params - ADC hardware specific parameters
+ * @default_adsmp: default ADC sampling period (see ADM3 register)
+ * @adsmp_mask: ADC sampling period mask (see ADM3 register)
+ * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
+ * @default_adcmp: default ADC cmp (see ADM3 register)
+ * @num_channels: number of supported channels
+ * @adivc: specifies if ADVIC register is available
+ */
+struct rzg2l_adc_hw_params {
+	u16 default_adsmp;
+	u16 adsmp_mask;
+	u16 adint_inten_mask;
+	u8 default_adcmp;
+	u8 num_channels;
+	bool adivc;
+};
+
 struct rzg2l_adc_data {
 	const struct iio_chan_spec *channels;
 	u8 num_channels;
@@ -71,10 +80,11 @@ struct rzg2l_adc {
 	void __iomem *base;
 	struct reset_control *presetn;
 	struct reset_control *adrstn;
-	struct completion completion;
 	const struct rzg2l_adc_data *data;
+	const struct rzg2l_adc_hw_params *hw_params;
+	u16 *last_val;
+	struct completion completion;
 	struct mutex lock;
-	u16 last_val[RZG2L_ADC_MAX_CHANNELS];
 };
 
 static const char * const rzg2l_adc_channel_name[] = {
@@ -153,6 +163,7 @@ static void rzg2l_set_trigger(struct rzg2l_adc *adc)
 
 static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 {
+	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	u32 reg;
 
 	if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
@@ -162,7 +173,7 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 
 	/* Select analog input channel subjected to conversion. */
 	reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
-	reg &= ~RZG2L_ADM2_CHSEL_MASK;
+	reg &= ~GENMASK(hw_params->num_channels - 1, 0);
 	reg |= BIT(ch);
 	rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
 
@@ -174,7 +185,7 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 	 */
 	reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
 	reg &= ~RZG2L_ADINT_INTS;
-	reg &= ~RZG2L_ADINT_INTEN_MASK;
+	reg &= ~hw_params->adint_inten_mask;
 	reg |= (RZG2L_ADINT_CSEEN | BIT(ch));
 	rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
 
@@ -183,6 +194,7 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 
 static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
 {
+	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	struct device *dev = indio_dev->dev.parent;
 	int ret;
 
@@ -200,7 +212,7 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
 
 	if (!wait_for_completion_timeout(&adc->completion, RZG2L_ADC_TIMEOUT)) {
 		rzg2l_adc_writel(adc, RZG2L_ADINT,
-				 rzg2l_adc_readl(adc, RZG2L_ADINT) & ~RZG2L_ADINT_INTEN_MASK);
+				 rzg2l_adc_readl(adc, RZG2L_ADINT) & ~hw_params->adint_inten_mask);
 		ret = -ETIMEDOUT;
 	}
 
@@ -217,8 +229,8 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
 			      int *val, int *val2, long mask)
 {
 	struct rzg2l_adc *adc = iio_priv(indio_dev);
+	u8 ch = chan->channel;
 	int ret;
-	u8 ch;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -226,7 +238,6 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		mutex_lock(&adc->lock);
-		ch = chan->channel & RZG2L_ADC_CHN_MASK;
 		ret = rzg2l_adc_conversion(indio_dev, adc, ch);
 		if (!ret)
 			*val = adc->last_val[ch];
@@ -254,6 +265,7 @@ static const struct iio_info rzg2l_adc_iio_info = {
 static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
 {
 	struct rzg2l_adc *adc = dev_id;
+	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	unsigned long intst;
 	u32 reg;
 	int ch;
@@ -266,11 +278,11 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
-	intst = reg & RZG2L_ADSTS_INTST_MASK;
+	intst = reg & GENMASK(hw_params->num_channels - 1, 0);
 	if (!intst)
 		return IRQ_NONE;
 
-	for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS)
+	for_each_set_bit(ch, &intst, hw_params->num_channels)
 		adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) & RZG2L_ADCR_AD_MASK;
 
 	/* clear the channel interrupt */
@@ -283,6 +295,7 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
 
 static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc)
 {
+	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	struct iio_chan_spec *chan_array;
 	struct rzg2l_adc_data *data;
 	unsigned int channel;
@@ -300,7 +313,7 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 		return -ENODEV;
 	}
 
-	if (num_channels > RZG2L_ADC_MAX_CHANNELS) {
+	if (num_channels > hw_params->num_channels) {
 		dev_err(&pdev->dev, "num of channel children out of range\n");
 		return -EINVAL;
 	}
@@ -316,7 +329,7 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 		if (ret)
 			return ret;
 
-		if (channel >= RZG2L_ADC_MAX_CHANNELS)
+		if (channel >= hw_params->num_channels)
 			return -EINVAL;
 
 		chan_array[i].type = IIO_VOLTAGE;
@@ -336,6 +349,7 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 
 static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 {
+	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	u32 reg;
 	int ret;
 
@@ -353,11 +367,13 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 	if (ret)
 		goto exit_hw_init;
 
-	/* Only division by 4 can be set */
-	reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
-	reg &= ~RZG2L_ADIVC_DIVADC_MASK;
-	reg |= RZG2L_ADIVC_DIVADC_4;
-	rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
+	if (hw_params->adivc) {
+		/* Only division by 4 can be set */
+		reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
+		reg &= ~RZG2L_ADIVC_DIVADC_MASK;
+		reg |= RZG2L_ADIVC_DIVADC_4;
+		rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
+	}
 
 	/*
 	 * Setup AMD3
@@ -368,8 +384,10 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 	reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
 	reg &= ~RZG2L_ADM3_ADIL_MASK;
 	reg &= ~RZG2L_ADM3_ADCMP_MASK;
-	reg &= ~RZG2L_ADM3_ADSMP_MASK;
-	reg |= (RZG2L_ADM3_ADCMP_E | RZG2L_ADSMP_DEFAULT_SAMPLING);
+	reg &= ~hw_params->adsmp_mask;
+	reg |= FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, hw_params->default_adcmp) |
+	       hw_params->default_adsmp;
+
 	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
 
 exit_hw_init:
@@ -392,6 +410,15 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 
 	adc = iio_priv(indio_dev);
 
+	adc->hw_params = device_get_match_data(dev);
+	if (!adc->hw_params)
+		return -EINVAL;
+
+	adc->last_val = devm_kcalloc(dev, adc->hw_params->num_channels,
+				     sizeof(*adc->last_val), GFP_KERNEL);
+	if (!adc->last_val)
+		return -ENOMEM;
+
 	ret = rzg2l_adc_parse_properties(pdev, adc);
 	if (ret)
 		return ret;
@@ -449,8 +476,17 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 	return devm_iio_device_register(dev, indio_dev);
 }
 
+static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
+	.num_channels = 8,
+	.default_adcmp = 0xe,
+	.default_adsmp = 0x578,
+	.adsmp_mask = GENMASK(15, 0),
+	.adint_inten_mask = GENMASK(7, 0),
+	.adivc = true
+};
+
 static const struct of_device_id rzg2l_adc_match[] = {
-	{ .compatible = "renesas,rzg2l-adc",},
+	{ .compatible = "renesas,rzg2l-adc", .data = &rzg2l_hw_params },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
-- 
2.39.2

Re: [PATCH 08/14] iio: adc: rzg2l_adc: Prepare for the addition of RZ/G3S support

[Jonathan Cameron]

On Tue,  3 Dec 2024 13:13:08 +0200
Claudiu <claudiu.beznea@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The ADC IP available on the RZ/G3S differs slightly from the one found on
> the RZ/G2L. The identified differences are as follows:
> - different number of channels (one being used for temperature conversion);
>   consequently, various registers differ
> - different default sampling periods
> - the RZ/G3S variant lacks the ADVIC register.
> 
> To accommodate these differences, the rzg2l_adc driver has been updated by
> introducing the struct rzg2l_adc_hw_params, which encapsulates the
> hardware-specific differences between the IP variants. A pointer to an
> object of type struct rzg2l_adc_hw_params is embedded in
> struct rzg2l_adc_data.
> 
> Additionally, the completion member of struct rzg2l_adc_data was relocated
> to avoid potential padding, if any.
> 
> The code has been adjusted to utilize hardware-specific parameters stored
> in the new structure instead of relying on plain macros.
> 
> The check of chan->channel in rzg2l_adc_read_raw() function, against the
> driver specific mask was removed as the subsystem should have already
> been done this before reaching the rzg2l_adc_read_raw() function.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/iio/adc/rzg2l_adc.c | 92 ++++++++++++++++++++++++++-----------
>  1 file changed, 64 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index fda8b42ded81..aff41152ebf8 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -32,20 +32,15 @@
>  #define RZG2L_ADM1_MS			BIT(2)
>  #define RZG2L_ADM1_BS			BIT(4)
>  #define RZG2L_ADM1_EGA_MASK		GENMASK(13, 12)
> -#define RZG2L_ADM2_CHSEL_MASK		GENMASK(7, 0)
>  #define RZG2L_ADM3_ADIL_MASK		GENMASK(31, 24)
>  #define RZG2L_ADM3_ADCMP_MASK		GENMASK(23, 16)
> -#define RZG2L_ADM3_ADCMP_E		FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe)
> -#define RZG2L_ADM3_ADSMP_MASK		GENMASK(15, 0)
>  
>  #define RZG2L_ADINT			0x20
> -#define RZG2L_ADINT_INTEN_MASK		GENMASK(7, 0)
>  #define RZG2L_ADINT_CSEEN		BIT(16)
>  #define RZG2L_ADINT_INTS		BIT(31)
>  
>  #define RZG2L_ADSTS			0x24
>  #define RZG2L_ADSTS_CSEST		BIT(16)
> -#define RZG2L_ADSTS_INTST_MASK		GENMASK(7, 0)
>  
>  #define RZG2L_ADIVC			0x28
>  #define RZG2L_ADIVC_DIVADC_MASK		GENMASK(8, 0)
> @@ -56,12 +51,26 @@
>  #define RZG2L_ADCR(n)			(0x30 + ((n) * 0x4))
>  #define RZG2L_ADCR_AD_MASK		GENMASK(11, 0)
>  
> -#define RZG2L_ADSMP_DEFAULT_SAMPLING	0x578
> -
> -#define RZG2L_ADC_MAX_CHANNELS		8
> -#define RZG2L_ADC_CHN_MASK		0x7
>  #define RZG2L_ADC_TIMEOUT		usecs_to_jiffies(1 * 4)
>  
> +/**
> + * struct rzg2l_adc_hw_params - ADC hardware specific parameters
> + * @default_adsmp: default ADC sampling period (see ADM3 register)
> + * @adsmp_mask: ADC sampling period mask (see ADM3 register)
> + * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
> + * @default_adcmp: default ADC cmp (see ADM3 register)
> + * @num_channels: number of supported channels
> + * @adivc: specifies if ADVIC register is available
> + */
> +struct rzg2l_adc_hw_params {
> +	u16 default_adsmp;
> +	u16 adsmp_mask;
> +	u16 adint_inten_mask;
> +	u8 default_adcmp;
> +	u8 num_channels;
> +	bool adivc;
> +};
> +
>  struct rzg2l_adc_data {
>  	const struct iio_chan_spec *channels;
>  	u8 num_channels;
> @@ -71,10 +80,11 @@ struct rzg2l_adc {
>  	void __iomem *base;
>  	struct reset_control *presetn;
>  	struct reset_control *adrstn;
> -	struct completion completion;
>  	const struct rzg2l_adc_data *data;
> +	const struct rzg2l_adc_hw_params *hw_params;
> +	u16 *last_val;
> +	struct completion completion;
>  	struct mutex lock;
> -	u16 last_val[RZG2L_ADC_MAX_CHANNELS];

Just make this big enough for the max device.  Chances are it will make little or
no difference to this allocation and nice to avoid the dynamic part.

Feel free to add a runtime check to make sure this is big enough to avoid any
future problems with forgetting to update it.

>  };
>

> @@ -392,6 +410,15 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  
>  	adc = iio_priv(indio_dev);
>  
> +	adc->hw_params = device_get_match_data(dev);
> +	if (!adc->hw_params)
> +		return -EINVAL;
> +
> +	adc->last_val = devm_kcalloc(dev, adc->hw_params->num_channels,
> +				     sizeof(*adc->last_val), GFP_KERNEL);
> +	if (!adc->last_val)
> +		return -ENOMEM;
> +
>  	ret = rzg2l_adc_parse_properties(pdev, adc);
>  	if (ret)
>  		return ret;
> @@ -449,8 +476,17 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  	return devm_iio_device_register(dev, indio_dev);
>  }

Re: [PATCH 08/14] iio: adc: rzg2l_adc: Prepare for the addition of RZ/G3S support

[Geert Uytterhoeven]

Hi Jonathan,

On Tue, Dec 3, 2024 at 9:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue,  3 Dec 2024 13:13:08 +0200
> Claudiu <claudiu.beznea@tuxon.dev> wrote:
> > The ADC IP available on the RZ/G3S differs slightly from the one found on
> > the RZ/G2L. The identified differences are as follows:
> > - different number of channels (one being used for temperature conversion);
> >   consequently, various registers differ
> > - different default sampling periods
> > - the RZ/G3S variant lacks the ADVIC register.
> >
> > To accommodate these differences, the rzg2l_adc driver has been updated by
> > introducing the struct rzg2l_adc_hw_params, which encapsulates the
> > hardware-specific differences between the IP variants. A pointer to an
> > object of type struct rzg2l_adc_hw_params is embedded in
> > struct rzg2l_adc_data.
> >
> > Additionally, the completion member of struct rzg2l_adc_data was relocated
> > to avoid potential padding, if any.
> >
> > The code has been adjusted to utilize hardware-specific parameters stored
> > in the new structure instead of relying on plain macros.
> >
> > The check of chan->channel in rzg2l_adc_read_raw() function, against the
> > driver specific mask was removed as the subsystem should have already
> > been done this before reaching the rzg2l_adc_read_raw() function.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> >  drivers/iio/adc/rzg2l_adc.c | 92 ++++++++++++++++++++++++++-----------
> >  1 file changed, 64 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > index fda8b42ded81..aff41152ebf8 100644
> > --- a/drivers/iio/adc/rzg2l_adc.c
> > +++ b/drivers/iio/adc/rzg2l_adc.c
> > @@ -32,20 +32,15 @@
> >  #define RZG2L_ADM1_MS                        BIT(2)
> >  #define RZG2L_ADM1_BS                        BIT(4)
> >  #define RZG2L_ADM1_EGA_MASK          GENMASK(13, 12)
> > -#define RZG2L_ADM2_CHSEL_MASK                GENMASK(7, 0)
> >  #define RZG2L_ADM3_ADIL_MASK         GENMASK(31, 24)
> >  #define RZG2L_ADM3_ADCMP_MASK                GENMASK(23, 16)
> > -#define RZG2L_ADM3_ADCMP_E           FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe)
> > -#define RZG2L_ADM3_ADSMP_MASK                GENMASK(15, 0)
> >
> >  #define RZG2L_ADINT                  0x20
> > -#define RZG2L_ADINT_INTEN_MASK               GENMASK(7, 0)
> >  #define RZG2L_ADINT_CSEEN            BIT(16)
> >  #define RZG2L_ADINT_INTS             BIT(31)
> >
> >  #define RZG2L_ADSTS                  0x24
> >  #define RZG2L_ADSTS_CSEST            BIT(16)
> > -#define RZG2L_ADSTS_INTST_MASK               GENMASK(7, 0)
> >
> >  #define RZG2L_ADIVC                  0x28
> >  #define RZG2L_ADIVC_DIVADC_MASK              GENMASK(8, 0)
> > @@ -56,12 +51,26 @@
> >  #define RZG2L_ADCR(n)                        (0x30 + ((n) * 0x4))
> >  #define RZG2L_ADCR_AD_MASK           GENMASK(11, 0)
> >
> > -#define RZG2L_ADSMP_DEFAULT_SAMPLING 0x578
> > -
> > -#define RZG2L_ADC_MAX_CHANNELS               8
> > -#define RZG2L_ADC_CHN_MASK           0x7
> >  #define RZG2L_ADC_TIMEOUT            usecs_to_jiffies(1 * 4)
> >
> > +/**
> > + * struct rzg2l_adc_hw_params - ADC hardware specific parameters
> > + * @default_adsmp: default ADC sampling period (see ADM3 register)
> > + * @adsmp_mask: ADC sampling period mask (see ADM3 register)
> > + * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
> > + * @default_adcmp: default ADC cmp (see ADM3 register)
> > + * @num_channels: number of supported channels
> > + * @adivc: specifies if ADVIC register is available
> > + */
> > +struct rzg2l_adc_hw_params {
> > +     u16 default_adsmp;
> > +     u16 adsmp_mask;
> > +     u16 adint_inten_mask;
> > +     u8 default_adcmp;
> > +     u8 num_channels;
> > +     bool adivc;
> > +};
> > +
> >  struct rzg2l_adc_data {
> >       const struct iio_chan_spec *channels;
> >       u8 num_channels;
> > @@ -71,10 +80,11 @@ struct rzg2l_adc {
> >       void __iomem *base;
> >       struct reset_control *presetn;
> >       struct reset_control *adrstn;
> > -     struct completion completion;
> >       const struct rzg2l_adc_data *data;
> > +     const struct rzg2l_adc_hw_params *hw_params;
> > +     u16 *last_val;
> > +     struct completion completion;
> >       struct mutex lock;
> > -     u16 last_val[RZG2L_ADC_MAX_CHANNELS];
>
> Just make this big enough for the max device.  Chances are it will make little or
> no difference to this allocation and nice to avoid the dynamic part.
>
> Feel free to add a runtime check to make sure this is big enough to avoid any
> future problems with forgetting to update it.

Flexible array member and the new __counted_by() attribute?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 08/14] iio: adc: rzg2l_adc: Prepare for the addition of RZ/G3S support

[Jonathan Cameron]

On Wed, 4 Dec 2024 10:40:58 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Jonathan,
> 
> On Tue, Dec 3, 2024 at 9:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Tue,  3 Dec 2024 13:13:08 +0200
> > Claudiu <claudiu.beznea@tuxon.dev> wrote:  
> > > The ADC IP available on the RZ/G3S differs slightly from the one found on
> > > the RZ/G2L. The identified differences are as follows:
> > > - different number of channels (one being used for temperature conversion);
> > >   consequently, various registers differ
> > > - different default sampling periods
> > > - the RZ/G3S variant lacks the ADVIC register.
> > >
> > > To accommodate these differences, the rzg2l_adc driver has been updated by
> > > introducing the struct rzg2l_adc_hw_params, which encapsulates the
> > > hardware-specific differences between the IP variants. A pointer to an
> > > object of type struct rzg2l_adc_hw_params is embedded in
> > > struct rzg2l_adc_data.
> > >
> > > Additionally, the completion member of struct rzg2l_adc_data was relocated
> > > to avoid potential padding, if any.
> > >
> > > The code has been adjusted to utilize hardware-specific parameters stored
> > > in the new structure instead of relying on plain macros.
> > >
> > > The check of chan->channel in rzg2l_adc_read_raw() function, against the
> > > driver specific mask was removed as the subsystem should have already
> > > been done this before reaching the rzg2l_adc_read_raw() function.
> > >
> > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > > ---
> > >  drivers/iio/adc/rzg2l_adc.c | 92 ++++++++++++++++++++++++++-----------
> > >  1 file changed, 64 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > > index fda8b42ded81..aff41152ebf8 100644
> > > --- a/drivers/iio/adc/rzg2l_adc.c
> > > +++ b/drivers/iio/adc/rzg2l_adc.c
> > > @@ -32,20 +32,15 @@
> > >  #define RZG2L_ADM1_MS                        BIT(2)
> > >  #define RZG2L_ADM1_BS                        BIT(4)
> > >  #define RZG2L_ADM1_EGA_MASK          GENMASK(13, 12)
> > > -#define RZG2L_ADM2_CHSEL_MASK                GENMASK(7, 0)
> > >  #define RZG2L_ADM3_ADIL_MASK         GENMASK(31, 24)
> > >  #define RZG2L_ADM3_ADCMP_MASK                GENMASK(23, 16)
> > > -#define RZG2L_ADM3_ADCMP_E           FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, 0xe)
> > > -#define RZG2L_ADM3_ADSMP_MASK                GENMASK(15, 0)
> > >
> > >  #define RZG2L_ADINT                  0x20
> > > -#define RZG2L_ADINT_INTEN_MASK               GENMASK(7, 0)
> > >  #define RZG2L_ADINT_CSEEN            BIT(16)
> > >  #define RZG2L_ADINT_INTS             BIT(31)
> > >
> > >  #define RZG2L_ADSTS                  0x24
> > >  #define RZG2L_ADSTS_CSEST            BIT(16)
> > > -#define RZG2L_ADSTS_INTST_MASK               GENMASK(7, 0)
> > >
> > >  #define RZG2L_ADIVC                  0x28
> > >  #define RZG2L_ADIVC_DIVADC_MASK              GENMASK(8, 0)
> > > @@ -56,12 +51,26 @@
> > >  #define RZG2L_ADCR(n)                        (0x30 + ((n) * 0x4))
> > >  #define RZG2L_ADCR_AD_MASK           GENMASK(11, 0)
> > >
> > > -#define RZG2L_ADSMP_DEFAULT_SAMPLING 0x578
> > > -
> > > -#define RZG2L_ADC_MAX_CHANNELS               8
> > > -#define RZG2L_ADC_CHN_MASK           0x7
> > >  #define RZG2L_ADC_TIMEOUT            usecs_to_jiffies(1 * 4)
> > >
> > > +/**
> > > + * struct rzg2l_adc_hw_params - ADC hardware specific parameters
> > > + * @default_adsmp: default ADC sampling period (see ADM3 register)
> > > + * @adsmp_mask: ADC sampling period mask (see ADM3 register)
> > > + * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
> > > + * @default_adcmp: default ADC cmp (see ADM3 register)
> > > + * @num_channels: number of supported channels
> > > + * @adivc: specifies if ADVIC register is available
> > > + */
> > > +struct rzg2l_adc_hw_params {
> > > +     u16 default_adsmp;
> > > +     u16 adsmp_mask;
> > > +     u16 adint_inten_mask;
> > > +     u8 default_adcmp;
> > > +     u8 num_channels;
> > > +     bool adivc;
> > > +};
> > > +
> > >  struct rzg2l_adc_data {
> > >       const struct iio_chan_spec *channels;
> > >       u8 num_channels;
> > > @@ -71,10 +80,11 @@ struct rzg2l_adc {
> > >       void __iomem *base;
> > >       struct reset_control *presetn;
> > >       struct reset_control *adrstn;
> > > -     struct completion completion;
> > >       const struct rzg2l_adc_data *data;
> > > +     const struct rzg2l_adc_hw_params *hw_params;
> > > +     u16 *last_val;
> > > +     struct completion completion;
> > >       struct mutex lock;
> > > -     u16 last_val[RZG2L_ADC_MAX_CHANNELS];  
> >
> > Just make this big enough for the max device.  Chances are it will make little or
> > no difference to this allocation and nice to avoid the dynamic part.
> >
> > Feel free to add a runtime check to make sure this is big enough to avoid any
> > future problems with forgetting to update it.  
> 
> Flexible array member and the new __counted_by() attribute?
Messy as it's embedded in iio_dev via a rather round about route.
It happens to be at the end of that structure but that's an implementation detail.

So in this particular case I'd go with no for flexible array and __counted_by.
It is very unlikely the difference in size will actually result in a bigger allocation
as it's a substantial allocation however big this is.

Jonathan

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

[PATCH 09/14] iio: adc: rzg2l_adc: Add support for channel 8

[Claudiu]

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

The ADC on the Renesas RZ/G3S SoC includes an additional channel (channel
8) dedicated to reading temperature values from the Thermal Sensor Unit
(TSU). There is a direct in-SoC connection between the ADC and TSU IPs.

To read the temperature reported by the TSU, a different sampling rate
(compared to channels 0-7) must be configured in the ADM3 register.

The rzg2l_adc driver has been updated to support reading the TSU
temperature.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 81 +++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index aff41152ebf8..f938b0f9a795 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -55,7 +55,8 @@
 
 /**
  * struct rzg2l_adc_hw_params - ADC hardware specific parameters
- * @default_adsmp: default ADC sampling period (see ADM3 register)
+ * @default_adsmp: default ADC sampling period (see ADM3 register); index 0 is
+ * used for voltage channels, index 1 is used for temperature channel
  * @adsmp_mask: ADC sampling period mask (see ADM3 register)
  * @adint_inten_mask: conversion end interrupt mask (see ADINT register)
  * @default_adcmp: default ADC cmp (see ADM3 register)
@@ -63,7 +64,7 @@
  * @adivc: specifies if ADVIC register is available
  */
 struct rzg2l_adc_hw_params {
-	u16 default_adsmp;
+	u16 default_adsmp[2];
 	u16 adsmp_mask;
 	u16 adint_inten_mask;
 	u8 default_adcmp;
@@ -87,15 +88,26 @@ struct rzg2l_adc {
 	struct mutex lock;
 };
 
-static const char * const rzg2l_adc_channel_name[] = {
-	"adc0",
-	"adc1",
-	"adc2",
-	"adc3",
-	"adc4",
-	"adc5",
-	"adc6",
-	"adc7",
+/**
+ * struct rzg2l_adc_channel - ADC channel descriptor
+ * @name: ADC channel name
+ * @type: ADC channel type
+ */
+struct rzg2l_adc_channel {
+	const char * const name;
+	enum iio_chan_type type;
+};
+
+static const struct rzg2l_adc_channel rzg2l_adc_channels[] = {
+	{ "adc0", IIO_VOLTAGE },
+	{ "adc1", IIO_VOLTAGE },
+	{ "adc2", IIO_VOLTAGE },
+	{ "adc3", IIO_VOLTAGE },
+	{ "adc4", IIO_VOLTAGE },
+	{ "adc5", IIO_VOLTAGE },
+	{ "adc6", IIO_VOLTAGE },
+	{ "adc7", IIO_VOLTAGE },
+	{ "adc8", IIO_TEMP },
 };
 
 static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
@@ -161,7 +173,7 @@ static void rzg2l_set_trigger(struct rzg2l_adc *adc)
 	rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
 }
 
-static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
+static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch, enum iio_chan_type type)
 {
 	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	u32 reg;
@@ -177,6 +189,15 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 	reg |= BIT(ch);
 	rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
 
+	reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
+	reg &= ~hw_params->adsmp_mask;
+	/*
+	 * type could be IIO_VOLTAGE = 0 or IIO_TEMP = 9. Divide to 8 to get
+	 * index 0 or 1 depending on the channel type.
+	 */
+	reg |= hw_params->default_adsmp[type / 8];
+	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
+
 	/*
 	 * Setup ADINT
 	 * INTS[31] - Select pulse signal
@@ -192,7 +213,8 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
 	return 0;
 }
 
-static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc, u8 ch)
+static int rzg2l_adc_conversion(struct iio_dev *indio_dev, enum iio_chan_type type,
+				struct rzg2l_adc *adc, u8 ch)
 {
 	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	struct device *dev = indio_dev->dev.parent;
@@ -202,7 +224,7 @@ static int rzg2l_adc_conversion(struct iio_dev *indio_dev, struct rzg2l_adc *adc
 	if (ret)
 		return ret;
 
-	ret = rzg2l_adc_conversion_setup(adc, ch);
+	ret = rzg2l_adc_conversion_setup(adc, ch, type);
 	if (ret)
 		goto rpm_put;
 
@@ -238,13 +260,27 @@ static int rzg2l_adc_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		mutex_lock(&adc->lock);
-		ret = rzg2l_adc_conversion(indio_dev, adc, ch);
+		ret = rzg2l_adc_conversion(indio_dev, chan->type, adc, ch);
 		if (!ret)
 			*val = adc->last_val[ch];
 		mutex_unlock(&adc->lock);
 
 		return ret ? ret : IIO_VAL_INT;
 
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type != IIO_TEMP)
+			return -EINVAL;
+
+		mutex_lock(&adc->lock);
+		ret = rzg2l_adc_conversion(indio_dev, chan->type, adc, ch);
+		if (!ret) {
+			/* Convert it to mili Celsius. */
+			*val = adc->last_val[ch] * 1000;
+		}
+		mutex_unlock(&adc->lock);
+
+		return ret ? ret : IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -254,7 +290,7 @@ static int rzg2l_adc_read_label(struct iio_dev *iio_dev,
 				const struct iio_chan_spec *chan,
 				char *label)
 {
-	return sysfs_emit(label, "%s\n", rzg2l_adc_channel_name[chan->channel]);
+	return sysfs_emit(label, "%s\n", rzg2l_adc_channels[chan->channel].name);
 }
 
 static const struct iio_info rzg2l_adc_iio_info = {
@@ -332,11 +368,14 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 		if (channel >= hw_params->num_channels)
 			return -EINVAL;
 
-		chan_array[i].type = IIO_VOLTAGE;
+		chan_array[i].type = rzg2l_adc_channels[channel].type;
 		chan_array[i].indexed = 1;
 		chan_array[i].channel = channel;
-		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
-		chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
+		if (rzg2l_adc_channels[channel].type == IIO_VOLTAGE)
+			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+		else
+			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED);
+		chan_array[i].datasheet_name = rzg2l_adc_channels[channel].name;
 		i++;
 	}
 
@@ -386,7 +425,7 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
 	reg &= ~RZG2L_ADM3_ADCMP_MASK;
 	reg &= ~hw_params->adsmp_mask;
 	reg |= FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, hw_params->default_adcmp) |
-	       hw_params->default_adsmp;
+	       hw_params->default_adsmp[0];
 
 	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
 
@@ -479,7 +518,7 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
 static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
 	.num_channels = 8,
 	.default_adcmp = 0xe,
-	.default_adsmp = 0x578,
+	.default_adsmp = { 0x578 },
 	.adsmp_mask = GENMASK(15, 0),
 	.adint_inten_mask = GENMASK(7, 0),
 	.adivc = true
-- 
2.39.2

Re: [PATCH 09/14] iio: adc: rzg2l_adc: Add support for channel 8

[Jonathan Cameron]

On Tue,  3 Dec 2024 13:13:09 +0200
Claudiu <claudiu.beznea@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The ADC on the Renesas RZ/G3S SoC includes an additional channel (channel
> 8) dedicated to reading temperature values from the Thermal Sensor Unit
> (TSU). There is a direct in-SoC connection between the ADC and TSU IPs.
> 
> To read the temperature reported by the TSU, a different sampling rate
> (compared to channels 0-7) must be configured in the ADM3 register.
> 
> The rzg2l_adc driver has been updated to support reading the TSU
> temperature.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

>  static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
> @@ -161,7 +173,7 @@ static void rzg2l_set_trigger(struct rzg2l_adc *adc)
>  	rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
>  }
>  
> -static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch, enum iio_chan_type type)
>  {
>  	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
>  	u32 reg;
> @@ -177,6 +189,15 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>  	reg |= BIT(ch);
>  	rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
>  
> +	reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
> +	reg &= ~hw_params->adsmp_mask;
> +	/*
> +	 * type could be IIO_VOLTAGE = 0 or IIO_TEMP = 9. Divide to 8 to get
> +	 * index 0 or 1 depending on the channel type.

That is not particularly nice and potentially a little fragile if we get other device
support in future. Better to match on the type in rzg2l_adc_channels[] possibly wrapped
up in a little utility function bool rzg2l_adc_channels_is_temp(); Then use a
? 1 : 0 to get the offset in default_adsmp[]


> +	 */
> +	reg |= hw_params->default_adsmp[type / 8];
> +	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
> +
>  	/*
>  	 * Setup ADINT
>  	 * INTS[31] - Select pulse signal
> @@ -192,7 +213,8 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>  	return 0;
>  }
>
>  
> +	case IIO_CHAN_INFO_PROCESSED:
> +		if (chan->type != IIO_TEMP)
> +			return -EINVAL;
> +
> +		mutex_lock(&adc->lock);
> +		ret = rzg2l_adc_conversion(indio_dev, chan->type, adc, ch);
> +		if (!ret) {
> +			/* Convert it to mili Celsius. */
> +			*val = adc->last_val[ch] * 1000;
Prefer you provide a scale of 1000 and report this raw.
> +		}
Also strong preference for error conditions out of line.
As in that other case, guard() makes that easier as yo ucan do
		{

			guard(mutex)(&adc->lock);
			ret = rz....
			if (ret)
				return ret;

			*val = ...
			
			return IIO_VAL_INT;
		}

> +		mutex_unlock(&adc->lock);
> +
> +		return ret ? ret : IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}

>  static const struct iio_info rzg2l_adc_iio_info = {
> @@ -332,11 +368,14 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
>  		if (channel >= hw_params->num_channels)
>  			return -EINVAL;
>  
> -		chan_array[i].type = IIO_VOLTAGE;
> +		chan_array[i].type = rzg2l_adc_channels[channel].type;
>  		chan_array[i].indexed = 1;
>  		chan_array[i].channel = channel;
> -		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> -		chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
> +		if (rzg2l_adc_channels[channel].type == IIO_VOLTAGE)
> +			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +		else
> +			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED);

Make it raw, but I'm curious we have no _SCALE on this device.  Do we really have no idea
of the calibration of these channels?

> +		chan_array[i].datasheet_name = rzg2l_adc_channels[channel].name;
>  		i++;
>  	}
>  
> @@ -386,7 +425,7 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
>  	reg &= ~RZG2L_ADM3_ADCMP_MASK;
>  	reg &= ~hw_params->adsmp_mask;
>  	reg |= FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, hw_params->default_adcmp) |
> -	       hw_params->default_adsmp;
> +	       hw_params->default_adsmp[0];
>  
>  	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
>  
> @@ -479,7 +518,7 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
>  	.num_channels = 8,
>  	.default_adcmp = 0xe,
> -	.default_adsmp = 0x578,
> +	.default_adsmp = { 0x578 },
>  	.adsmp_mask = GENMASK(15, 0),
>  	.adint_inten_mask = GENMASK(7, 0),
>  	.adivc = true

Re: [PATCH 09/14] iio: adc: rzg2l_adc: Add support for channel 8

[Claudiu Beznea]

Hi, Jonathan,

On 03.12.2024 22:18, Jonathan Cameron wrote:
> On Tue,  3 Dec 2024 13:13:09 +0200
> Claudiu <claudiu.beznea@tuxon.dev> wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The ADC on the Renesas RZ/G3S SoC includes an additional channel (channel
>> 8) dedicated to reading temperature values from the Thermal Sensor Unit
>> (TSU). There is a direct in-SoC connection between the ADC and TSU IPs.
>>
>> To read the temperature reported by the TSU, a different sampling rate
>> (compared to channels 0-7) must be configured in the ADM3 register.
>>
>> The rzg2l_adc driver has been updated to support reading the TSU
>> temperature.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>>  static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
>> @@ -161,7 +173,7 @@ static void rzg2l_set_trigger(struct rzg2l_adc *adc)
>>  	rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
>>  }
>>  
>> -static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>> +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch, enum iio_chan_type type)
>>  {
>>  	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
>>  	u32 reg;
>> @@ -177,6 +189,15 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>>  	reg |= BIT(ch);
>>  	rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
>>  
>> +	reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
>> +	reg &= ~hw_params->adsmp_mask;
>> +	/*
>> +	 * type could be IIO_VOLTAGE = 0 or IIO_TEMP = 9. Divide to 8 to get
>> +	 * index 0 or 1 depending on the channel type.
> 
> That is not particularly nice and potentially a little fragile if we get other device
> support in future. Better to match on the type in rzg2l_adc_channels[] possibly wrapped
> up in a little utility function bool rzg2l_adc_channels_is_temp(); Then use a
> ? 1 : 0 to get the offset in default_adsmp[]

I though about this, too, but considered we should not go beyond 1 as the
rzg2l_adc_conversion_setup() is currently call in places where the channel
type is only IIO_VOLTAGE and IIO_TEMP, just saying...

I'll switch to the approach you propose in the next version.

> 
> 
>> +	 */
>> +	reg |= hw_params->default_adsmp[type / 8];
>> +	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
>> +
>>  	/*
>>  	 * Setup ADINT
>>  	 * INTS[31] - Select pulse signal
>> @@ -192,7 +213,8 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
>>  	return 0;
>>  }
>>
>>  
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		if (chan->type != IIO_TEMP)
>> +			return -EINVAL;
>> +
>> +		mutex_lock(&adc->lock);
>> +		ret = rzg2l_adc_conversion(indio_dev, chan->type, adc, ch);
>> +		if (!ret) {
>> +			/* Convert it to mili Celsius. */
>> +			*val = adc->last_val[ch] * 1000;
> Prefer you provide a scale of 1000 and report this raw.

I'll look to this.

>> +		}
> Also strong preference for error conditions out of line.
> As in that other case, guard() makes that easier as yo ucan do
> 		{
> 
> 			guard(mutex)(&adc->lock);
> 			ret = rz....
> 			if (ret)
> 				return ret;
> 
> 			*val = ...
> 			
> 			return IIO_VAL_INT;
> 		}


I agree, looks cleaner with guard().

> 
>> +		mutex_unlock(&adc->lock);
>> +
>> +		return ret ? ret : IIO_VAL_INT;
>> +
>>  	default:
>>  		return -EINVAL;
>>  	}
> 
>>  static const struct iio_info rzg2l_adc_iio_info = {
>> @@ -332,11 +368,14 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
>>  		if (channel >= hw_params->num_channels)
>>  			return -EINVAL;
>>  
>> -		chan_array[i].type = IIO_VOLTAGE;
>> +		chan_array[i].type = rzg2l_adc_channels[channel].type;
>>  		chan_array[i].indexed = 1;
>>  		chan_array[i].channel = channel;
>> -		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> -		chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
>> +		if (rzg2l_adc_channels[channel].type == IIO_VOLTAGE)
>> +			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +		else
>> +			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED);
> 
> Make it raw, but I'm curious we have no _SCALE on this device.  Do we really have no idea
> of the calibration of these channels?

The calibration data is stored on the TSU (Thermal Sensor Unit) hardware
block. From the TSU driver (not yet published) we read the temperature
through the ADC channel and apply the calibration data on the value
returned by ADC (this is how HW manual suggests). This is the current
(internal) struct thermal_zone_device_ops::get_temp() API of the TSU driver
(it will be published later, after some clarifications with hardware team
on SoC specific aspects):

// ..

#define TSU_READ_STEPS		8
#define MCELSIUS(temp)		(temp * MILLIDEGREE_PER_DEGREE)

// ...

struct rzg3s_thermal_priv {
	void __iomem *base;
	struct device *dev;
	struct thermal_zone_device *tz;
	struct reset_control *rstc;
	struct iio_channel *channel;
	u16 calib0;
	u16 calib1;
};

// ...

static int rzg3s_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
{
	struct rzg3s_thermal_priv *priv = thermal_zone_device_priv(tz);
	struct device *dev = priv->dev;
	u32 ts_code_ave = 0;
	int ret, val;

	ret = pm_runtime_resume_and_get(dev);
	if (ret)
		return ret;

	for (u8 i = 0; i < TSU_READ_STEPS; i++) {
		ret = iio_read_channel_processed(priv->channel, &val);
		if (ret < 0)
			goto rpm_put;
		
		ts_code_ave += val;
		/*
		 * According to HW manual (section 40.4.4 Procedure for
		 * Measuring the Temperature) we need to wait here at
		 * leat 3us.
		 */
		usleep_range(5, 10);
	}

	ret = 0;
	ts_code_ave = DIV_ROUND_CLOSEST(ts_code_ave, TSU_READ_STEPS);

	/*
	 * According to HW manual (section 40.4.4 Procedure for Measuring
	 * the Temperature) the formula mentioned in the hardware manual is
	 * as follows;
	 *
	 * Tj = (ts_code_ave - priv->calib0) * (165 / (priv->calib0 -
	 * priv->calib1)) - 40
	 *
	 * As the temperature retrieved from the IIO channel is in
	 * milicelsius convert all members of the equation to milicelsius.
	 * It has been chose like this to avoid loosing precisions by
	 * converting IIO reported temperature to celsius.
	 */
	*temp = DIV_ROUND_CLOSEST_ULL(((u64)(ts_code_ave -
		MCELSIUS(priv->calib1)) * MCELSIUS(165)),
		MCELSIUS((priv->calib0 - priv->calib1))) - MCELSIUS(40);

	/* Round it up to 0.5 degrees Celsius. */
	*temp = roundup(*temp, 500);

rpm_put:
	pm_runtime_mark_last_busy(dev);
	pm_runtime_put_autosuspend(dev);

	return ret;
}


> 
>> +		chan_array[i].datasheet_name = rzg2l_adc_channels[channel].name;
>>  		i++;
>>  	}
>>  
>> @@ -386,7 +425,7 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
>>  	reg &= ~RZG2L_ADM3_ADCMP_MASK;
>>  	reg &= ~hw_params->adsmp_mask;
>>  	reg |= FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, hw_params->default_adcmp) |
>> -	       hw_params->default_adsmp;
>> +	       hw_params->default_adsmp[0];
>>  
>>  	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
>>  
>> @@ -479,7 +518,7 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
>>  	.num_channels = 8,
>>  	.default_adcmp = 0xe,
>> -	.default_adsmp = 0x578,
>> +	.default_adsmp = { 0x578 },
>>  	.adsmp_mask = GENMASK(15, 0),
>>  	.adint_inten_mask = GENMASK(7, 0),
>>  	.adivc = true
> 

Re: [PATCH 09/14] iio: adc: rzg2l_adc: Add support for channel 8

[Jonathan Cameron]

On Wed, 4 Dec 2024 10:50:03 +0200
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> Hi, Jonathan,
> 
> On 03.12.2024 22:18, Jonathan Cameron wrote:
> > On Tue,  3 Dec 2024 13:13:09 +0200
> > Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >   
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The ADC on the Renesas RZ/G3S SoC includes an additional channel (channel
> >> 8) dedicated to reading temperature values from the Thermal Sensor Unit
> >> (TSU). There is a direct in-SoC connection between the ADC and TSU IPs.
> >>
> >> To read the temperature reported by the TSU, a different sampling rate
> >> (compared to channels 0-7) must be configured in the ADM3 register.
> >>
> >> The rzg2l_adc driver has been updated to support reading the TSU
> >> temperature.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>  
> >   
> >>  static unsigned int rzg2l_adc_readl(struct rzg2l_adc *adc, u32 reg)
> >> @@ -161,7 +173,7 @@ static void rzg2l_set_trigger(struct rzg2l_adc *adc)
> >>  	rzg2l_adc_writel(adc, RZG2L_ADM(1), reg);
> >>  }
> >>  
> >> -static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> >> +static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch, enum iio_chan_type type)
> >>  {
> >>  	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
> >>  	u32 reg;
> >> @@ -177,6 +189,15 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> >>  	reg |= BIT(ch);
> >>  	rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
> >>  
> >> +	reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
> >> +	reg &= ~hw_params->adsmp_mask;
> >> +	/*
> >> +	 * type could be IIO_VOLTAGE = 0 or IIO_TEMP = 9. Divide to 8 to get
> >> +	 * index 0 or 1 depending on the channel type.  
> > 
> > That is not particularly nice and potentially a little fragile if we get other device
> > support in future. Better to match on the type in rzg2l_adc_channels[] possibly wrapped
> > up in a little utility function bool rzg2l_adc_channels_is_temp(); Then use a
> > ? 1 : 0 to get the offset in default_adsmp[]  
> 
> I though about this, too, but considered we should not go beyond 1 as the
> rzg2l_adc_conversion_setup() is currently call in places where the channel
> type is only IIO_VOLTAGE and IIO_TEMP, just saying...
> 
> I'll switch to the approach you propose in the next version.
> 
> > 
> >   
> >> +	 */
> >> +	reg |= hw_params->default_adsmp[type / 8];
> >> +	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
> >> +
> >>  	/*
> >>  	 * Setup ADINT
> >>  	 * INTS[31] - Select pulse signal
> >> @@ -192,7 +213,8 @@ static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
> >>  	return 0;
> >>  }
> >>
> >>  
> >> +	case IIO_CHAN_INFO_PROCESSED:
> >> +		if (chan->type != IIO_TEMP)
> >> +			return -EINVAL;
> >> +
> >> +		mutex_lock(&adc->lock);
> >> +		ret = rzg2l_adc_conversion(indio_dev, chan->type, adc, ch);
> >> +		if (!ret) {
> >> +			/* Convert it to mili Celsius. */
> >> +			*val = adc->last_val[ch] * 1000;  
> > Prefer you provide a scale of 1000 and report this raw.  
> 
> I'll look to this.
> 
> >> +		}  
> > Also strong preference for error conditions out of line.
> > As in that other case, guard() makes that easier as yo ucan do
> > 		{
> > 
> > 			guard(mutex)(&adc->lock);
> > 			ret = rz....
> > 			if (ret)
> > 				return ret;
> > 
> > 			*val = ...
> > 			
> > 			return IIO_VAL_INT;
> > 		}  
> 
> 
> I agree, looks cleaner with guard().
> 
> >   
> >> +		mutex_unlock(&adc->lock);
> >> +
> >> +		return ret ? ret : IIO_VAL_INT;
> >> +
> >>  	default:
> >>  		return -EINVAL;
> >>  	}  
> >   
> >>  static const struct iio_info rzg2l_adc_iio_info = {
> >> @@ -332,11 +368,14 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
> >>  		if (channel >= hw_params->num_channels)
> >>  			return -EINVAL;
> >>  
> >> -		chan_array[i].type = IIO_VOLTAGE;
> >> +		chan_array[i].type = rzg2l_adc_channels[channel].type;
> >>  		chan_array[i].indexed = 1;
> >>  		chan_array[i].channel = channel;
> >> -		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> >> -		chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
> >> +		if (rzg2l_adc_channels[channel].type == IIO_VOLTAGE)
> >> +			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> >> +		else
> >> +			chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED);  
> > 
> > Make it raw, but I'm curious we have no _SCALE on this device.  Do we really have no idea
> > of the calibration of these channels?  
> 
> The calibration data is stored on the TSU (Thermal Sensor Unit) hardware
> block. From the TSU driver (not yet published) we read the temperature
> through the ADC channel and apply the calibration data on the value
> returned by ADC (this is how HW manual suggests). This is the current
> (internal) struct thermal_zone_device_ops::get_temp() API of the TSU driver
> (it will be published later, after some clarifications with hardware team
> on SoC specific aspects):
> 
> // ..
> 
> #define TSU_READ_STEPS		8
> #define MCELSIUS(temp)		(temp * MILLIDEGREE_PER_DEGREE)
> 
> // ...
> 
> struct rzg3s_thermal_priv {
> 	void __iomem *base;
> 	struct device *dev;
> 	struct thermal_zone_device *tz;
> 	struct reset_control *rstc;
> 	struct iio_channel *channel;
> 	u16 calib0;
> 	u16 calib1;
> };
> 
> // ...
> 
> static int rzg3s_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> {
> 	struct rzg3s_thermal_priv *priv = thermal_zone_device_priv(tz);
> 	struct device *dev = priv->dev;
> 	u32 ts_code_ave = 0;
> 	int ret, val;
> 
> 	ret = pm_runtime_resume_and_get(dev);
> 	if (ret)
> 		return ret;
> 
> 	for (u8 i = 0; i < TSU_READ_STEPS; i++) {
> 		ret = iio_read_channel_processed(priv->channel, &val);

I'd switch this to iio_read_channel_raw() given the scale is meaningless
without the calibration done in here and that processed channel is
exposed to userspace as well.  If it were to use raw and scale and
then IIRC iio_read_channel_raw() does the maths for you anyway giving
same result here.

Beyond that, this seems reasonable except (see below)

> 		if (ret < 0)
> 			goto rpm_put;
> 		
> 		ts_code_ave += val;
> 		/*
> 		 * According to HW manual (section 40.4.4 Procedure for
> 		 * Measuring the Temperature) we need to wait here at
> 		 * leat 3us.
> 		 */
> 		usleep_range(5, 10);
> 	}
> 
> 	ret = 0;
> 	ts_code_ave = DIV_ROUND_CLOSEST(ts_code_ave, TSU_READ_STEPS);
> 
> 	/*
> 	 * According to HW manual (section 40.4.4 Procedure for Measuring
> 	 * the Temperature) the formula mentioned in the hardware manual is
> 	 * as follows;
> 	 *
> 	 * Tj = (ts_code_ave - priv->calib0) * (165 / (priv->calib0 -
> 	 * priv->calib1)) - 40
> 	 *
> 	 * As the temperature retrieved from the IIO channel is in
> 	 * milicelsius convert all members of the equation to milicelsius.
> 	 * It has been chose like this to avoid loosing precisions by
> 	 * converting IIO reported temperature to celsius.

I'm confused. How does it have a unit if you are only applying a calibration
here?  Or is this a precision tweak on a roughly right fixed calibration?

Jonathan


> 	 */
> 	*temp = DIV_ROUND_CLOSEST_ULL(((u64)(ts_code_ave -
> 		MCELSIUS(priv->calib1)) * MCELSIUS(165)),
> 		MCELSIUS((priv->calib0 - priv->calib1))) - MCELSIUS(40);
> 
> 	/* Round it up to 0.5 degrees Celsius. */
> 	*temp = roundup(*temp, 500);
> 
> rpm_put:
> 	pm_runtime_mark_last_busy(dev);
> 	pm_runtime_put_autosuspend(dev);
> 
> 	return ret;
> }
> 
> 
> >   
> >> +		chan_array[i].datasheet_name = rzg2l_adc_channels[channel].name;
> >>  		i++;
> >>  	}
> >>  
> >> @@ -386,7 +425,7 @@ static int rzg2l_adc_hw_init(struct device *dev, struct rzg2l_adc *adc)
> >>  	reg &= ~RZG2L_ADM3_ADCMP_MASK;
> >>  	reg &= ~hw_params->adsmp_mask;
> >>  	reg |= FIELD_PREP(RZG2L_ADM3_ADCMP_MASK, hw_params->default_adcmp) |
> >> -	       hw_params->default_adsmp;
> >> +	       hw_params->default_adsmp[0];
> >>  
> >>  	rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
> >>  
> >> @@ -479,7 +518,7 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> >>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
> >>  	.num_channels = 8,
> >>  	.default_adcmp = 0xe,
> >> -	.default_adsmp = 0x578,
> >> +	.default_adsmp = { 0x578 },
> >>  	.adsmp_mask = GENMASK(15, 0),
> >>  	.adint_inten_mask = GENMASK(7, 0),
> >>  	.adivc = true  
> >   

[PATCH 10/14] iio: adc: rzg2l_adc: Add suspend/resume support

[Claudiu]

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

The Renesas RZ/G3S SoC features a power-saving mode where power to most of
the SoC components is turned off, including the ADC IP.

Suspend/resume support has been added to the rzg2l_adc driver to restore
functionality after resuming from this power-saving mode. During suspend,
the ADC resets are asserted, and the ADC is powered down. On resume, the
ADC resets are de-asserted, the hardware is re-initialized, and the ADC
power is restored using the runtime PM APIs.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 70 +++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index f938b0f9a795..634073e7241f 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -86,6 +86,7 @@ struct rzg2l_adc {
 	u16 *last_val;
 	struct completion completion;
 	struct mutex lock;
+	bool was_rpm_active;
 };
 
 /**
@@ -550,8 +551,77 @@ static int rzg2l_adc_pm_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int rzg2l_adc_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct rzg2l_adc *adc = iio_priv(indio_dev);
+	struct reset_control_bulk_data resets[] = {
+		{ .rstc = adc->presetn },
+		{ .rstc = adc->adrstn },
+	};
+	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 = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
+	if (ret)
+		goto rpm_restore;
+
+	return 0;
+
+rpm_restore:
+	if (adc->was_rpm_active)
+		pm_runtime_force_resume(dev);
+
+	return ret;
+}
+
+static int rzg2l_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct rzg2l_adc *adc = iio_priv(indio_dev);
+	struct reset_control_bulk_data resets[] = {
+		{ .rstc = adc->adrstn },
+		{ .rstc = adc->presetn },
+	};
+	int ret;
+
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
+	if (ret)
+		return ret;
+
+	if (adc->was_rpm_active) {
+		ret = pm_runtime_force_resume(dev);
+		if (ret)
+			goto resets_restore;
+	}
+
+	ret = rzg2l_adc_hw_init(dev, adc);
+	if (ret)
+		goto rpm_restore;
+
+	return 0;
+
+rpm_restore:
+	if (adc->was_rpm_active) {
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
+resets_restore:
+	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
+	return ret;
+}
+
 static const struct dev_pm_ops rzg2l_adc_pm_ops = {
 	RUNTIME_PM_OPS(rzg2l_adc_pm_runtime_suspend, rzg2l_adc_pm_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(rzg2l_adc_suspend, rzg2l_adc_resume)
 };
 
 static struct platform_driver rzg2l_adc_driver = {
-- 
2.39.2

[PATCH 11/14] dt-bindings: iio: adc: renesas,rzg2l-adc: Document RZ/G3S SoC

[Claudiu]

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

Document the ADC IP available on the RZ/G3S SoC. The ADC IP on the RZ/G3S
differs slightly from the one found on the RZ/G2L. The identified
differences are as follows:
- different number of channels (one being used for temperature conversion);
  consequently, various registers differ; the temperature channel
  support was not available for the RZ/G2L variant; the #io-channel-cells
  property was added to be able to request the temperature channel from
  the thermal driver
- different default sampling periods
- the RZ/G3S variant lacks the ADVIC register.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 .../bindings/iio/adc/renesas,rzg2l-adc.yaml   | 37 +++++++++++++------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml b/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
index ba86c7b7d622..40341d541726 100644
--- a/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.yaml
@@ -17,12 +17,15 @@ description: |
 
 properties:
   compatible:
-    items:
-      - enum:
-          - renesas,r9a07g043-adc   # RZ/G2UL and RZ/Five
-          - renesas,r9a07g044-adc   # RZ/G2L
-          - renesas,r9a07g054-adc   # RZ/V2L
-      - const: renesas,rzg2l-adc
+    oneOf:
+      - items:
+          - enum:
+              - renesas,r9a07g043-adc   # RZ/G2UL and RZ/Five
+              - renesas,r9a07g044-adc   # RZ/G2L
+              - renesas,r9a07g054-adc   # RZ/V2L
+          - const: renesas,rzg2l-adc
+      - items:
+          - const: renesas,r9a08g045-adc  # RZ/G3S
 
   reg:
     maxItems: 1
@@ -57,6 +60,9 @@ properties:
   '#size-cells':
     const: 0
 
+  "#io-channel-cells":
+    const: 1
+
 required:
   - compatible
   - reg
@@ -68,7 +74,7 @@ required:
   - reset-names
 
 patternProperties:
-  "^channel@[0-7]$":
+  "^channel@[0-8]$":
     $ref: adc.yaml
     type: object
     description: |
@@ -78,6 +84,8 @@ patternProperties:
       reg:
         description: |
           The channel number.
+        minimum: 0
+        maximum: 8
 
     required:
       - reg
@@ -92,18 +100,25 @@ allOf:
             const: renesas,r9a07g043-adc
     then:
       patternProperties:
-        "^channel@[2-7]$": false
+        "^channel@[2-8]$": false
         "^channel@[0-1]$":
           properties:
             reg:
-              minimum: 0
               maximum: 1
-    else:
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - renesas,r9a07g044-adc
+              - renesas,r9a07g054-adc
+    then:
       patternProperties:
+        "^channel@[8]$": false
         "^channel@[0-7]$":
           properties:
             reg:
-              minimum: 0
               maximum: 7
 
 additionalProperties: false
-- 
2.39.2

Re: [PATCH 11/14] dt-bindings: iio: adc: renesas,rzg2l-adc: Document RZ/G3S SoC

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Tue, Dec 03, 2024 at 01:13:11PM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Document the ADC IP available on the RZ/G3S SoC. The ADC IP on the RZ/G3S
> differs slightly from the one found on the RZ/G2L. The identified
> differences are as follows:
> - different number of channels (one being used for temperature conversion);
>   consequently, various registers differ; the temperature channel
>   support was not available for the RZ/G2L variant; the #io-channel-cells
>   property was added to be able to request the temperature channel from
>   the thermal driver
> - different default sampling periods
> - the RZ/G3S variant lacks the ADVIC register.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 12/14] iio: adc: rzg2l_adc: Add support for Renesas RZ/G3S

[Claudiu]

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

Add ADC support for the Renesas RZ/G3S SoC. The key features of this IP
include:
- 9 channels, with one dedicated to reading the temperature reported by the
  Thermal Sensor Unit (TSU)
- A different default ADCMP value, which is written to the ADM3 register.
- Different default sampling rates
- ADM3.ADSMP field is 8 bits wide
- ADINT.INTEN field is 11 bits wide

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/iio/adc/rzg2l_adc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 634073e7241f..dd2ef8203966 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -525,7 +525,16 @@ static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
 	.adivc = true
 };
 
+static const struct rzg2l_adc_hw_params rzg3s_hw_params = {
+	.num_channels = 9,
+	.default_adcmp = 0x1d,
+	.default_adsmp = { 0x7f, 0xff },
+	.adsmp_mask = GENMASK(7, 0),
+	.adint_inten_mask = GENMASK(11, 0),
+};
+
 static const struct of_device_id rzg2l_adc_match[] = {
+	{ .compatible = "renesas,r9a08g045-adc", .data = &rzg3s_hw_params },
 	{ .compatible = "renesas,rzg2l-adc", .data = &rzg2l_hw_params },
 	{ /* sentinel */ }
 };
-- 
2.39.2

Re: [PATCH 12/14] iio: adc: rzg2l_adc: Add support for Renesas RZ/G3S

[Jonathan Cameron]

On Tue,  3 Dec 2024 13:13:12 +0200
Claudiu <claudiu.beznea@tuxon.dev> wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Add ADC support for the Renesas RZ/G3S SoC. The key features of this IP
> include:
> - 9 channels, with one dedicated to reading the temperature reported by the
>   Thermal Sensor Unit (TSU)
> - A different default ADCMP value, which is written to the ADM3 register.
> - Different default sampling rates
> - ADM3.ADSMP field is 8 bits wide
> - ADINT.INTEN field is 11 bits wide
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Hi Claudia,

This one and the others I haven't comment on look good to me.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/rzg2l_adc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 634073e7241f..dd2ef8203966 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -525,7 +525,16 @@ static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
>  	.adivc = true
>  };
>  
> +static const struct rzg2l_adc_hw_params rzg3s_hw_params = {
> +	.num_channels = 9,
> +	.default_adcmp = 0x1d,
> +	.default_adsmp = { 0x7f, 0xff },
> +	.adsmp_mask = GENMASK(7, 0),
> +	.adint_inten_mask = GENMASK(11, 0),
> +};
> +
>  static const struct of_device_id rzg2l_adc_match[] = {
> +	{ .compatible = "renesas,r9a08g045-adc", .data = &rzg3s_hw_params },
>  	{ .compatible = "renesas,rzg2l-adc", .data = &rzg2l_hw_params },
>  	{ /* sentinel */ }
>  };

[PATCH 13/14] arm64: dts: renesas: r9a08g045: Add ADC node

[Claudiu]

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

Add the device tree node for the ADC IP available on the Renesas RZ/G3S
SoC.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
index be8a0a768c65..eb57a52d2086 100644
--- a/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a08g045.dtsi
@@ -87,6 +87,59 @@ rtc: rtc@1004ec00 {
 			status = "disabled";
 		};
 
+		adc: adc@10058000 {
+			compatible = "renesas,r9a08g045-adc";
+			reg = <0 0x10058000 0 0x400>;
+			interrupts = <GIC_SPI 312 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&cpg CPG_MOD R9A08G045_ADC_ADCLK>,
+				 <&cpg CPG_MOD R9A08G045_ADC_PCLK>;
+			clock-names = "adclk", "pclk";
+			resets = <&cpg R9A08G045_ADC_PRESETN>,
+				 <&cpg R9A08G045_ADC_ADRST_N>;
+			reset-names = "presetn", "adrst-n";
+			power-domains = <&cpg>;
+			#io-channel-cells = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			channel@0 {
+				reg = <0>;
+			};
+
+			channel@1 {
+				reg = <1>;
+			};
+
+			channel@2 {
+				reg = <2>;
+			};
+
+			channel@3 {
+				reg = <3>;
+			};
+
+			channel@4 {
+				reg = <4>;
+			};
+
+			channel@5 {
+				reg = <5>;
+			};
+
+			channel@6 {
+				reg = <6>;
+			};
+
+			channel@7 {
+				reg = <7>;
+			};
+
+			channel@8 {
+				reg = <8>;
+			};
+		};
+
 		vbattb: clock-controller@1005c000 {
 			compatible = "renesas,r9a08g045-vbattb";
 			reg = <0 0x1005c000 0 0x1000>;
-- 
2.39.2

[PATCH 14/14] arm64: dts: renesas: rzg3s-smarc-som: Enable ADC

[Claudiu]

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

Enable ADC.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
index 2ed01d391554..57ebdfc858eb 100644
--- a/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg3s-smarc-som.dtsi
@@ -94,6 +94,10 @@ vcc_sdhi2: regulator2 {
 	};
 };
 
+&adc {
+	status = "okay";
+};
+
 #if SW_CONFIG3 == SW_ON
 &eth0 {
 	pinctrl-0 = <&eth0_pins>;
-- 
2.39.2