From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DDFB7CD4851 for ; Tue, 12 May 2026 13:30:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=xG4eUljkev6EFMCHkXPSd1wU7e+AIHUocEa6ai+XaE4=; b=agZ5M4a0q1RUZCvN5p8nfP1s8y JlPgpkIMT3BopFwH14+3bR/3ZwF4BKnONmuV8Dpp1aKMj37CU4U1q8sLjxA5a7S7W0mZMpelpM2A5 WvHpviHJdwizTSK0+YRMNEw2MZPK/jSDLmU6WJ0r2zN9XmYDyNBeEew+81H/I29maTHGZz+tuf0AD V/G1gprgbSSUupb0NSd0GhQ82xxx4dAgF5KWBN0FcMoKL6KsQg1N8DiqP8RwmxZeBXDft4hkDyVun arLYOdgJltk4l0nkINjAEbljpDi9OYXVbbBn7HVdxRiqiG4YmqVQIza5CnmZzRWkVZ3gkPZBedyJ7 QLoLyfBw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wMnBj-0000000Gr5i-2HII; Tue, 12 May 2026 13:30:19 +0000 Received: from tor.source.kernel.org ([172.105.4.254]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wMnBh-0000000Gr5J-45Hz; Tue, 12 May 2026 13:30:18 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 0B7146183A; Tue, 12 May 2026 13:29:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5DD06C2BCF6; Tue, 12 May 2026 13:29:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1778592586; bh=OE2XOA8Ys/J68M2y8Svmcp4K9BF42SihLc9PvIZ2c1U=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=m3bqYVgss5gGSLp+d7Jk7GkVsx9WevUkW0NCpmq904DyHvmgN774LUagw55vDZGyu LFNVeRPgb3SGxxeC0uIzZco6gL3Ft1tWpZGWx0oINYONCbj04EL154U7vvLo/hn9mN j+ZdFPGuVyOsF1VjQO7nz3Ic49b76nyCvnnizE48f9VGyApyCQx+fw+ot2QqJa3uoS v4+ubQQhpwXR5P2rvgkUTS5OmYEexMko68cLgCXQtslT6heauPB8jNc6ZwN85IO/gZ sYLoBwrfAhqkGgRYZy+SSc2r8atrc0fyKxPINOG5COJuZdKjWuLqwkdSpSoXUVfVNJ xZxJOsN8dq/Sg== Date: Tue, 12 May 2026 14:29:32 +0100 From: Jonathan Cameron To: Roman Vivchar via B4 Relay Cc: rva333@protonmail.com, David Lechner , Nuno =?UTF-8?B?U8Oh?= , Andy Shevchenko , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Matthias Brugger , AngeloGioacchino Del Regno , Sen Chu , Sean Wang , Macpaul Lin , Lee Jones , Srinivas Kandagatla , "Rafael J. Wysocki" , Daniel Lezcano , Zhang Rui , Lukasz Luba , linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-pm@vger.kernel.org, Ben Grisdale Subject: Re: [PATCH v2 05/16] iio: adc: mediatek: add mt6323 PMIC AUXADC driver Message-ID: <20260512142932.5c6801d1@jic23-huawei> In-Reply-To: <20260512-mt6323-v2-5-3efcba579e88@protonmail.com> References: <20260512-mt6323-v2-0-3efcba579e88@protonmail.com> <20260512-mt6323-v2-5-3efcba579e88@protonmail.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Tue, 12 May 2026 08:18:19 +0300 Roman Vivchar via B4 Relay wrote: > From: Roman Vivchar > > 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 # Amazon Echo Dot (2nd Generation) > Signed-off-by: Roman Vivchar 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 > + * > + * Based on drivers/iio/adc/mt6359-auxadc.c > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#include > + > +#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() > +}