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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

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@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@linaro.org>
> > > Reviewed-by: Simon Glass <sjg@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]