Message-ID: <2250556.Mh6RI2rZIc@radijator>
Date: Fri, 29 Aug 2025 17:20:17 +0200
From: Duje Mihanović <duje@...emihanovic.xyz>
To: David Lechner <dlechner@...libre.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Karel Balej <balejk@...fyz.cz>,
 Lee Jones <lee@...nel.org>, David Wronek <david@...nlining.org>,
 phone-devel@...r.kernel.org, ~postmarketos/upstreaming@...ts.sr.ht,
 linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

On Friday, 29 August 2025 01:40:56 Central European Summer Time David Lechner wrote:
> > +#define ADC_CHANNEL(index, lsb, _type, name) {	\
> > +	.type = _type, \
> > +	.indexed = 1, \
> > +	.channel = index, \
> > +	.address = lsb, \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > +			      BIT(IIO_CHAN_INFO_PROCESSED), \
> > +	.datasheet_name = name, \
> 
> Do you have a link for the datasheet?

No, unfortunately. The only reference I have for the ADC itself is this
vendor driver:
https://github.com/acorn-marvell/brillo_pxa_kernel/blob/master/drivers/iio/adc/88pm88x-gpadc.c

> > +	ADC_CHANNEL(VSC_CHAN, 1367, IIO_VOLTAGE, "vsc"),
> > +	ADC_CHANNEL(VCHG_PWR_CHAN, 1709, IIO_VOLTAGE, "vchg_pwr"),
> > +	ADC_CHANNEL(VCF_OUT_CHAN, 1367, IIO_VOLTAGE, "vcf_out"),
> > +	ADC_CHANNEL(TINT_CHAN, 104, IIO_TEMP, "tint"),
> > +
> > +	ADC_CHANNEL(GPADC0_CHAN, 342, IIO_VOLTAGE, "gpadc0"),
> > +	ADC_CHANNEL(GPADC1_CHAN, 342, IIO_VOLTAGE, "gpadc1"),
> > +	ADC_CHANNEL(GPADC2_CHAN, 342, IIO_VOLTAGE, "gpadc2"),
> > +
> > +	ADC_CHANNEL(VBAT_CHAN, 1367, IIO_VOLTAGE, "vbat"),
> > +	ADC_CHANNEL(GNDDET1_CHAN, 342, IIO_VOLTAGE, "gnddet1"),
> > +	ADC_CHANNEL(GNDDET2_CHAN, 342, IIO_VOLTAGE, "gnddet2"),
> > +	ADC_CHANNEL(VBUS_CHAN, 1709, IIO_VOLTAGE, "vbus"),
> > +	ADC_CHANNEL(GPADC3_CHAN, 342, IIO_VOLTAGE, "gpadc3"),
> > +	ADC_CHANNEL(MIC_DET_CHAN, 1367, IIO_VOLTAGE, "mic_det"),
> > +	ADC_CHANNEL(VBAT_SLP_CHAN, 1367, IIO_VOLTAGE, "vbat_slp"),
> > +
> > +	ADC_CHANNEL(GPADC0_RES_CHAN, 342, IIO_RESISTANCE, "gpadc0_res"),
> > +	ADC_CHANNEL(GPADC1_RES_CHAN, 342, IIO_RESISTANCE, "gpadc1_res"),
> > +	ADC_CHANNEL(GPADC2_RES_CHAN, 342, IIO_RESISTANCE, "gpadc2_res"),
> > +	ADC_CHANNEL(GPADC3_RES_CHAN, 342, IIO_RESISTANCE, "gpadc3_res"),
> 
> Is it safe (or sensible) to have both voltage and resistance channels
> for the same input at the same time? It seems like if a voltage
> channel was connected to an active circuit, we would not want to be
> supplying current to it to take a resistance reading (this doesn't
> sound safe). Likewise, if a voltage input has a passive load on it,
> wouldn't the voltage channel always return 0 because no current was
> supplied to induce a voltate (doesn't seem sensible to have a channel
> that does notthing useful).
> 
> It might make sense to have some firmware (e.g. devicetree) to describe
> if the input is active or passive on the voltage inputs and set up the
> channels accordingly.
> 
> I'm also wondering if the other channels like vbat and vbus are always
> wired up to these things internally or if this channel usage is only for
> a specific system.

