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

[David Lechner]

On 2/16/26 1:02 PM, David Lechner wrote:
> 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.
> 

Actually, I just saw this was in staging, so I guess not breaking userspace
isn't the top priority if there the other way is objectively better. I would
assume that testing/measurement could show which is actually correct.

>>>
>>>> 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
>>>
>>>
>>
>>
>