Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver
From: Chi-Wen Weng
Date: Mon Jun 29 2026 - 03:14:13 EST
Hi Andy,
Thanks for the review and the kind words.
I will address the cleanup comments in v2:
- trim the include list and add the missing specific headers such as
linux/types.h and linux/jiffies.h,
- rename the RMW helper to ma35d1_adc_update(),
- mask the update value in the helper,
- add set/clear bits helpers,
- use loop-local unsigned int indices where applicable,
- use devm_kasprintf() or drop the channel name handling if it is no
longer needed,
- switch to device_for_each_child_node_scoped(),
- simplify the IRQ and triggered-buffer error paths.
> > + if (have_single && have_diff)
> > + return -EINVAL;
>
> Is it possible IRL?
The EADC differential enable bit is global, so the check was intended to
reject buffered scans containing both single-ended and differential
channels.
However, after looking at the hardware constraints and the other review
comments, I plan to simplify v2 and reduce the initial upstream driver
scope. The v2 driver will focus on direct raw reads for the external
single-ended ADC channels only.
Triggered buffered capture, the device trigger and differential channel
support will be dropped from the initial submission. They can be added
later as follow-up patches once the scan sequencing, trigger model and
differential pair constraints are handled properly.
Thanks,
Chi-Wen
Andy Shevchenko 於 2026/6/26 下午 08:54 寫道:
On Thu, Jun 25, 2026 at 07:06:38PM +0800, Chi-Wen Weng wrote:
Add an IIO driver for the Nuvoton MA35D1 Enhanced ADC controller.Nice written driver, some small issues here and there, and I think in a couple
The driver supports direct raw reads and triggered buffered capture. The
controller end-of-conversion interrupt is exposed as the device trigger
and is used to push samples into the IIO buffer.
Channels are described by firmware child nodes and can be configured as
single-ended or differential inputs. Since the differential enable bit is
global, mixed single-ended and differential buffered scans are rejected.
DMA support is intentionally not included in this initial upstream driver;
conversions are handled through the interrupt-driven path.
of versions it will stabilize and can be accepted.
...
+#include <linux/bitfield.h>No need, bitmap.h covers this.
+#include <linux/bits.h>
+#include <linux/bitmap.h>No need, covered by platform_device.h.
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/err.h>No way this header should be in the mere drivers.
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>Also missing some headers, such as types.h.
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/property.h>
...
+#define MA35D1_EADC_TIMEOUT msecs_to_jiffies(1000)+ jiffies.h
...
+static inline u32 ma35d1_adc_read(struct ma35d1_adc *adc, u32 reg)Name it _update() to be aligned with the _read() and _write() above.
+{
+ return readl(adc->regs + reg);
+}
+
+static inline void ma35d1_adc_write(struct ma35d1_adc *adc, u32 reg, u32 val)
+{
+ writel(val, adc->regs + reg);
+}
+
+static void ma35d1_adc_rmw(struct ma35d1_adc *adc, u32 reg, u32 mask, u32 val)
+{Correct pattern is to use
+ u32 tmp;
+
+ tmp = ma35d1_adc_read(adc, reg);
+ tmp &= ~mask;
+ tmp |= val;
tmp = (tmp & ~mask) | (val & mask);
+ ma35d1_adc_write(adc, reg, tmp);...
+}
+static void ma35d1_adc_config_sample(struct ma35d1_adc *adc,I don't see the need of this variable, use the value directly.
+ unsigned int sample, unsigned int channel)
+{
+ u32 reg = MA35D1_EADC_SCTL(sample);
+ ma35d1_adc_rmw(adc, reg,...
+ MA35D1_EADC_SCTL_CHSEL_MASK |
+ MA35D1_EADC_SCTL_TRGSEL_MASK,
+ FIELD_PREP(MA35D1_EADC_SCTL_CHSEL_MASK, channel) |
+ MA35D1_EADC_SCTL_TRGSEL_ADINT0);
+}
+static irqreturn_t ma35d1_adc_trigger_handler(int irq, void *p)for (unsigned int i = 0; i < adc->scan_chancnt; i++)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ma35d1_adc *adc = iio_priv(indio_dev);
+ int i;
+
+ for (i = 0; i < adc->scan_chancnt; i++)
+ adc->scan.channels[i] =...
+ ma35d1_adc_read(adc, MA35D1_EADC_DAT(i)) &
+ MA35D1_EADC_DAT_MASK;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &adc->scan, pf->timestamp);
+ iio_trigger_notify_done(adc->trig);
+
+ ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
+ MA35D1_EADC_CTL_ADCIEN0);
+ ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
+
+ return IRQ_HANDLED;
+}
+static int ma35d1_adc_validate_scan(struct iio_dev *indio_dev,Make it last in the loop, it will be standard pattern. Otherwise it's hard to
+ const unsigned long *scan_mask)
+{
+ const struct iio_chan_spec *chan;
+ bool have_single = false;
+ bool have_diff = false;
+ unsigned int count = 0;
+ unsigned long bit;
+
+ for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
+ chan = &indio_dev->channels[bit];
+
+ if (chan->type == IIO_TIMESTAMP)
+ continue;
+ count++;
read and find. Also it's recommended to split assignment and definition
for better maintenance.
unsigned int count;
...
count = 0;
for_each_set_bit(bit, scan_mask, indio_dev->masklength) {
...
count++;
}
+ if (chan->differential)Is it possible IRL?
+ have_diff = true;
+ else
+ have_single = true;
+ }
+
+ if (!count || count > MA35D1_EADC_MAX_SAMPLE_MODULES)
+ return -EINVAL;
+ if (have_single && have_diff)
+ return -EINVAL;
+ return 0;...
+}
+static int ma35d1_adc_buffer_postenable(struct iio_dev *indio_dev)for (unsigned int i = 0; i < adc->scan_chancnt; i++)
+{
+ struct ma35d1_adc *adc = iio_priv(indio_dev);
+ int i;
+
+ if (!adc->scan_chancnt)
+ return -EINVAL;
+
+ ma35d1_adc_write(adc, MA35D1_EADC_STATUS2, MA35D1_EADC_STATUS2_ADIF0);
+ ma35d1_adc_rmw(adc, MA35D1_EADC_INTSRC0,
+ MA35D1_EADC_INTSRC0_ADINT0,
+ MA35D1_EADC_INTSRC0_ADINT0);
+ ma35d1_adc_rmw(adc, MA35D1_EADC_REFADJCTL,
+ MA35D1_EADC_REFADJCTL_EXT_VREF,
+ MA35D1_EADC_REFADJCTL_EXT_VREF);
+ ma35d1_adc_rmw(adc, MA35D1_EADC_SELSMP0, GENMASK(1, 0), 3);
+ ma35d1_adc_set_diff(adc, adc->scan_differential);
+ for (i = 0; i < adc->scan_chancnt; i++)
+ ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),Ditto.
+ MA35D1_EADC_SCTL_TRGDLY_MASK,
+ MA35D1_EADC_SCTL_TRGDLY_MASK);
+
+ ma35d1_adc_rmw(adc, MA35D1_EADC_CTL, MA35D1_EADC_CTL_ADCIEN0,
+ MA35D1_EADC_CTL_ADCIEN0);
+ ma35d1_adc_write(adc, MA35D1_EADC_SWTRG, 1);
+
+ return 0;
+}
+
+static int ma35d1_adc_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct ma35d1_adc *adc = iio_priv(indio_dev);
+ int i;
+
+ ma35d1_adc_disable_irq(adc);
+ for (i = 0; i < adc->scan_chancnt; i++)
+ ma35d1_adc_rmw(adc, MA35D1_EADC_SCTL(i),
+ MA35D1_EADC_SCTL_TRGSEL_MASK, 0);
Also looking to the cases of setting 0s, I would rather have a helper
_set_bits() / _clear_bits() in conjunction with _update().
+ return 0;...
+}
+static void ma35d1_adc_init_channel(struct ma35d1_adc *adc,Can compiler prove the buffer size is enough?
+ struct iio_chan_spec *chan, u32 vinp,
+ u32 vinn, int scan_index, bool differential)
+{
+ char *name = adc->chan_name[vinp];
+
+ chan->type = IIO_VOLTAGE;
+ chan->indexed = 1;
+ chan->channel = vinp;
+ chan->address = vinp;
+ chan->scan_index = scan_index;
+ chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+ chan->scan_type.sign = 'u';
+ chan->scan_type.realbits = 12;
+ chan->scan_type.storagebits = 16;
+ chan->scan_type.endianness = IIO_CPU;
+
+ if (differential) {
+ chan->differential = 1;
+ chan->channel2 = vinn;
+ snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d-in%d", vinp,
+ vinn);
+ } else {Why not use devm_kasprintf() instead?
+ snprintf(name, MA35D1_EADC_CHAN_NAME_LEN, "in%d", vinp);
+ }
+
+ chan->datasheet_name = name;
+}...
+static int ma35d1_adc_parse_channels(struct iio_dev *indio_dev,Perhaps >= ?
+ struct device *dev)
+{
+ struct ma35d1_adc *adc = iio_priv(indio_dev);
+ DECLARE_BITMAP(used_channels, MA35D1_EADC_MAX_CHANNELS);
+ struct fwnode_handle *child;
+ struct iio_chan_spec *channels;
+ int num_channels;
+ int scan_index = 0;
+ int ret;
+
+ bitmap_zero(used_channels, MA35D1_EADC_MAX_CHANNELS);
+
+ num_channels = device_get_child_node_count(dev);
+ if (!num_channels)
+ return dev_err_probe(dev, -ENODATA,
+ "no ADC channels configured\n");
+
+ if (num_channels > MA35D1_EADC_MAX_CHANNELS)
+ return dev_err_probe(dev, -EINVAL, "too many ADC channels\n");Use _scoped() variant.
+
+ channels = devm_kcalloc(dev, num_channels + 1, sizeof(*channels),
+ GFP_KERNEL);
+ if (!channels)
+ return -ENOMEM;
+
+ device_for_each_child_node(dev, child) {
+ u32 diff[2];...
+ u32 reg;
+ bool differential = false;
+
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, ret,
+ "missing channel reg property\n");
+ }
+
+ if (reg >= MA35D1_EADC_MAX_CHANNELS) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, -EINVAL,
+ "invalid ADC channel %u\n", reg);
+ }
+
+ if (test_and_set_bit(reg, used_channels)) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, -EINVAL,
+ "duplicate ADC channel %u\n", reg);
+ }
+
+ if (fwnode_property_present(child, "diff-channels")) {
+ ret = fwnode_property_read_u32_array(child,
+ "diff-channels",
+ diff,
+ ARRAY_SIZE(diff));
+ if (ret) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, ret,
+ "invalid diff-channels for channel %u\n",
+ reg);
+ }
+
+ if (diff[0] != reg ||
+ diff[1] >= MA35D1_EADC_MAX_CHANNELS ||
+ diff[0] == diff[1]) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, -EINVAL,
+ "invalid differential ADC channel %u-%u\n",
+ diff[0], diff[1]);
+ }
+
+ if (test_and_set_bit(diff[1], used_channels)) {
+ fwnode_handle_put(child);
+ return dev_err_probe(dev, -EINVAL,
+ "ADC channel %u already used\n",
+ diff[1]);
+ }
+
+ differential = true;
+ }
+
+ ma35d1_adc_init_channel(adc, &channels[scan_index], reg,
+ differential ? diff[1] : 0,
+ scan_index, differential);
+ scan_index++;
+ }
+
+ channels[scan_index] = (struct iio_chan_spec)
+ IIO_CHAN_SOFT_TIMESTAMP(scan_index);
+
+ indio_dev->channels = channels;
+ indio_dev->num_channels = scan_index + 1;
+ indio_dev->masklength = indio_dev->num_channels;
+
+ return 0;
+}
+ ret = devm_request_irq(dev, irq, ma35d1_adc_isr, 0, dev_name(dev),Make it a single line, here it's fine.
+ indio_dev);
+ if (ret)Remove duplicate error message.
+ return dev_err_probe(dev, ret, "failed to request IRQ %d\n", irq);
...
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,So, it seems this is very rarely can be not -ENOMEM, and hence it's 99.99% dead
+ iio_pollfunc_store_time,
+ ma35d1_adc_trigger_handler,
+ &ma35d1_adc_buffer_ops);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to setup triggered buffer\n");
code, just
return ret;