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)
