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

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

On Wed, Oct 29, 2025 at 03:46:42PM +0100, 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 ADC cores (ADC1
> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.

Looks much better, thanks! My comments below.

...

+ array_size,h

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>

+ dev_printk.h
+ err.h

> +#include <linux/iio/iio.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>

+ mutex.h

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

+ types.h

...

> +#define RZN1_ADC_CONTROL_REG			0x2c

I would go with fixed-width values, e.g., 0x02c for the register definitions.

> +#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_CONFIG_REG			0x40

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

> +#define RZN1_ADC_ADC1_DATA_REG(_n)		(0x100 + 4 * (_n))
> +#define RZN1_ADC_ADC2_DATA_REG(_n)		(0x140 + 4 * (_n))

...

> +static int rzn1_adc_get_vref_mv(struct rzn1_adc *rzn1_adc, unsigned int chan)
> +{
> +	/*
> +	 * chan 0..7 use ADC1 ch 0..7. Vref related to ADC1 core
> +	 * chan 8..15 use ADC2 ch 0..7. Vref related to ADC2 core
> +	 */

Split it to two one line comments per each conditional.

> +	if (chan < 8)
> +		return rzn1_adc->adc1_vref_mv;
> +	else if (chan < 16)

Redundant 'else'.

> +		return rzn1_adc->adc2_vref_mv;
> +
> +	return -EINVAL;
> +}

...

> +static int rzn1_adc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct rzn1_adc *rzn1_adc = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = rzn1_adc_read_raw_ch(rzn1_adc, chan->channel, val);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = rzn1_adc_get_vref_mv(rzn1_adc, chan->channel);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		*val2 = 12;
> +		return IIO_VAL_FRACTIONAL_LOG2;

> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;

	default:
		return -EINVAL;

> +}

...

> +static const struct iio_info rzn1_adc_info = {
> +	.read_raw = &rzn1_adc_read_raw

Leave trailing comma, this is not a terminator entry.

> +};

...

> +	if (rzn1_adc->adc1_vref_mv >= 0) {

Can we call it _mV?

...

> +	if (rzn1_adc->adc2_vref_mv >= 0) {

Ditto.


...

> +	ret = devm_regulator_get_enable_optional(dev, avdd_name);
> +	if (ret < 0) {
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get '%s' regulator\n",
> +					     avdd_name);
> +		return 0;
> +	}

	if (ret == -ENODEV)
		return dev_err_probe(); // takes less LoCs
	if (ret < 0) // do we need ' < 0' part?
		return 0;

> +	ret = devm_regulator_get_enable_read_voltage(dev, vref_name);
> +	if (ret < 0) {
> +		if (ret != -ENODEV)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get '%s' regulator\n",
> +					     vref_name);
> +		return 0;
> +	}

In the same way as above.

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(rzn1_adc_pm_ops,
> +				 rzn1_adc_pm_runtime_suspend,
> +				 rzn1_adc_pm_runtime_resume, NULL);

Please, split it based on logic:

static DEFINE_RUNTIME_DEV_PM_OPS(rzn1_adc_pm_ops,
				 rzn1_adc_pm_runtime_suspend, rzn1_adc_pm_runtime_resume, NULL);

OR

static DEFINE_RUNTIME_DEV_PM_OPS(rzn1_adc_pm_ops,
				 rzn1_adc_pm_runtime_suspend,
				 rzn1_adc_pm_runtime_resume,
				 NULL);

-- 
With Best Regards,
Andy Shevchenko