Message-ID: <0b06794b-34c5-ec0d-59c6-8412a8789eaf@ti.com>
Date: Tue, 27 Aug 2024 20:44:58 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Jani Nikula <jani.nikula@...el.com>,
        Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>
CC: <mchehab@...nel.org>, <hverkuil-cisco@...all.nl>,
        <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <sebastian.fricke@...labora.com>, <linux-doc@...r.kernel.org>,
        <praneeth@...com>, <nm@...com>, <vigneshr@...com>, <s-jain1@...com>,
        <r-donadkar@...com>, <b-brnich@...com>, <detheridge@...com>,
        <p-mantena@...com>, <vijayp@...com>, <andi.shyti@...ux.intel.com>,
        <nicolas@...fresne.ca>, <jirislaby@...nel.org>, <davidgow@...gle.com>,
        <dlatypov@...gle.com>, <corbet@....net>, <broonie@...nel.org>,
        <rdunlap@...radead.org>, <nik.borisov@...e.com>, <Dave.Martin@....com>
Subject: Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest
 value

Hi Nikula,

Thanks for the review.

On 27/08/24 18:10, Jani Nikula wrote:
> On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@...com> wrote:

[..]

>> The equivalency between roundclosest w.r.t round_closest is same as
>> equivalency between existing macros rounddown w.r.t round_down. Functionally
>> both are same but the former is recommended to be used only for the scenario
>> where multiple is not power of 2 and latter is faster but is strictly for the
>> scenario where multiple is power of 2. I think the same is already summarized
>> well in commit message and further elaborated in the patch itself as part of
>> header file comments [1] so I personally don't think any update is required
>> w.r.t this.
> 
> I still don't think rounddown vs. round_down naming is a good example to
> model anything after.
> 
> I have yet to hear a single compelling argument in favor of having a
> single underscore in the middle of a name having implications about the
> inputs of a macro/function.
> 
> The macros being added here are at 2 or 3 in Rusty's scale [1]. We could
> aim for 6 (The name tells you how to use it), but also do opportunistic
> 8 (The compiler will warn if you get it wrong) for compile-time
> constants.
> 

The macros use existing round_up/round_down underneath, so need to check if
they have compile-time checks but either-way this should not block the current
series as the series does not aim to enhance the existing round_up/round_down
macros.

> As-is, these, and round_up/round_down, are just setting a trap for an
> unsuspecting kernel developer to fall into.
> 

I understand what you are saying but I believe this was already discussed in
original patch series [1] where you had raised the same issue and it was even
discussed if we want to go with a new naming convention (like
round_closest_up_pow_2) [2] or not and the final consensus reached on naming
was to align with the existing round*() macros [3]. Moreover, I didn't hear
back any further arguments on this in further 8 revisions carrying this patch,
so thought this was aligned per wider consensus.

Anyways, to re-iterate the discussion do we want to change to below naming scheme?

round_closest_up_pow_2
round_closest_down_pow_2
roundclosest

or any other suggestions for naming ?

If there is a wider consensus on the suggested name (and ok to deviate from
existing naming convention), then we can go ahead with that.

[1]: https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/
[2]: https://lore.kernel.org/all/5ebcf480-81c6-4c2d-96e8-727d44f21ca9@ti.com/
[3]:
https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/#:~:text=series%20is%20that%3A%0A%2D-,align,-naming%20(with%20the

Regards
Devarsh
> 
> BR,
> Jani.
> 
> 
> [1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> 
> 
>>
>> [1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com
>>
>> Regards
>> Devarsh
> 

Powered by blists - more mailing lists