linux-kernel - Re: [PATCH 2/2] iio: adc: qcom-pm8xxx-xoadc: fix incorrect calibration values

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: <506feaa8-5808-48d4-8cc7-baadd8f43976@baylibre.com>
Date: Mon, 27 Oct 2025 15:36:33 -0500
From: David Lechner <dlechner@...libre.com>
To: Antony Kurniawan Soemardi <linux@...nkusors.com>,
 Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: qcom-pm8xxx-xoadc: fix incorrect
 calibration values

On 10/27/25 2:53 PM, Antony Kurniawan Soemardi wrote:
> On 10/28/2025 1:35 AM, David Lechner wrote:
>> On 10/27/25 12:29 PM, Antony Kurniawan Soemardi wrote:
>>> On msm8960 phones, the XOADC driver was using incorrect calibration
>>> values:
>>> absolute calibration dx = 625000 uV, dy = 4 units
>>> ratiometric calibration dx = 1800, dy = -29041 units
>>>
>>> As a result, reading from the IIO bus returned unexpected results:
>>> in_voltage_7 (USB_VBUS): 0
>>> in_voltage_10 (125V): 0
>>>
>>> The issue was caused by not setting the ratiometric scale (amux_ip_rsv)
>>> from the predefined channels. Additionally, the downstream code always
>> Mentioning downstream kernels is usually a red flag. :-)
>>
>> We can justify it here though by saying that there is no documentation
>> available other than downstream source code, so we are just using it
>> as a reference.
> ah ok, rewording needed then
>>> set the ADC_ARB_USRP_DIG_PARAM register to PM8XXX_ADC_ARB_ANA_DIG [1].
>>> That value does not include the SEL_SHIFT0 and SEL_SHIFT1 bits. Enabling
>>> those bits caused calibration errors too, so they were removed.
>>>
>>> With these fixes, calibration now uses the correct values:
>>> absolute calibration dx = 625000 uV, dy = 6307 units
>>> ratiometric calibration dx = 1800, dy = 18249 units
>>>
>>> Reading from the IIO bus now returns expected results:
>>> in_voltage_7 (USB_VBUS): 4973836
>>> in_voltage_10 (125V): 1249405
>> Would be useful to mention which hardware you tested with in case
>> it turns out that there is some other hardware that does require the
>> SHIFT0/1 bits to be set.
> I did mention Sony Xperia SP from cover letter, but I haven't
> referenced it in this commit yet. Also I tried to search on Github for
> SHIFT0/1 bits, but couldn't find any usage of them...
>>> [1] https://github.com/LineageOS/android_kernel_sony_msm8960t/blob/93319b1e5aa343ec1c1aabcb028c5e88c7df7c01/drivers/hwmon/pm8xxx-adc.c#L407-L408
>>>
>> Since this is a fix, it should have a Fixes: tag. And it sounds like
>> possibly two separate fixes. In that case, it should be two separate
>> patches.
> Fixes into 63c3ecd946d4ae2879ec0d8c6dcb90132a74d831?

The correct format is:

Fixes: 63c3ecd946d4 ("iio: adc: add a driver for Qualcomm PM8xxx HK/XOADC")

See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes

>>> Signed-off-by: Antony Kurniawan Soemardi <linux@...nkusors.com>
>>> ---
>>>   drivers/iio/adc/qcom-pm8xxx-xoadc.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
>>> index 8555f34036fb13c41ac720dc02c1dc39876e9198..a53d361456ec36b66d258041877bd96ab37838c4 100644
>>> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
>>> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
>>> @@ -503,10 +503,11 @@ static int pm8xxx_read_channel_rsv(struct pm8xxx_xoadc *adc,
>>>           goto unlock;
>>>         /* Decimation factor */
>>> -    ret = regmap_write(adc->map, ADC_ARB_USRP_DIG_PARAM,
>>> -               ADC_ARB_USRP_DIG_PARAM_SEL_SHIFT0 |
>>> -               ADC_ARB_USRP_DIG_PARAM_SEL_SHIFT1 |
>>> -               ch->decimation << ADC_DIG_PARAM_DEC_SHIFT);
>>> +    ret = regmap_update_bits(adc->map,
>>> +                 ADC_ARB_USRP_DIG_PARAM,
>>> +                 ADC_ARB_USRP_DIG_PARAM_DEC_RATE0 |
>>> +                 ADC_ARB_USRP_DIG_PARAM_DEC_RATE1,
>>> +                 ch->decimation << ADC_DIG_PARAM_DEC_SHIFT);
>> As a follow-up patch, it would be nice to update the driver to use FIELD_PREP().
>>
>> I.e. remove ADC_ARB_USRP_DIG_PARAM_DEC_RATE0, ADC_ARB_USRP_DIG_PARAM_DEC_RATE1
>> and ADC_DIG_PARAM_DEC_SHIFT macros and replace them with one macro:
>>
>> #define ADC_ARB_USRP_DIG_PARAM_DEC_RATE        GENMASK(6, 5)
>>
>> Then use it like:
>>
>>     ret = regmap_update_bits(adc->map,
>>                  ADC_ARB_USRP_DIG_PARAM,
>>                  ADC_ARB_USRP_DIG_PARAM_DEC_RATE,
>>                  FIELD_PREP(ADC_ARB_USRP_DIG_PARAM_DEC_RATE,
>>                         ch->decimation));
>>
>> This should be done for all of the similar multi-bit fields.
> as a follow up patch, you mean next version of this patch series, or
> separate patch series?

Either way is fine. It's just a "nice to have" change.

>>>       if (ret)
>>>           goto unlock;
>>>   @@ -783,6 +784,7 @@ static int pm8xxx_xoadc_parse_channel(struct device *dev,
>>>       ch->calibration = VADC_CALIB_ABSOLUTE;
>>>       /* Everyone seems to use default ("type 2") decimation */
>>>       ch->decimation = VADC_DEF_DECIMATION;
>>> +    ch->amux_ip_rsv = hwchan->amux_ip_rsv;
>>>         if (!fwnode_property_read_u32(fwnode, "qcom,ratiometric", &rsv)) {
>>>           ch->calibration = VADC_CALIB_RATIOMETRIC;
>>>
>