Hi Alyssa,
Thank you for the patch.
On Wed, Sep 01, 2021 at 01:54:27PM -0400, Alyssa Rosenzweig wrote:
> This constructs a fixed 16.16 rational, useful to specify the minimum
> and maximum scaling in drm_atomic_helper_check_plane_state. It is
> open-coded as a macro in multiple drivers, so let's share the helper.
>
> Signed-off-by: Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx>
> ---
> include/drm/drm_fixed.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 553210c02ee0..df1f369b4918 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -208,4 +208,9 @@ static inline s64 drm_fixp_exp(s64 x)
> return sum;
> }
>
Missing documentation :-)
> +static inline int drm_fixed_16_16(s32 mult, s32 div)
You should return a s32.
The function name isn't very explicit, and departs from the naming
scheme of other functions in the same file. As fixed-point numbers are
stored in a s64 for the drm_fixp_* helpers, we shouldn't rese the
drm_fixp_ prefix, maybe drm_fixp_s16_16_ would be a good prefix. The
function should probably be named drm_fixp_s16_16 from_fraction() then,
but then the same logic should possibly be replicated to ensure optimal
precision. I wonder if it wouldn't be best to simply use
drm_fixp_from_fraction() and shift the result right by 16 bits.
> +{
> + return (mult << 16) / div;
> +}
> +
> #endif
--
Regards,
Laurent Pinchart
