Re: [PATCH] staging: iio: impedance-analyzer: ad5933: use div64_ul() instead of do_div()

[David Lechner]

On 2/16/26 9:52 AM, Archit Anant wrote:
> 
> On Thu, Jan 22, 2026 at 11:33 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxx> wrote:
>>
>> On Thu, Jan 22, 2026 at 10:27:52PM +0530, Archit Anant wrote:
>>> (Resending to the list, apologies for the private reply earlier.)
>>
>> First of all, do no top post!
>>
>>> Thanks for the review and the detailed math breakdown.
>>>
>>> I agree that `(freq * BIT_ULL(29)) / mclk` is algebraically superior and
>>> improves precision.
>>>
>>> However, regarding your concern about the original code:
>>>> "why the original code drops precision, was it deliberate?"
>>>
>>> Since I do not have the AD5933 hardware to test if the register expects the
>>> truncated value from `(mclk / 4)`, I am hesitant to change the logic in
>>> this patch.
>>>
>>> Would you prefer I:
>>> 1. Stick to a purely mechanical change (keep the logic equivalent, just use
>>> `div64_ul` and `BIT_ULL(27)` for readability)?
>>> 2. Or proceed with the `BIT_ULL(29)` simplification assuming the precision
>>> loss was unintentional?
>>
>> I definitely prefer #2. That's why I plead to AD people to confirm.
>>
>>> I'm leaning towards #1 for safety, but happy to do #2 if the maintainers
>>> (Lars/Michael) think it's safe.

> Gentle ping on this.


As Andy said, please don't top-post. Put the reply in context to make it easy
to see what "this" is referring to.

I moved your reply to show what we mean.

> 
> Has anyone from AD had a chance to confirm whether the original
> truncation via (mclk / 4) was intentional?
> 
> If there are no objections, I can prepare a v2 using the BIT_ULL(29)
> form as suggested.
> 
> Thanks,
> Archit


Given how old this driver is, I would not expect an reply as the
author has likely long forgotten what they were thinking at the
time.

I would keep the (mclk / 4) as it is to be safe as you suggest. There
could be userspace depending on exact values currently being returned.

>>
>>> On Thu, Jan 22, 2026 at 8:45 PM Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>>> wrote:
>>>
>>>> On Thu, Jan 22, 2026 at 05:12:42PM +0200, Andy Shevchenko wrote:
>>>>> On Thu, Jan 22, 2026 at 08:26:33PM +0530, Archit Anant wrote:
>>>>
>>>> ...
>>>>
>>>>>> -   freqreg = (u64)freq * (u64)(1 << 27);
>>>>>> -   do_div(freqreg, st->mclk_hz / 4);
>>>>>> +   freqreg = div64_ul((u64)freq * (u64)(1 << 27),
>>>>>> +                      st->mclk_hz / 4);
>>>>>
>>>>> It can be one line to begin with.
>>>>> Then drop that ugly castings and explicit big shifts.
>>>>>
>>>>>       freqreg = div64_ul(BIT_ULL(27) * freq, st->mclk_hz / 4);
>>>>>
>>>>> Now you can see That 4 is only 2 bits, so this can be written in
>>>>> simpler way:
>>>>>
>>>>>       freqreg = div64_ul(BIT_ULL(29) * freq, st->mclk_hz);
>>>>>
>>>>> which may give a better precision at the end of the day.
>>>>
>>>> It also might be worth to add a comment on top to explain (with given
>>>> context
>>>> I don't know if there is already one on top of the function, though).
>>>>
>>>> And I think we want AD people to comment on this and maybe explain better
>>>> the calculations done (and why the original code drops precision, was it
>>>> deliberate?).
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>
> 
>