Re: [PATCH 04/13] iio: adc: mediatek: add mt6323 PMIC AUXADC driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, May 04, 2026 at 09:24:56PM +0300, Roman Vivchar via B4 Relay 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/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/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>

Follow IWYU. At least stringify.h is missing.

...

> +#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) }

Make {} each to occupy a single line.

...

> +/**
> + * struct mt6323_auxadc - Main driver structure
> + * @dev:           Device pointer
> + * @regmap:        Regmap from PWRAP
> + * @lock:          Mutex to serialize AUXADC reading vs configuration

 *
 * ...put struct decription here... See below why.
 *

> + */
> +struct mt6323_auxadc {
> +	struct device *dev;
> +	struct regmap *regmap;

Do you need both? Are they different? (I mean that regmap may be derived from
device and vice versa depending on the case.)

Ok, it seems the dev is the current platform device, while regmap comes from
parent->parent to it (2 levels up!). This needs a good comment in the struct
description explaining the hierarchy.

> +	struct mutex lock;
> +};

...

> +static int mt6323_auxadc_check_if_stuck(struct mt6323_auxadc *auxadc)
> +{
> +	int i, ret;

Why is 'i' signed?

> +	u32 val;
> +
> +	for (i = 0; i < 50; i++) {

Magic 50 and the whole thing is reinvention of something from iopoll.h.

> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON19, &val);
> +		if (ret)
> +			return ret;
> +
> +		if (FIELD_GET(AUXADC_DECI_GDLY_MASK, val)) {
> +			ret = regmap_read(auxadc->regmap, MT6323_AUXADC_ADC19,
> +					  &val);
> +			if (ret)
> +				return ret;
> +
> +			if (!FIELD_GET(AUXADC_ADC19_BUSY_MASK, val)) {
> +				ret = regmap_update_bits(

Bad indentation, there is a room for parameter on the previous line.

> +					auxadc->regmap, MT6323_AUXADC_CON19,
> +					FIELD_PREP(AUXADC_DECI_GDLY_MASK, 3),
> +					0x0);
> +				if (ret)
> +					return ret;
> +			}
> +		} else {
> +			return 0;
> +		}
> +
> +		fsleep(10);
> +	}
> +
> +	return -ETIMEDOUT;
> +}

TL;DR: Find a suitable macro in iopoll.h and use it.

...

> +static int mt6323_auxadc_request(struct mt6323_auxadc *auxadc,
> +				 unsigned long channel)
> +{
> +	int ret;
> +	u32 pmic_val, adc_val;
> +
> +	if (channel < 9) {
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON11,
> +					 AUXADC_VBUF_EN, AUXADC_VBUF_EN);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON22,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_LOW_CHANNEL_MASK, pmic_val);
> +		adc_val &= ~BIT(channel);

We also have FIELD_MODIFY(). Use it in all cases where appropriate.

> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON22,
> +					 AUXADC_LOW_CHANNEL_MASK, adc_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON22,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_LOW_CHANNEL_MASK, pmic_val);
> +		adc_val |= BIT(channel);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON22,
> +					 AUXADC_LOW_CHANNEL_MASK, adc_val);

> +

Stray blank line.

> +	} else {

Redundant 'else' as this may be returned directly. So, refactor each branch to
a helper and use this as a small wrapper.

	if (channel < 9)
		return ...helper_for_chan_<_9...;

	return ...otherwise...;

> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON23,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_AUDIO_CHANNEL_MASK, pmic_val);
> +		adc_val &= ~BIT(channel - 9);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON23,
> +					 AUXADC_AUDIO_CHANNEL_MASK, adc_val);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(auxadc->regmap, MT6323_AUXADC_CON23,
> +				  &pmic_val);
> +		if (ret)
> +			return ret;
> +
> +		adc_val = FIELD_GET(AUXADC_AUDIO_CHANNEL_MASK, pmic_val);
> +		adc_val |= BIT(channel - 9);
> +
> +		ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON23,
> +					 AUXADC_AUDIO_CHANNEL_MASK, adc_val);
> +	}
> +
> +	return ret;
> +}

...

> +static int mt6323_auxadc_read(struct mt6323_auxadc *auxadc,
> +			      const struct iio_chan_spec *chan, int *out)
> +{
> +	int ret;
> +	u32 reg = mt6323_auxadc_channel_to_reg(chan->address);
> +	u32 val;
> +
> +	ret = regmap_read_poll_timeout(auxadc->regmap, reg, val,
> +				       (val & AUXADC_RDY_MASK), 1000, 100000);

Redundant parentheses, also use multipliers from time.h

	struct regmap *map = ... // use this trick elsewhere as well

	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) {
> +		scoped_guard(mutex, &auxadc->lock)
> +		{

Why? It's not a switch-case, the guard()() should be fine.

> +			ret = mt6323_auxadc_check_if_stuck(auxadc);
> +			if (ret)
> +				return ret;
> +
> +			ret = mt6323_auxadc_request(auxadc, chan->address);
> +			if (ret)
> +				return ret;
> +
> +			usleep_range(300, 500);

We have fsleep().

> +			ret = mt6323_auxadc_read(auxadc, chan, val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +		}
> +	} else if (mask == IIO_CHAN_INFO_SCALE) {
> +		if (chan->channel == MT6323_AUXADC_ISENSE ||
> +		    chan->address == MT6323_AUXADC_BATSNS)
> +			mult = 4;
> +
> +		*val = mult * 1800;
> +		*val2 = 32768;
> +
> +		return IIO_VAL_FRACTIONAL;
> +	} else
> +		return -EINVAL;
> +}

...

> +	ret = regmap_update_bits(auxadc->regmap, MT6323_AUXADC_CON10,
> +				 AUXADC_TRIM_CH2 | AUXADC_TRIM_CH4 |
> +					 AUXADC_TRIM_CH5 | AUXADC_TRIM_CH6,
> +				 AUXADC_TRIM_CH2 | AUXADC_TRIM_CH4 |
> +					 AUXADC_TRIM_CH5 | AUXADC_TRIM_CH6);
> +	if (ret)
> +		return ret;

We have _set_bits()/_clear_bits()/_assign_bits() of regmap API. Use them here
and in many other cases in this driver (and probably in the entire series.
I'm not going to comment each of the cases.

...

> +static int mt6323_auxadc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mt6323_auxadc *auxadc;
> +	struct iio_dev *iio;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	/* mfd->pwrap regmap */
> +	regmap = dev_get_regmap(dev->parent->parent, NULL);
> +	if (!regmap)
> +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*auxadc));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	auxadc = iio_priv(iio);
> +	auxadc->regmap = regmap;
> +	auxadc->dev = dev;

> +	mutex_init(&auxadc->lock);

devm.

> +	ret = mt6323_auxadc_init(auxadc);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to initialize auxadc\n");
> +
> +	iio->name = "mt6323-auxadc";
> +	iio->info = &mt6323_auxadc_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = mt6323_auxadc_channels;
> +	iio->num_channels = ARRAY_SIZE(mt6323_auxadc_channels);
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)

> +		return dev_err_probe(dev, ret,
> +				     "failed to register iio device\n");

One line

> +	return 0;
> +}

...

> +static const struct of_device_id mt6323_auxadc_of_match[] = {
> +	{ .compatible = "mediatek,mt6323-auxadc" },
> +	{}


IIRC we use { } (with space) in IIO for the terminator entry in ID tables.

> +};

-- 
With Best Regards,
Andy Shevchenko