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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d0d42804-f437-e964-1c0d-4eb65e76db6c@linaro.org>
Date:   Wed, 19 Jan 2022 17:42:51 +0000
From:   Caleb Connolly <caleb.connolly@...aro.org>
To:     Jonathan Cameron <jic23@...23.retrosnub.co.uk>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Lee Jones <lee.jones@...aro.org>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, sumit.semwal@...aro.org,
        amit.pundir@...aro.org, john.stultz@...aro.org
Subject: Re: [PATCH v3 3/7] iio: adc: qcom-spmi-rradc: introduce round robin
 adc



On 09/01/2022 17:29, Jonathan Cameron wrote:
> On Thu,  6 Jan 2022 17:31:27 +0000
> Caleb Connolly <caleb.connolly@...aro.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@...aro.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)