Re: [PATCH v2 2/3] iio: adc: mt6397-auxadc: add mt6397 PMIC AUXADC driver

From: Ryan Brue

Date: Sat Sep 19 2026 - 22:48:43 EST

On 9/18/26 2:31 AM, Andy Shevchenko wrote:
The below should be part of the comment block, no commit message needs to
be polluted with this.

A new driver was created here, instead of modifying an existing driver
such as mt6323-auxadc or mt6359-auxadc, for the following reasons:

- Both mt6323-auxadc and mt6359-auxadc select channels through a request
register (1 bit per channel), while mt6397 uses a 4-bit numeric field
CHSEL in CON1 (10:7), and then pulses a START bit (CON1 bit 0).

- For mt6323-auxadc, which is the closest I could find to the mt6397
(CON0..CON27), it has 13 more registers than the mt6397 (CON0..CON14).
It uses CON22 for its request register, and reads the result value
from the same register as the ready bit. We don't do that - the mt6397
has a factory calibrated value for each channel at 0x16 higher than the
raw value. mt6323 also has a 1800 mV / 15 bit scale / resolution while
we have 1200 mV / 10 bits. We also have some per-channel preparation
that we have to do before the burst, that the mt6323 doesn't have to
do.

- For mt6359-auxadc, it has a more generic framework for describing the
AUXADC, but it assumes requests are channel-per-bit, and so we would
have to basically ignore req_idx, req_mask, rdy_idx, and rdy_mask.

- We also have our own software sampling, which the vendor does too
(Amazon Fire OS based on Linux 3.18). We'd have to have our own
sampling callback to do it.

