Message-ID: <1e8d7c96cdfaa93bcc0f581103dc0e13dfee17b7.camel@gmail.com>
Date: Wed, 15 Oct 2025 16:21:09 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: "Herve Codina (Schneider Electric)" <herve.codina@...tlin.com>,  Wolfram
 Sang <wsa+renesas@...g-engineering.com>, Jonathan Cameron
 <jic23@...nel.org>, David Lechner	 <dlechner@...libre.com>, Nuno
 Sá <nuno.sa@...log.com>,  Andy Shevchenko	
 <andy@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski	
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Geert
 Uytterhoeven	 <geert+renesas@...der.be>, Magnus Damm
 <magnus.damm@...il.com>, Liam Girdwood	 <lgirdwood@...il.com>, Mark Brown
 <broonie@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Pascal Eberhard	
 <pascal.eberhard@...com>, Miquel Raynal <miquel.raynal@...tlin.com>, Thomas
 Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

On Wed, 2025-10-15 at 16:28 +0200, Herve Codina (Schneider Electric) wrote:
> The Renesas RZ/N1 ADC controller is the ADC controller available in the
> Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1
> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.
> 
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@...tlin.com>
> ---
>  drivers/iio/adc/Kconfig    |  10 +
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/rzn1-adc.c | 626 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 637 insertions(+)
>  create mode 100644 drivers/iio/adc/rzn1-adc.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58a14e6833f6..113f6a5c9745 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1403,6 +1403,16 @@ config RZG2L_ADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rzg2l_adc.
>  
> +config RZN1_ADC
> +	tristate "Renesas RZ/N1 ADC driver"
> +	depends on ARCH_RZN1 || COMPILE_TEST
> +	help
> +	  Say yes here to build support for the ADC found in Renesas
> +	  RZ/N1 family.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called rzn1-adc.
> +
>  config SC27XX_ADC
>  	tristate "Spreadtrum SC27xx series PMICs ADC"
>  	depends on MFD_SC27XX_PMIC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d008f78dc010..ba7a8a63d070 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_ROHM_BD79112) += rohm-bd79112.o
>  obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>  obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
> +obj-$(CONFIG_RZN1_ADC) += rzn1-adc.o
>  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
>  obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
>  obj-$(CONFIG_SOPHGO_CV1800B_ADC) += sophgo-cv1800b-adc.o
> diff --git a/drivers/iio/adc/rzn1-adc.c b/drivers/iio/adc/rzn1-adc.c
> new file mode 100644
> index 000000000000..f5e16b9cdf17
> --- /dev/null
> +++ b/drivers/iio/adc/rzn1-adc.c
> @@ -0,0 +1,626 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/N1 ADC driver
> + *
> + * Copyright (C) 2025 Schneider-Electric
> + *
> + * Author: Herve Codina <herve.codina@...tlin.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/bits.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
> +
> +/*                  ADC1 ADC2
> + * RZ/N1D, BGA 400   y    y
> + * RZ/N1D, BGA 324   y    n
> + * RZ/N1S, BGA 324   y    n
> + * RZ/N1S, BGA 196   y    n
> + * RZ/N1L, BGA 196   y    n
> + */
> +
> +#define RZN1_ADC_CONTROL_REG	0x2c
> +#define RZN1_ADC_CONTROL_ADC_BUSY	BIT(6)
> +#define RZN1_ADC_FORCE_REG		0x30
> +#define RZN1_ADC_SET_FORCE_REG		0x34
> +#define RZN1_ADC_CLEAR_FORCE_REG	0x38
> +#define RZN1_ADC_FORCE_VC(_n)		BIT(_n)
> +
> +#define RZN1_ADC_CONFIG_REG	0x40
> +#define RZN1_ADC_CONFIG_ADC_POWER_DOWN	BIT(3)
> +
> 

...

> +static int rzn1_adc_enable(struct rzn1_adc *rzn1_adc)
> +{
> +	int ret;
> +
> +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[0]);
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_core_power_on(&rzn1_adc->adc_core[1]);
> +	if (ret)
> +		goto poweroff_adc_core0;
> +
> +	ret = clk_prepare_enable(rzn1_adc->pclk);
> +	if (ret)
> +		goto poweroff_adc_core1;
> +
> +	ret = clk_prepare_enable(rzn1_adc->adc_clk);
> +	if (ret)
> +		goto disable_pclk;
> +
> +	ret = rzn1_adc_power(rzn1_adc, true);
> +	if (ret)
> +		goto disable_adc_clk;

Can we use devm_actions() on the above to avoid the complex error path plus the
.remove() callback?

