Messages in this thread | | | Date | Wed, 19 Jan 2022 17:42:51 +0000 | Subject | Re: [PATCH v3 3/7] iio: adc: qcom-spmi-rradc: introduce round robin adc | From | Caleb Connolly <> |
| |
On 09/01/2022 17:29, Jonathan Cameron wrote: > On Thu, 6 Jan 2022 17:31:27 +0000 > Caleb Connolly <caleb.connolly@linaro.org> 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@linaro.org> > 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)
| |