Assisted-by: LLM
Signed-off-by: Ryan Brue<ryanbrue.dev@xxxxxxxxx>
---
...here is the comment block...
Done in v3.
+static int mt6397_auxadc_read_once(struct mt6397_auxadc *adc,
+ const struct iio_chan_spec *chan, int *val)
+{
+ struct regmap *map = adc->regmap;
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_update_bits(map, MT6397_AUXADC_CON1,
+ MT6397_AUXADC_CON1_CHSEL,
+ FIELD_PREP(MT6397_AUXADC_CON1_CHSEL,
+ chan->address));
Make it two a bit long lines rather than four.
Done in v3.
+ if (ret)
+ return ret;
+
+ /* START is edge triggered: it has to be lowered before being raised. */
+ ret = regmap_clear_bits(map, MT6397_AUXADC_CON1, MT6397_AUXADC_CON1_START);
+ if (ret)
+ return ret;
Does it need any settling timeout (in case it was set before)?
I don't think so. The ready bit gets cleared when START gets raised, not when it gets lowered, so a missed edge wouldn't surface as an error. The poll would match the previous conversion's ready bit and a stale value would be averaged into the burst, so I forced one by suppressing the clear on START.

With the clear suppressed the set finds START already high, so regmap doesn't issue a write at all and no edge happens. All 40 reads still succeeded, but each one returns sixteen identical conversions instead of the usual spread, and nothing shows up in dmesg.

It also let me calibrate the probe, which I wanted before trusting a zero out of it. Probing the result register just after the rise, it counts the injected stale bits exactly: 93.75% with the clear suppressed, which is 15 of 16 because in the first conversion of a burst START is already low and still makes an edge, and 43.79% against 43.75% predicted when seven of sixteen are made stale. In normal operation everything it sees is conversions that have already finished, and once I subtract those out, what's left at 30 us -- which is where the poll first looks -- is -5.1e-3 +/- 2.9e-3 over 96000 conversions.

The two writes are never back to back anyway, since each one is its own transaction on an uncached regmap over the PMIC wrapper and the set alone takes at least 7.3 us. I also tried inserting a gap of 30 and 300 us, and it moves the reading by under 0.06 LSB, with no poll timing out across about a million conversions.

Even though the vendor isn't necessarily what we care about, it also writes START 0 then 1 with nothing in between.

I don't have a datasheet figure for a minimum low time, this is all measured, so if you end up wanting a wait there let me know, v3 has a bit more context in the comment.

+static int mt6397_auxadc_battemp_bias(struct mt6397_auxadc *adc, bool on)
There is nothing common between on==false and on==true cases. Make it two
distinct functions and drop bool parameter. It's actually a recommended
pattern.
Done in v3.
+{
+ struct regmap *map = adc->regmap;
+ int ret, err;
+
+ if (on) {
+ ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
+ MT6397_AUXADC_CON0_BUF_PWD_ON);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6397_AUXADC_CON0,
+ MT6397_AUXADC_CON0_BUF_PWD_B);
+ if (ret)
+ return ret;
+
+ ret = regmap_set_bits(map, MT6397_CHR_CON7,
+ MT6397_CHR_CON7_BATON_TDET_EN);
+ } else {
+ /*
+ * Every step of the teardown is attempted even if an earlier
+ * one failed, so that one failing write cannot leave the bias
+ * or the input buffer powered. The first error is reported.
+ */
+ ret = regmap_clear_bits(map, MT6397_CHR_CON7,
+ MT6397_CHR_CON7_BATON_TDET_EN);
+
+ err = regmap_clear_bits(map, MT6397_AUXADC_CON0,
+ MT6397_AUXADC_CON0_BUF_PWD_B);
+ if (!ret)
+ ret = err;
+
+ err = regmap_clear_bits(map, MT6397_AUXADC_CON0,
+ MT6397_AUXADC_CON0_BUF_PWD_ON);
+ if (!ret)
+ ret = err;
These 'if (!ret)' bug me. What can we do if the ret == 0 and err != 0
on the caller's level? In other words, what can caller do in such a case?
Nothing, and in v2 it didn't do anything -- read_channel() discarded the teardown's return anyways, so the value wasn't even used. v3 hands it to dev_err() instead, as mt6323-auxadc.c does on its own release path, and the accumulators are gone.

The 'if (!ret)' left in read_channel() are sequencing the next step rather than merging an error into it, and the teardown below it is unconditional, but if you don't want 'if (!ret)' at all, let me know.
+ if (!ret)
+ ret = err;
+
+ return ret;
This can be written as

if (ret)
return ret;

return err;


But the same Q as per above remains.
I took it a step further and made both teardowns return on the first failure rather than attempting the rest. mt6323_auxadc_release() does the same, and so do twelve others in drivers/iio that I could find. One does continue after a failed write -- ltr390_powerdown(), but it's a void devm cleanup callback that logs each error as it comes, so it has nothing to return.

With this change, a failed write can leave the later bits set. The read still returns its value with the failure logged, and the teardown runs at the end of every read, so the next read of that channel clears them.
+ /* Held across the whole burst: the channel select is shared state. */
Unneeded comment. It's obvious that guard()() takes the whole scope.
Done in v3.
+ guard(mutex)(&adc->lock);
+
+ /*
+ * Once any part of the per-channel setup has been written, the
+ * teardown has to run, so every exit below goes through it.
+ */
+ if (isense) {
+ ret = mt6397_auxadc_isense_enable(adc);
+ if (ret)
+ goto out_teardown;
Have you compiled this?
Yes, with clang on arm64 and gcc on x86_64 allmodconfig, W=1 clean at every patch in the series, and the codegen was correct: one mutex_lock, one mutex_unlock and a single ret in read_raw() with read_channel() inlined into it, and no path that takes the lock reaches that ret without passing the unlock. The only two branches ahead of the lock are the SCALE and default cases, and neither takes it. Still, I missed that cleanup.h wants goto and cleanup helpers to not mix, and I shouldn't have mixed them. v3 puts the sample loop into its own function, so read_channel() becomes setup, then a conditional burst and an unconditional teardown, with no label.
+ fsleep(MT6397_AUXADC_ISENSE_SETTLE_US);
+ } else {
+ ret = mt6397_auxadc_battemp_bias(adc, true);
+ if (ret)
+ goto out_teardown;
+ fsleep(MT6397_AUXADC_BATTEMP_SETTLE_US);
+ }
+
+ for (unsigned int i = 0; i < MT6397_AUXADC_SAMPLES; i++) {
+ ret = mt6397_auxadc_read_once(adc, chan, &sample);
+ if (ret)
+ goto out_teardown;
+
+ sum += sample;
+ }
+
+ *val = DIV_ROUND_CLOSEST(sum, MT6397_AUXADC_SAMPLES);
+
+out_teardown:
+ /* Lower START so the converter is not left armed between reads. */
+ regmap_clear_bits(adc->regmap, MT6397_AUXADC_CON1,
+ MT6397_AUXADC_CON1_START);
+
+ if (isense)
+ mt6397_auxadc_isense_disable(adc);
+ else
+ mt6397_auxadc_battemp_bias(adc, false);
+
+ return ret;
+}
So, this function has to refactored. And please, compile and test the code
*each* time you update it.
Ack. Admittedly I hadn't tested the 'goto out_teardown' path prior to sending out the v2, so I apologize. I have now tested it by injecting a failure into each of the helpers read_channel() calls. All five paths return the helper's errno, the teardown runs, and leaves every bit the partial setup wrote clear again, and guard() releases the mutex.

I also stubbed out the teardown as a negative control, and the same faults do leave the bits set in that situation, so the check can fail. The restructure is in v3.
...

+static int mt6397_auxadc_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct mt6397_auxadc *adc = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = mt6397_auxadc_read_channel(adc, chan, val);
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ /* 1200 mV full range with 10-bit resolution. */
+ *val = 1200;
+ if (chan->channel == MT6397_AUXADC_ISENSE)
+ *val *= MT6397_AUXADC_ISENSE_DIVIDER;
Make it if-else.
Done in v3.
+ *val2 = 10;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+
+ default:
+ return -EINVAL;
+ }
+}
Thanks Andy!

Best regards,
Ryan