[PATCH v2 05/16] iio: adc: mediatek: add mt6323 PMIC AUXADC driver
Jonathan Cameron
jic23 at kernel.org
Tue May 12 06:29:32 PDT 2026
On Tue, 12 May 2026 08:18:19 +0300
Roman Vivchar via B4 Relay <devnull+rva333.protonmail.com at kernel.org> wrote:
> From: Roman Vivchar <rva333 at protonmail.com>
>
> The mt6323 AUXADC is a 15-bit ADC used for system monitoring. This driver
> provides support for reading various channels including battery and
> charger voltages, battery and chip temperature, current sensing and
> accessory detection.
>
> Add a driver for the AUXADC found in the MediaTek mt6323 PMIC.
>
> Tested-by: Ben Grisdale <bengris32 at protonmail.ch> # Amazon Echo Dot (2nd Generation)
> Signed-off-by: Roman Vivchar <rva333 at protonmail.com>
Hi Roman
Various comments inline - mostly naming related.
Jonathan
> diff --git a/drivers/iio/adc/mt6323-auxadc.c b/drivers/iio/adc/mt6323-auxadc.c
> new file mode 100644
> index 000000000000..2c2b495e3d38
> --- /dev/null
> +++ b/drivers/iio/adc/mt6323-auxadc.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2026 Roman Vivchar <rva333 at protonmail.com>
> + *
> + * Based on drivers/iio/adc/mt6359-auxadc.c
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/stringify.h>
> +#include <linux/types.h>
> +
> +#include <linux/mfd/mt6397/core.h>
> +#include <linux/mfd/mt6323/registers.h>
> +
> +#include <dt-bindings/iio/adc/mediatek,mt6323-auxadc.h>
> +
> +#define AUXADC_RSTB_SEL BIT(7)
> +#define AUXADC_RSTB_SW BIT(5)
> +
> +#define AUXADC_CTL_CK BIT(5)
> +
> +#define AUXADC_TRIM_CH2 (3 << 10)
> +#define AUXADC_TRIM_CH4 (3 << 8)
> +#define AUXADC_TRIM_CH5 (3 << 4)
> +#define AUXADC_TRIM_CH6 (3 << 2)
> +
> +#define AUXADC_VREF18_ENB_MD BIT(15)
> +#define AUXADC_MD_STATUS BIT(0)
> +
> +#define AUXADC_GPS_STATUS BIT(1)
> +
> +#define AUXADC_VREF18_SELB BIT(1)
> +#define AUXADC_DECI_GDLY_SEL BIT(0)
> +
> +#define AUXADC_VBUF_EN BIT(4)
> +
> +#define AUXADC_DECI_GDLY_MASK GENMASK(15, 14)
Why you can it is much better to clearly associate a field mask
definition with which register it is in. Lets us quickly spot
if there is a missmatch.
#define AUXADC_CON19_DECI_GDLY_MASK for example.
THough DECI_GDLY isn't exactly easy to understand as abbreviations
go!
> +#define AUXADC_ADC19_BUSY_MASK GENMASK(15, 1)
> +#define AUXADC_RDY_MASK BIT(15)
> +#define AUXADC_DATA_MASK GENMASK(14, 0)
> +
> +#define AUXADC_OSR_MASK GENMASK(12, 10)
> +#define AUXADC_DEFAULT_OSR 3
> +
> +#define AUXADC_LOW_CHANNEL_MASK GENMASK(9, 0)
> +#define AUXADC_AUDIO_CHANNEL_MASK GENMASK(8, 0)
> +
> +#define VOLTAGE_FULL_RANGE 1800
Probably better to have this inline - however if you do keep it
prefix t he define VOLTAGE_FULL_RANGE sounds too generic!
> +#define AUXADC_PRECISE 32768
I'd put that inline. Little benefit it in having it up here...
> +
> +#define MTK_PMIC_IIO_CHAN(_name, _idx, _ch_type) \
> +{ \
> + .type = _ch_type, \
> + .indexed = 1, \
> + .channel = _idx, \
> + .address = _idx, \
Why put an index in address? Seems to me that complicates things
vs putting the relevant register address in there.
> + .datasheet_name = __stringify(_name), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) \
> +}
> +
> +static const struct iio_chan_spec mt6323_auxadc_channels[] = {
> + MTK_PMIC_IIO_CHAN(baton2, MT6323_AUXADC_BATON2, IIO_VOLTAGE),
> + MTK_PMIC_IIO_CHAN(ch6, MT6323_AUXADC_CH6, IIO_VOLTAGE),
> + MTK_PMIC_IIO_CHAN(bat_temp, MT6323_AUXADC_BAT_TEMP, IIO_TEMP),
> + MTK_PMIC_IIO_CHAN(chip_temp, MT6323_AUXADC_CHIP_TEMP, IIO_TEMP),
> + MTK_PMIC_IIO_CHAN(vcdt, MT6323_AUXADC_VCDT, IIO_VOLTAGE),
> + MTK_PMIC_IIO_CHAN(baton1, MT6323_AUXADC_BATON1, IIO_VOLTAGE),
> + MTK_PMIC_IIO_CHAN(isense, MT6323_AUXADC_ISENSE, IIO_VOLTAGE),
> + MTK_PMIC_IIO_CHAN(batsns, MT6323_AUXADC_BATSNS, IIO_VOLTAGE),
> + MTK_PMIC_IIO_CHAN(accdet, MT6323_AUXADC_ACCDET, IIO_VOLTAGE),
> +};
> +
> +/**
> + * struct mt6323_auxadc - Main driver structure
> + * @regmap: Regmap from PWRAP
> + * @lock: Mutex to serialize AUXADC reading vs configuration
> + *
> + * The MediaTek MT6323 (as well as lot of other PMICs) have the following hierarchy:
> + * PMIC AUXADC <- PMIC MFD <- SoC PWRAP (wrapper for PWRAP FSM)
> + *
> + * Therefore, PWRAP regmap should be get using dev->parent->parent.
> + */
> +struct mt6323_auxadc {
> + struct regmap *regmap;
> + struct mutex lock;
> +};
> +
> +static u32 mt6323_auxadc_channel_to_reg(unsigned long channel)
> +{
> + switch (channel) {
> + case MT6323_AUXADC_BATON2:
> + return MT6323_AUXADC_ADC6;
You should put these in chan->address perhaps to avoid
need for a separate lookup function.
> + case MT6323_AUXADC_CH6:
> + return MT6323_AUXADC_ADC11;
> + case MT6323_AUXADC_BAT_TEMP:
> + return MT6323_AUXADC_ADC5;
> + case MT6323_AUXADC_CHIP_TEMP:
> + return MT6323_AUXADC_ADC4;
> + case MT6323_AUXADC_VCDT:
> + return MT6323_AUXADC_ADC2;
> + case MT6323_AUXADC_BATON1:
> + return MT6323_AUXADC_ADC3;
> + case MT6323_AUXADC_ISENSE:
> + return MT6323_AUXADC_ADC1;
> + case MT6323_AUXADC_BATSNS:
> + return MT6323_AUXADC_ADC0;
> + case MT6323_AUXADC_ACCDET:
> + return MT6323_AUXADC_ADC7;
> + default:
> + return MT6323_AUXADC_ADC17;
> + }
> +}
> +static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
> + unsigned long channel)
> +{
> + struct regmap *map = auxadc->regmap;
> + int ret;
> +
> + ret = regmap_set_bits(map, MT6323_AUXADC_CON11, AUXADC_VBUF_EN);
As above. I'd like that field name to include which register it is in.
That makes it easier to spot mismatches.
> + if (ret)
> + return ret;
> +
> + ret = regmap_clear_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> + if (ret)
> + return ret;
> +
> + return regmap_set_bits(map, MT6323_AUXADC_CON22, BIT(channel));
> +}
> +
> +static int mt6323_auxadc_read(struct mt6323_auxadc *auxadc,
> + const struct iio_chan_spec *chan, int *out)
> +{
> + struct regmap *map = auxadc->regmap;
> + u32 val, reg = mt6323_auxadc_channel_to_reg(chan->address);
Don't mix elements that assign with ones that don't. Doesn't make for easy
to read code.
> + int ret;
> +
> + ret = regmap_read_poll_timeout(map, reg, val, (val & AUXADC_RDY_MASK),
> + 1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> + if (ret)
> + return ret;
> +
> + *out = FIELD_GET(AUXADC_DATA_MASK, val);
> +
> + return 0;
> +}
> +
> +static int mt6323_auxadc_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int *val,
> + int *val2, long mask)
> +{
> + struct mt6323_auxadc *auxadc = iio_priv(indio_dev);
> + int ret, mult = 1;
> +
> + if (mask == IIO_CHAN_INFO_RAW) {
> + guard(mutex)(&auxadc->lock);
> + ret = mt6323_auxadc_prepare_channel(auxadc);
> + if (ret)
> + return ret;
> +
> + ret = mt6323_auxadc_request(auxadc, chan->address);
> + if (ret)
> + return ret;
> +
> + fsleep(300);
> +
> + ret = mt6323_auxadc_read(auxadc, chan, val);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + } else if (mask == IIO_CHAN_INFO_SCALE) {
Andy covered not having the else etc already I think. A switch might
work better though.
> + if (chan->channel == MT6323_AUXADC_ISENSE ||
> + chan->channel == MT6323_AUXADC_BATSNS)
> + mult = 4;
> +
> + *val = mult * VOLTAGE_FULL_RANGE;
> + *val2 = AUXADC_PRECISE;
IIO_VAL_FRACTIONAL_LOG2 probably more appropriate here
(which would be more obvious with the values down here.
> +
> + return IIO_VAL_FRACTIONAL;
> + } else
> + return -EINVAL;
> +}
> +
> +static int mt6323_auxadc_init(struct mt6323_auxadc *auxadc)
> +{
> + struct regmap *map = auxadc->regmap;
> + int ret;
> +
> + ret = regmap_set_bits(map, MT6323_STRUP_CON10,
> + AUXADC_RSTB_SW | AUXADC_RSTB_SEL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(map, MT6323_TOP_CKPDN2, AUXADC_CTL_CK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(map, MT6323_AUXADC_CON10,
> + AUXADC_TRIM_CH2 | AUXADC_TRIM_CH4 |
> + AUXADC_TRIM_CH5 | AUXADC_TRIM_CH6);
ret = regmap_set_bits(map, MT6323_AUXADC_CON10,
AUXADC_TRIM_CH2 | AUXADC_TRIM_CH4 |
AUXADC_TRIM_CH5 | AUXADC_TRIM_CH6);
is fine.
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(map, MT6323_AUXADC_CON27,
> + AUXADC_VREF18_ENB_MD | AUXADC_MD_STATUS);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(map, MT6323_AUXADC_CON19, AUXADC_GPS_STATUS);
> + if (ret)
> + return ret;
> +
> + ret = regmap_set_bits(map, MT6323_AUXADC_CON26,
> + AUXADC_VREF18_SELB | AUXADC_DECI_GDLY_SEL);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(map, MT6323_AUXADC_CON9, AUXADC_OSR_MASK,
> + FIELD_PREP(AUXADC_OSR_MASK,
> + AUXADC_DEFAULT_OSR));
> + return ret;
Might as well do
return regmap_update_bits()
> +}
More information about the Linux-mediatek
mailing list