Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <d0d42804-f437-e964-1c0d-4eb65e76db6c@linaro.org>
Date:   Wed, 19 Jan 2022 17:42:51 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.1
Subject: Re: [PATCH v3 3/7] iio: adc: qcom-spmi-rradc: introduce round robin
 adc
Content-Language: en-US
To:     Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Lee Jones <lee.jones@linaro.org>, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, sumit.semwal@linaro.org,
        amit.pundir@linaro.org, john.stultz@linaro.org
References: <20220106173131.3279580-1-caleb.connolly@linaro.org>
 <20220106173131.3279580-4-caleb.connolly@linaro.org>
 <20220109172948.76dbb1fa@jic23-huawei>
From:   Caleb Connolly <caleb.connolly@linaro.org>
In-Reply-To: <20220109172948.76dbb1fa@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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)