[PATCH] video: Add parentheses around VNBYTES() macro

[Tom Rini]

On Mon, Sep 02, 2024 at 10:17:59AM +0200, Miquel Raynal wrote:
> Hello,
> 
> trini at konsulko.com wrote on Tue, 19 Sep 2023 11:14:56 -0400:
> 
> > On Mon, Sep 18, 2023 at 07:04:41PM -0600, Simon Glass wrote:
> > > Hi Tom, Dan,
> > > 
> > > On Tue, 8 Aug 2023 at 19:39, Tom Rini <trini at konsulko.com> wrote:  
> > > >
> > > > On Wed, Jul 26, 2023 at 09:54:08AM +0300, Dan Carpenter wrote:
> > > >  
> > > > > The VNBYTES() macro needs to have parentheses to prevent some (harmless)
> > > > > macro expansion bugs.  The VNBYTES() macro is used like this:
> > > > >
> > > > >       VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
> > > > >
> > > > > The * operation is done before the / operation.  It still ends up with
> > > > > the same results, but it's not ideal.
> > > > >
> > > > > Signed-off-by: Dan Carpenter <dan.carpenter at linaro.org>
> > > > > Reviewed-by: Simon Glass <sjg at chromium.org>  
> > > >
> > > > In that this seems correct:
> > > >
> > > > Applied to u-boot/next, thanks!
> > > >
> > > > But I want to note that with gcc-13.1 (and binutils 2.40) or more
> > > > specifically the kernel.org arm/aarch64/etc toolchains, this causes the
> > > > generated code to change:
> > > >    aarch64: (for 1/1 boards) all +8.0 text +8.0
> > > >             bananapi-m5    : all +8 text +8
> > > >                u-boot: add: 0/0, grow: 1/0 bytes: 8/0 (8)
> > > >                  function                                   old     new   delta
> > > >                  video_post_probe                           248     256      +8
> > > >
> > > > So I don't know that this was a harmless bug.  
> > > 
> > > Hmm I should have read the comment directly above...
> > > 
> > > "Note we omit the outer brackets to allow multiplication by fractional pixels."
> > > 
> > > Having said that, it doesn't seem to matter getting (if indeed we do)
> > > a less accurate result.  
> > 
> > But we should fix the comment then, or decide no, this is intentional.
> 
> One year has passed and I just stumbled upon this incoherency. It would
> be nice to clarify the comment or otherwise to revert Dan's patch if
> judged irrelevant in the end.

I guess at this point I vote for fixing the comment.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240906/cd31391a/attachment.sig>