Re: [PATCH v2 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC
From: Andy Shevchenko
Date: Thu Oct 30 2025 - 05:00:19 EST
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