Re: [PATCH v3 3/7] iio: adc: qcom-spmi-rradc: introduce round robin adc

[Caleb Connolly]

On 09/01/2022 17:29, Jonathan Cameron wrote:
> On Thu,  6 Jan 2022 17:31:27 +0000
> Caleb Connolly <caleb.connolly@xxxxxxxxxx> wrote:
>
> > The Round Robin ADC is responsible for reading data about the rate of
> > charge from the USB or DC in jacks, it can also read the battery
> > ID (resistence) and some temperatures. It is found on the PMI8998 and
> > PM660 Qualcomm PMICs.
> >
> > Signed-off-by: Caleb Connolly <caleb.connolly@xxxxxxxxxx>
> Hi Calib,
Hi Jonathan,

I've spent some time on this and mostly reworked things, thanks a lot for
your feedback, it's been quite interesting to learn about IIO. :)

Quite a few of the channels fit well into the (adc_code + offset) * scale format,
however the one you commented on "rradc_post_process_chg_temp()" doesn't seem to
fit, it requires multiple steps of applying offsets and scale and I haven't been
able to re-arrange it to work sensibly.

I noticed the calibbias properties which seems like something I should expose
for "rradc_get_fab_coeff()"?

Could you point me in the right direction here? For reference my WIP tree can be
found here: https://github.com/aospm/linux/commits/upstreaming/spmi-rradc

I also tried switching to labels, but I found that when I drop the extend_name
property the driver fails to probe because multiple channels end up with the same
name in sysfs (e.g. "in_temp_raw"). I've read through the docs and looked at a few
other drivers but I wasn't able to find out what I'm missing for this to work.

I've snipped to the relevant bits below.

Kind regards,
Caleb
> Various things inline but biggest is probably that in IIO we prefer
> if possible to make application of offsets and scales a job for the caller,
> either userspace or in kernel callers. This allows them to maintain precision
> better if they need to further transform the data.
>
> Jonathan
>
> > ---
> >   drivers/iio/adc/Kconfig           |   13 +
> >   drivers/iio/adc/Makefile          |    1 +
> >   drivers/iio/adc/qcom-spmi-rradc.c | 1070 +++++++++++++++++++++++++++++
> >   3 files changed, 1084 insertions(+)
> >   create mode 100644 drivers/iio/adc/qcom-spmi-rradc.c

[snip]

> > +static int rradc_post_process_chg_temp(struct rradc_chip *chip, u16 adc_code,
> > +				       int *result_millidegc)
> > +{
> > +	int64_t uv, offset, slope;
> > +	int ret;
> > +
> > +	ret = rradc_get_fab_coeff(chip, &offset, &slope);
> > +	if (ret < 0) {
> > +		dev_err(chip->dev, "Unable to get fab id coefficients\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	uv = ((int64_t)adc_code * RR_ADC_TEMP_FS_VOLTAGE_NUM);
> > +	uv = div64_s64(uv,
> > +		       (RR_ADC_TEMP_FS_VOLTAGE_DEN * RR_ADC_CHAN_MAX_VALUE));
> > +	uv = offset - uv;
> > +	uv = div64_s64((uv * MILLI), slope);
> > +	uv += RR_ADC_CHG_TEMP_OFFSET_MILLI_DEGC;
> > +	*result_millidegc = (int)uv;
>
> Marginally harder than the one below, but this is still looking like it can
> be well expressed as an offset + scale.  Thus making the tedious maths
> userspaces or callers problem.  I'm working backwards hence won't comment on
> similar before this point. Key is to transform whatever maths you have into
>
> (adc_code + offset) * scale then expose offset and scale as well as the
> raw value.  The right maths will get done for in kernel users and
> userspace can do it nicely with floating point.
>
> > +
> > +	return 0;
> > +}

[snip]

> > +static const struct iio_chan_spec rradc_iio_chans[RR_ADC_CHAN_MAX] = {
> > +	{
> > +		.extend_name = "batt_id",
>
> We recently introduced channel labels to try and avoid the need for
> extend_name.  The problem with extend_name is that generic software then
> has trouble parsing the resulting sysfs files as they can have very
> freeform naming.  Moving it to label makes that much easier.  Note that
> there is code to give a default label of extend_name to work around
> this problem for older drivers.
>
> > +		.type = IIO_RESISTANCE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > +		.address = RR_ADC_BATT_ID,
> > +	},

> Thanks,
>
> Jonathan

--
Kind Regards,
Caleb (they/them)