Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220211091927.2988283-1-javierm@redhat.com> <20220211091927.2988283-2-javierm@redhat.com>
 <YgY6OqN+guBlt/ED@smile.fi.intel.com> <4fa465d9-4fac-4199-9a04-d8e09d164308@redhat.com>
 <YgZEuXvJ2ZiOyNS+@smile.fi.intel.com> <7560cd10-0a7c-3fda-da83-9008833e3901@suse.de>
 <87pmnt7gm3.fsf@intel.com>
In-Reply-To: <87pmnt7gm3.fsf@intel.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 11 Feb 2022 13:27:29 +0100
Message-ID: <CAMuHMdWJ_T5HoWcOuSCDP-uquqpiWGPUmKNaC9U5d=JkGRGP0g@mail.gmail.com>
Subject: Re: [PATCH v4 1/6] drm/format-helper: Add drm_fb_xrgb8888_to_gray8_line()
To:     Jani Nikula <jani.nikula@linux.intel.com>
Cc:     Thomas Zimmermann <tzimmermann@suse.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Javier Martinez Canillas <javierm@redhat.com>,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        =?UTF-8?Q?Noralf_Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Maxime Ripard <maxime@cerno.tech>,
        Sam Ravnborg <sam@ravnborg.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jani,

On Fri, Feb 11, 2022 at 1:06 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 11 Feb 2022, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > Am 11.02.22 um 12:12 schrieb Andy Shevchenko:
> >> On Fri, Feb 11, 2022 at 11:40:13AM +0100, Javier Martinez Canillas wrote:
> >>> On 2/11/22 11:28, Andy Shevchenko wrote:
> >>>> On Fri, Feb 11, 2022 at 10:19:22AM +0100, Javier Martinez Canillas wrote:
> >>
> >> ...
> >>
> >>>>> +static void drm_fb_xrgb8888_to_gray8_line(u8 *dst, const u32 *src, unsigned int pixels)
> >>>>> +{
> >>>>> + unsigned int x;
> >>>>> +
> >>>>> + for (x = 0; x < pixels; x++) {
> >>>>> +         u8 r = (*src & 0x00ff0000) >> 16;
> >>>>> +         u8 g = (*src & 0x0000ff00) >> 8;
> >>>>> +         u8 b =  *src & 0x000000ff;
> >>>>> +
> >>>>> +         /* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
> >>>>> +         *dst++ = (3 * r + 6 * g + b) / 10;
> >>>>> +         src++;
> >>>>> + }
> >>>>
> >>>> Can be done as
> >>>>
> >>>>    while (pixels--) {
> >>>>            ...
> >>>>    }
> >>>>
> >>>> or
> >>>>
> >>>>    do {
> >>>>            ...
> >>>>    } while (--pixels);
> >>>>
> >>>
> >>> I don't see why a while loop would be an improvement here TBH.
> >>
> >> Less letters to parse when reading the code.
> >
> > It's a simple refactoring of code that has worked well so far. Let's
> > leave it as-is for now.
>
> IMO *always* prefer a for loop over while or do-while.

(guess what ;-) I tend to disagree.

> The for (i = 0; i < N; i++) is such a strong paradigm in C. You
> instantly know how many times you're going to loop, at a glance. Not so
> with with the alternatives, which should be used sparingly.

In this case it's fairly obvious, and you get rid of the extra variable x.
Less code, less variables, what can go wrong? ;-)

> And yes, the do-while suggested above is buggy, and you actually need to
> stop and think to see why.

Yes, that one is wrong.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds