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
