Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:

Thanks for the contribution, my comments below.

TL;DR: seems like somebody just submitted a driver without doing an internal
review with the kernel developers familiar with the upstream (and I believe
there are such in your company). This is not good. You, guys, should try your
best and not put a burden on the maintainer's/reviewers' shoulders.

> 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.

...

> +#include <linux/kernel.h>

Definitely big no for this one.

> +#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>

And simple "no" to this one.

> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>

In IIO we do
1) follow IWYU principle;
2) keep headers ordered (with possible grouping of iio/* ones or others if more
than one and specific to a domain);

...

> +#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)

Please, make sure _REG definition values are on the column, same for bitfield
defs (some people like indent them slightly differently to the regs).

> +#define RZN1_ADC_CONFIG_REG	0x40
> +#define RZN1_ADC_CONFIG_ADC_POWER_DOWN	BIT(3)

...

> +#define RZN1_ADC_VC_REG(_n)	(0xc0 + 0x4 * (_n))

0x4 --> 4 as it's just a stride. Same for the rest of the similar cases.

...

> +struct rzn1_adc_core {
> +	int is_used;
> +	struct regulator *avdd;
> +	struct regulator *vref;

Possible waste of 4 bytes on some architectures. Please, run `pahole` and see
the opportunities to improve all data structure types in the driver.

> +};

...

> +static int rzn1_adc_core_power_on(struct rzn1_adc_core *adc_core)
> +{
> +	int ret;
> +
> +	if (!adc_core->is_used)
> +		return 0;
> +
> +	ret = regulator_enable(adc_core->avdd);
> +	if (ret)
> +		return ret;
> +
> +	ret = regulator_enable(adc_core->vref);
> +	if (ret) {
> +		regulator_disable(adc_core->avdd);
> +		return ret;
> +	}

Don't we have bulk API? Can't it be used here? Why?

> +	return 0;
> +}

...

> +static int rzn1_adc_core_get_regulators(struct rzn1_adc *rzn1_adc,
> +					struct rzn1_adc_core *adc_core,
> +					const char *avdd_name, const char *vref_name)
> +{
> +	struct device *dev = rzn1_adc->dev;
> +	int ret;
> +
> +	adc_core->avdd = devm_regulator_get_optional(dev, avdd_name);

	ret = PTR_ERR_OR_ZERO(...);

might simplify the below.

> +	if (IS_ERR(adc_core->avdd)) {
> +		ret = PTR_ERR(adc_core->avdd);
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get '%s' regulator\n",
> +					     avdd_name);
> +		adc_core->avdd = NULL;
> +	}

Like
	if (ret == -ENODEV)
		...
	else if (ret)
		return dev_err_probe(...);

> +	adc_core->vref = devm_regulator_get_optional(dev, vref_name);
> +	if (IS_ERR(adc_core->vref)) {
> +		ret = PTR_ERR(adc_core->vref);
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get '%s' regulator\n",
> +					     vref_name);
> +		adc_core->vref = NULL;
> +	}

Ditto. (However see above about bulk API)

I'm so much wondering when somebody provides a wrapper that _optional() for
regulator will be optional from device source code pattern so we won't see
anymore these checks for ENODEV (with chance > 0 of a mistake in some cases as
it's usually an unintuitive for use API if you need to test for the specific
error code).

> +	/*
> +	 * Both regulators (avdd and vref) need to be available to have the
> +	 * related adc_core used.
> +	 */
> +	adc_core->is_used = adc_core->vref && adc_core->avdd;
> +	return 0;
> +}

...

> +static int rzn1_adc_core_get_vref_mv(struct rzn1_adc_core *adc_core)
> +{
> +	int vref_uv;

_uV ?

> +
> +	if (!adc_core->vref)
> +		return -ENODEV;
> +
> +	vref_uv = regulator_get_voltage(adc_core->vref);
> +	if (vref_uv < 0)
> +		return vref_uv;
> +
> +	return vref_uv / 1000;

Hmm... (MICRO/MILLI) ?

> +}

...

> +static int rzn1_adc_power(struct rzn1_adc *rzn1_adc, bool power)
> +{
> +	u32 v;
> +
> +	writel(power ? 0 : RZN1_ADC_CONFIG_ADC_POWER_DOWN,
> +	       rzn1_adc->regs + RZN1_ADC_CONFIG_REG);
> +
> +	/*
> +	 * Wait for the ADC_BUSY to clear.

> +	 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.

Useless comment. It might be something else in some cases, we don't need to
explain this in every caller.

> +	 */
> +	return readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_CONTROL_REG,
> +					 v, !(v & RZN1_ADC_CONTROL_ADC_BUSY),
> +					 0, 500);
> +}

...

> +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> +					 int adc1_ch, int adc2_ch)
> +{
> +	u32 vc = 0;
> +
> +	if (adc1_ch != -1)

The >= 0 is more robust in case the value gets an error pointer or -err code.

> +		vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> +
> +	if (adc2_ch != -1)
> +		vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
> +
> +	writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
> +}

> +static int rzn1_adc_vc_wait_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> +				       u32 *adc1_data, u32 *adc2_data)
> +{
> +	u32 data_reg;
> +	int ret;
> +	u32 v;
> +
> +	/*
> +	 * When a VC is selected, it needs 20 ADC clocks to perform the
> +	 * conversion.
> +	 *
> +	 * The worst case is when the 16 VCs need to perform a conversion and
> +	 * our VC is the lowest in term of priority.
> +	 *
> +	 * In that case, the conversion is performed in 16 * 20 ADC clocks.
> +	 *
> +	 * The ADC clock can be set from 4MHz to 20MHz. This leads to a worst
> +	 * case of  16 * 20 * 1/4Mhz = 80us.
> +	 *
> +	 * Round it up to 100us

Missing period.

> +	 */
> +
> +	/*
> +	 * Wait for the ADC_FORCE_VC(n) to clear.
> +	 *
> +	 * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
> +	 */
> +	ret = readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_FORCE_REG,
> +					v, !(v & RZN1_ADC_FORCE_VC(ch)),
> +					0, 100);
> +	if (ret)
> +		return ret;
> +
> +	if (adc1_data) {
> +		data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC1_DATA_REG(ch));
> +		*adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
> +	}
> +
> +	if (adc2_data) {
> +		data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC2_DATA_REG(ch));
> +		*adc2_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
> +	}
> +
> +	return 0;
> +}

I almost stopped here, see TL;DR above why.

...

> +static const struct of_device_id rzn1_adc_of_match[] = {
> +	{ .compatible = "renesas,rzn1-adc" },
> +	{ /* sentinel */ },

No comments, no commas in terminator entry. This is a common style in IIO.

> +};

> +

Unneeded blank line.

> +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),

New code shouldn't use of_match_ptr().

> +		.pm = pm_ptr(&rzn1_adc_pm_ops),
> +	},
> +};

> +

Unneeded blank line.

> +module_platform_driver(rzn1_adc_driver);

-- 
With Best Regards,
Andy Shevchenko