> +
> +	return 0;
> +
> +disable_adc_clk:
> +	clk_disable_unprepare(rzn1_adc->adc_clk);
> +disable_pclk:
> +	clk_disable_unprepare(rzn1_adc->pclk);
> +poweroff_adc_core1:
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> +poweroff_adc_core0:
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> +	return ret;
> +}
> +
> +static void rzn1_adc_disable(struct rzn1_adc *rzn1_adc)
> +{
> +	rzn1_adc_power(rzn1_adc, false);
> +
> +	clk_disable_unprepare(rzn1_adc->adc_clk);
> +	clk_disable_unprepare(rzn1_adc->pclk);
> +
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[1]);
> +	rzn1_adc_core_power_off(&rzn1_adc->adc_core[0]);
> +}
> +
> +static int rzn1_adc_set_iio_dev_channels(struct rzn1_adc *rzn1_adc,
> +					 struct iio_dev *indio_dev)
> +{
> +	int adc_used;
> +
> +	adc_used = rzn1_adc->adc_core[0].is_used ? 0x01 : 0x00;
> +	adc_used |= rzn1_adc->adc_core[1].is_used ? 0x02 : 0x00;
> +
> +	switch (adc_used) {
> +	case 0x01:
> +		indio_dev->channels = rzn1_adc1_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc1_channels);
> +		return 0;
> +	case 0x02:
> +		indio_dev->channels = rzn1_adc2_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(rzn1_adc2_channels);
> +		return 0;
> +	case 0x03:
> +		indio_dev->channels = rzn1_adc1_adc2_channels;
> +		indio_dev->num_channels =
> ARRAY_SIZE(rzn1_adc1_adc2_channels);
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	dev_err(rzn1_adc->dev, "Failed to set IIO channels, no ADC core
> used\n");
> +	return -ENODEV;

dev_err_probe()?

> +}
> +
> +static int rzn1_adc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct iio_dev *indio_dev;
> +	struct rzn1_adc *rzn1_adc;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*rzn1_adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	rzn1_adc = iio_priv(indio_dev);
> +	rzn1_adc->dev = dev;
> +	mutex_init(&rzn1_adc->lock);

devm_mutex_init()

> +
> +	rzn1_adc->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rzn1_adc->regs))
> +		return PTR_ERR(rzn1_adc->regs);
> +
> +	rzn1_adc->pclk = devm_clk_get(dev, "pclk");
> +	if (IS_ERR(rzn1_adc->pclk))
> +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to
> get pclk\n");
> +
> +	rzn1_adc->adc_clk = devm_clk_get(dev, "adc-clk");
> +	if (IS_ERR(rzn1_adc->pclk))
> +		return dev_err_probe(dev, PTR_ERR(rzn1_adc->pclk), "Failed to
> get adc-clk\n");
> +
> +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[0],
> +					   "adc1-avdd", "adc1-vref");
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_core_get_regulators(rzn1_adc, &rzn1_adc->adc_core[1],
> +					   "adc2-avdd", "adc2-vref");
> +	if (ret)
> +		return ret;

Hmm, is avdd really an optional regulator? I mean can the ADC power up at all
without a supply in AVDD? Even vref seems to be mandatory as we can't properly
scale the sample without it.

Also, can't we have getting and enabling the regulator together? Then, we could
use some of the modern helpers to simplify the code (ok I see you use them in
the PM callbacks).

> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = dev_name(dev);

dev_name() should not be used for the above. It's typically the part name so I
guess in here "rzn1-adc" would be the appropriate one.

> +	indio_dev->info = &rzn1_adc_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	ret = rzn1_adc_set_iio_dev_channels(rzn1_adc, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = rzn1_adc_enable(rzn1_adc);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

There's a devm_pm_runtime_enable() API now.

> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		goto disable;
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +disable:
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +	return ret;
> +}
> +
> +static void rzn1_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	pm_runtime_disable(rzn1_adc->dev);
> +	pm_runtime_set_suspended(rzn1_adc->dev);
> +	pm_runtime_dont_use_autosuspend(rzn1_adc->dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +}

I'm fairly confident we can sanely go without .remove().
 
> +
> +static int rzn1_adc_pm_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	rzn1_adc_disable(rzn1_adc);
> +
> +	return 0;
> +}
> +
> +static int rzn1_adc_pm_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +
> +	return rzn1_adc_enable(rzn1_adc);
> +}
> +
> +static const struct dev_pm_ops rzn1_adc_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(rzn1_adc_pm_runtime_suspend,
> rzn1_adc_pm_runtime_resume, NULL)
> +};
> +
> +static const struct of_device_id rzn1_adc_of_match[] = {
> +	{ .compatible = "renesas,rzn1-adc" },
> +	{ /* sentinel */ },
> +};

We typically don't add the sentinel comment in IIO.

> +
> +MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);
> +
> +static struct platform_driver rzn1_adc_driver = {
> +	.probe = rzn1_adc_probe,
> +	.remove = rzn1_adc_remove,
> +	.driver = {
> +		.name = "rzn1-adc",
> +		.of_match_table = of_match_ptr(rzn1_adc_of_match),

Drop of_match_ptr().

- Nuno Sá

Powered by blists - more mailing lists