Looking at the vendor kernel, I am fairly confident that the channels
labeled gpadc are indeed general-purpose and connected to arbitrary
resistances (thermistors and battery detection depending on the board),
while the rest are fixed-function as their names imply.

The above most likely is safe as my board is still functional, but it
probably doesn't make sense to keep the voltage channels so I suppose
I'll drop these in v2.

> > +	int val, ret;
> > +	u8 buf[2];
> > +
> > +	if (chan >= GPADC0_RES_CHAN)
> > +		/* Resistor voltage drops are read from the corresponding voltage channel
> > */ +		chan -= GPADC0_RES_CHAN - GPADC0_CHAN;
> 
> Does this actually work for GPADC3_RES_CHAN?
> 
> GPADC3_RES_CHAN == GPADC0_RES_CHAN + 3 but GPADC3_CHAN != GPADC0_CHAN + 3

Good catch. Upon closer inspection, the order of the channel enum
doesn't matter much and I'll fix this by simply ordering the enum more
wisely.

> > +		     long mask)
> > +{
> > +	struct device *dev = iio->dev.parent;
> > +	int raw, ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (chan->type == IIO_RESISTANCE) {
> > +		raw = gpadc_get_resistor(iio, chan);
> > +		if (raw < 0) {
> > +			ret = raw;
> > +			goto out;
> > +		}
> > +
> > +		*val = raw;
> > +		dev_dbg(&iio->dev, "chan: %d, %d Ohm\n", chan->channel, *val);
> > +		ret = IIO_VAL_INT;
> > +		goto out;
> > +	}
> > +
> > +	raw = gpadc_get_raw(iio, chan->channel);
> > +	if (raw < 0) {
> > +		ret = raw;
> > +		goto out;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> 
> If there is IIO_CHAN_INFO_RAW, then we also should have IIO_CHAN_INFO_SCALE.
> 
> > +		*val = raw;
> > +		dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	case IIO_CHAN_INFO_PROCESSED: {
> 
> Unusual to have both raw and processed. What is the motivation?

I was following what ab8500-gpadc does, no particular motivation.
Considering the above, to me it makes the most sense to limit it to
processed.

> > @@ -67,6 +68,35 @@
> > 
> >  #define PM886_REG_BUCK4_VOUT		0xcf
> >  #define PM886_REG_BUCK5_VOUT		0xdd
> > 
> > +/* GPADC enable/disable registers */
> > +#define PM886_REG_GPADC_CONFIG1		0x1
> > +#define PM886_REG_GPADC_CONFIG2		0x2
> > +#define PM886_REG_GPADC_CONFIG3		0x3
> > +#define PM886_REG_GPADC_CONFIG6		0x6
> 
> Could just write this as:
> 
> #define PM886_REG_GPADC_CONFIG(n)		(n)
> 
> > +
> > +/* GPADC bias current configuration registers */
> > +#define PM886_REG_GPADC_CONFIG11	0xb
> > +#define PM886_REG_GPADC_CONFIG12	0xc
> > +#define PM886_REG_GPADC_CONFIG13	0xd
> > +#define PM886_REG_GPADC_CONFIG14	0xe
> > +#define PM886_REG_GPADC_CONFIG20	0x14
> 
> which covers these too.
> 
> Most of these aren't used anyway.
> 
> Also suspicious that there are 5 registers listed here
> but only 4 channels for resistance.

The last one is used to enable the bias generators, the rest only set
the bias current for their respective channel.

Regards,
--
Duje

Powered by blists - more mailing lists