[PATCH v1 4/5] iio: adc: mt6359: Add support for MediaTek MT6363 PMIC AUXADC
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Thu Jul 3 06:05:38 PDT 2025
Il 28/06/25 18:01, Jonathan Cameron ha scritto:
> On Mon, 23 Jun 2025 14:00:27 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.com> wrote:
>
>> MediaTek MT6363 is a PMIC found on MT8196/MT6991 board designs
>> and communicates with the SoC over SPMI.
>>
>> This PMIC integrates an Auxiliary ADC (AUXADC) which has a grand
>> total of 54 ADC channels: 49 PMIC-internal channels, 2 external
>> NTC thermistor channels and 2 generic ADC channels (mapped to 7
>> PMIC ADC external inputs).
>>
>> To use a generic ADC channel it is necessary to enable one of
>> the PMIC ADC inputs at a time and only then start the reading,
>> so in this case it is possible to read only one external input
>> for each generic ADC channel.
>>
>> Due to the lack of documentation, this implementation supports
>> using only one generic ADC channel, hence supports reading only
>> one external input at a time.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.com>
> Hi,
>
> A few comments that may or may not overlap with Andy's review.
>
> thanks,
>
> Jonathan
>
>> ---
>> drivers/iio/adc/mt6359-auxadc.c | 238 +++++++++++++++++++++++++++++---
>> 1 file changed, 217 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
>> index ae7b65f5f551..f49b0b6e78da 100644
>> --- a/drivers/iio/adc/mt6359-auxadc.c
>> +++ b/drivers/iio/adc/mt6359-auxadc.c
>
>> +enum mtk_pmic_auxadc_flags {
>> + MTK_PMIC_AUXADC_IS_SPMI = BIT(0),
>> + MTK_PMIC_AUXADC_NO_RESET = BIT(1),
>> +};
>
> With just two bits I think flags obscures what is going on over
> a pair of separate booleans.
>
Okay, I'll change this to two booleans then!
>> };
>> @@ -123,7 +155,9 @@ struct mtk_pmic_auxadc_chan {
>> * @desc: PMIC AUXADC channel data
>> * @regs: List of PMIC specific registers
>> * @sec_unlock_key: Security unlock key for HK_TOP writes
>> + * @vref_mv: AUXADC Reference Voltage (VREF) in millivolts
>> * @imp_adc_num: ADC channel for battery impedance readings
>> + * @flags: Feature flags
>> * @read_imp: Callback to read impedance channels
>> */
>> struct mtk_pmic_auxadc_info {
>> @@ -133,22 +167,33 @@ struct mtk_pmic_auxadc_info {
>> const struct mtk_pmic_auxadc_chan *desc;
>> const u16 *regs;
>> u16 sec_unlock_key;
>> + u16 vref_mv;
> I'd not worry about the space saving here and instead make this a u32 so that
> can avoid the casting when using this.
>
Okay, will do.
>> u8 imp_adc_num;
>> + u8 flags;
>
> As above. Pair of bool preferred.
>
>> int (*read_imp)(struct mt6359_auxadc *adc_dev,
>> const struct iio_chan_spec *chan, int *vbat, int *ibat);
>> };
>
>> static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
>> {
>> const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
>> @@ -379,13 +488,13 @@ static int mt6359_read_imp(struct mt6359_auxadc *adc_dev,
>> int ret;
>>
>> /* Start conversion */
>> - regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
>> + regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
>
> Given desc->req_idx is not introduced in this patch, why is this needed now
> but not previously? Maybe this change belongs in a separate patch with
> a description to explain that.
>
Oh wow, many many many... many thanks for catching this!!!!
This change was not supposed to be in this commit, as I had implemented both
the IBAT and VBAT "IMP" for the 6363 but then left them out because I was in
doubt whether to add them here or if they are read from the fuel gauge chip
transparently and in firmware; I wouldn't even be able to test that on the
MT8196 Chromebook that I'm bringing up, because the battery is managed by EC
instead, unlike smartphones.
I have to remove that from mt6359_read_imp, the addition was unintentional;
only mt6359_auxadc_read_adc() should use it (and already did before this commit).
>> ret = regmap_read_poll_timeout(regmap, cinfo->regs[desc->rdy_idx],
>> val, val & desc->rdy_mask,
>> IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>>
>> /* Stop conversion regardless of the result */
>> - regmap_write(regmap, cinfo->regs[PMIC_AUXADC_IMP0], 0);
>> + regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
>> if (ret)
>> return ret;
>>
>> @@ -416,6 +525,7 @@ static const struct mtk_pmic_auxadc_info mt6357_chip_info = {
>> .regs = mt6357_auxadc_regs,
>> .imp_adc_num = MT6357_IMP_ADC_NUM,
>> .read_imp = mt6358_read_imp,
>> + .vref_mv = 1800,
>> };
>>
>> static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
>> @@ -426,6 +536,7 @@ static const struct mtk_pmic_auxadc_info mt6358_chip_info = {
>> .regs = mt6358_auxadc_regs,
>> .imp_adc_num = MT6358_IMP_ADC_NUM,
>> .read_imp = mt6358_read_imp,
>> + .vref_mv = 1800,
>> };
>>
>> static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
>> @@ -436,6 +547,17 @@ static const struct mtk_pmic_auxadc_info mt6359_chip_info = {
>> .regs = mt6359_auxadc_regs,
>> .sec_unlock_key = 0x6359,
>> .read_imp = mt6359_read_imp,
>> + .vref_mv = 1800,
>
> Add vref_mv and code using it in a precursor patch. Not a problem that all
> vref_mv will be 1800 at that point. That way we can quickly see that it
> has no affect on existing parts, and simplify what is present in this patch.
>
Right, I agree. I'll move that addition to a separate patch.
>> +};
>> +
>> +static const struct mtk_pmic_auxadc_info mt6363_chip_info = {
>> + .model_name = "MT6363",
>> + .channels = mt6363_auxadc_channels,
>> + .num_channels = ARRAY_SIZE(mt6363_auxadc_channels),
>> + .desc = mt6363_auxadc_ch_desc,
>> + .regs = mt6363_auxadc_regs,
>> + .flags = MTK_PMIC_AUXADC_IS_SPMI | MTK_PMIC_AUXADC_NO_RESET,
>> + .vref_mv = 1840,
>> };
>>
>> static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
>> @@ -464,27 +586,74 @@ static int mt6359_auxadc_read_adc(struct mt6359_auxadc *adc_dev,
>> const struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
>> const struct mtk_pmic_auxadc_chan *desc = &cinfo->desc[chan->scan_index];
>> struct regmap *regmap = adc_dev->regmap;
>> - u32 val;
>> + u32 reg, rdy_mask, val, lval;
>> + u8 ext_sel;
>> int ret;
>>
>> + if (desc->ext_sel_idx >= 0) {
>> + ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, desc->ext_sel_pu);
>> + ext_sel |= FIELD_PREP(MT6363_EXT_CHAN_MASK, desc->ext_sel_ch);
>> +
>> + ret = regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
>> + MT6363_EXT_PURES_MASK | MT6363_EXT_CHAN_MASK,
>> + ext_sel);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> /* Request to start sampling for ADC channel */
>> ret = regmap_write(regmap, cinfo->regs[desc->req_idx], desc->req_mask);
>> if (ret)
>> - return ret;
>> + goto end;
>>
>> /* Wait until all samples are averaged */
>> fsleep(desc->num_samples * AUXADC_AVG_TIME_US);
>>
>> - ret = regmap_read_poll_timeout(regmap,
>> - cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1),
>> - val, val & PMIC_AUXADC_RDY_BIT,
>> + reg = cinfo->regs[PMIC_AUXADC_ADC0] + (chan->address << 1);
>> + rdy_mask = PMIC_AUXADC_RDY_BIT;
>> +
>> + /*
>> + * Even though for both PWRAP and SPMI cases the ADC HW signals that
>> + * the data is ready by setting AUXADC_RDY_BIT, for SPMI the register
>> + * read is only 8 bits long: for this case, the check has to be done
>> + * on the ADC(x)_H register (high bits) and the rdy_mask needs to be
>> + * shifted to the right by the same 8 bits.
>> + */
>> + if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
>
> This is getting close to the point where the complexity for the IS_SPMI case
> is compled enough you'd be better off just splitting the code. I'd try that
> and see if it ends up neater than this.
>
That was the first thing I did... but it's not so many changes... I'd be ending
up almost completely duplicating this driver for no reason.
I'm mostly sure that there won't be any more spmi-specific differences in the
AuxADC, but if one day turns out I'm wrong, I guess we can always split the
driver in two and move the SPMI platforms away... but again, I'm mostly sure
that won't happen.
>> + rdy_mask >>= 8;
>> + reg += 1;
>> + }
>> +
>> + ret = regmap_read_poll_timeout(regmap, reg, val, val & rdy_mask,
>> AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> - if (ret)
>> - return ret;
>> + if (ret) {
>> + dev_dbg(adc_dev->dev, "ADC read timeout for chan %lu\n", chan->address);
>> + goto end;
>> + }
>> +
>> + if (MTK_AUXADC_HAS_FLAG(cinfo, IS_SPMI)) {
>> + /* If the previous read succeeded, this can't fail */
>
> As per discussion with Andy, I don't think we can ever assume that.
>
I'll add all the checks around :-)
>> + regmap_read(regmap, reg - 1, &lval);
>> + val = (val << 8) | lval;
>> + }
>>
>> - /* Stop sampling */
>
> If you have code that ends up with an internal goto for a specific
> block, that often suggests you should be factoring some code out to simplify
> the flow.
>
> I would take everything between the activiate ADC GPIO and deactivate out
> as another function. That will still need a goto to get to the stop
> sampling but then we won't have the dance below where we do some
> stuff from the main code flow on error and then exit (with more after
> that not run).
>
Actually, I have removed the goto as well - I moved everything but the write
to stop the sampling to a new function; I'm calling it, then stopping the
sampling unconditionally, as was already done before.
To let you understand, this is the description:
/**
* mt6359_auxadc_sample_value() - Start ADC channel sampling and read value
* @adc_dev: Main driver structure
* @out: Preallocated variable to store the value read from HW
*
* This function starts the sampling for an ADC channel, waits until all
* of the samples are averaged and then reads the value from the HW.
*
* Note that the caller must stop the ADC sampling on its own, as this
* function *never* stops it.
*
* Return:
* Negative number for error;
* Upon success returns zero and writes the read value to *out.
*/
...you can imagine the rest, or anyway I'm sending a v2 shortly :-)
>> +end:
>> + /* Stop sampling unconditionally... */
>> regmap_write(regmap, cinfo->regs[desc->req_idx], 0);
>>
>> + /* ...and deactivate the ADC GPIO if previously done */
>> + if (desc->ext_sel_idx >= 0) {
>> + ext_sel = FIELD_PREP(MT6363_EXT_PURES_MASK, MT6363_PULLUP_RES_OPEN);
>> +
>> + regmap_update_bits(regmap, cinfo->regs[desc->ext_sel_idx],
>> + MT6363_EXT_PURES_MASK, ext_sel);
>> + }
>> +
>> + /* Check if we reached this point because of an error or regular flow */
>> + if (ret)
>> + return ret;
>> +
>> + /* Everything went fine, give back the ADC reading */
>> *out = val & GENMASK(chan->scan_type.realbits - 1, 0);
>> return 0;
>> }
>> @@ -505,7 +674,7 @@ static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
>> int ret;
>>
>> if (mask == IIO_CHAN_INFO_SCALE) {
>> - *val = desc->r_ratio.numerator * AUXADC_VOLT_FULL;
>> + *val = desc->r_ratio.numerator * (u32)cinfo->vref_mv;
>
> As above. If vref_mv was already a (u32) no need to cast here.
Changed!
Cheers,
Angelo
More information about the Linux-mediatek
mailing list