Re: [PATCH v2 05/16] iio: adc: mediatek: add mt6323 PMIC AUXADC driver
From: Andy Shevchenko
Date: Tue May 12 2026 - 02:45:01 EST
On Tue, May 12, 2026 at 8:21 AM Roman Vivchar via B4 Relay
<devnull+rva333.protonmail.com@xxxxxxxxxx> wrote:
>
> 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.
...
> +#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>
+ time.h // USEC_PER_MSEC
> +#include <linux/types.h>
...
> +#define AUXADC_TRIM_CH2 (3 << 10)
> +#define AUXADC_TRIM_CH4 (3 << 8)
> +#define AUXADC_TRIM_CH5 (3 << 4)
> +#define AUXADC_TRIM_CH6 (3 << 2)
Without a comment it's hard to say if these are like masks or actual
values. Can you clarify that in the comment on top of these four?
> +#define VOLTAGE_FULL_RANGE 1800
Are there any units? Are they millivolts or is it just some scale?
...
> +#define MTK_PMIC_IIO_CHAN(_name, _idx, _ch_type) \
> +{ \
> + .type = _ch_type, \
> + .indexed = 1, \
> + .channel = _idx, \
> + .address = _idx, \
> + .datasheet_name = __stringify(_name), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) \
Keep the trailing comma as this is not a terminator.
> +}
...
> +/**
> + * 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.
get --> obtained
> + */
...
> +static int mt6323_auxadc_prepare_channel(struct mt6323_auxadc *auxadc)
> +{
> + struct regmap *map = auxadc->regmap;
> + u32 val;
> + int ret;
> +
> + ret = regmap_read(map, MT6323_AUXADC_CON19, &val);
> + if (ret)
> + return ret;
> +
> + /* The ADC is idle */
> + if (!(val & AUXADC_DECI_GDLY_MASK))
> + return 0;
> +
> + ret = regmap_read_poll_timeout(map, MT6323_AUXADC_ADC19, val,
> + !(val & AUXADC_ADC19_BUSY_MASK), 10, 500);
It's better to have a logical split
ret = regmap_read_poll_timeout(map, MT6323_AUXADC_ADC19,
val, !(val & AUXADC_ADC19_BUSY_MASK),
10, 500);
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(map, MT6323_AUXADC_CON19,
> + AUXADC_DECI_GDLY_MASK);
> +}
...
> +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);
> + int ret;
> +
> + ret = regmap_read_poll_timeout(map, reg, val, (val & AUXADC_RDY_MASK),
Parentheses are not needed in this case. But I'm fine with it here as
it probably makes it easier to get the idea.
> + 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)
Logical split
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;
Decouple assignment and definition. These types of assignments are
hard to maintain and might lead to subtle mistakes in the future.
> + 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;
Please, add a comment with the reference to a datasheet (ideally)
explaining this sleep.
> + fsleep(300);
> +
> + ret = mt6323_auxadc_read(auxadc, chan, val);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + } else if (mask == IIO_CHAN_INFO_SCALE) {
Redundant 'else'
> + if (chan->channel == MT6323_AUXADC_ISENSE ||
> + chan->channel == MT6323_AUXADC_BATSNS)
> + mult = 4;
> +
> + *val = mult * VOLTAGE_FULL_RANGE;
> + *val2 = AUXADC_PRECISE;
> +
> + return IIO_VAL_FRACTIONAL;
> + } else
Ditto, and it's the wrong style. Read the Coding Style documentation
to clarify this.
> + return -EINVAL;
> +}
...
> + ret = devm_mutex_init(dev, &auxadc->lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to initialize mutex\n");
Unneeded error message. Most likely it's -ENOMEM, which will be
ignored by dev_err_probe() anyway.
...
> + ret = devm_iio_device_register(dev, iio);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register iio device\n");
If you don't see the device, it's failed to register, do we need this message?
--
With Best Regards,
Andy Shevchenko