lkml.org 
[lkml]   [2018]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
    Hi Andrezj,

    On Tue, Mar 27, 2018 at 10:36 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
    > On 27.03.2018 09:36, Geert Uytterhoeven wrote:
    >> On Tue, Mar 27, 2018 at 9:28 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
    >>>>> --- /dev/null
    >>>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
    >>>>> +static void thc63_enable(struct drm_bridge *bridge)
    >>>>> +{
    >>>>> + struct thc63_dev *thc63 = to_thc63(bridge);
    >>>>> + struct regulator *vcc;
    >>>>> + int i;
    >>>> unsigned int i;
    >>> Why? You are introducing silly bug this way, see few lines below.
    >>>
    >>>>> +
    >>>>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
    >>>>> + vcc = thc63->vccs[i];
    >>>>> + if (!vcc)
    >>>>> + continue;
    >>>>> +
    >>>>> + if (regulator_enable(vcc))
    >>>>> + goto error_vcc_enable;
    >>>>> + }
    >>>>> +
    >>>>> + if (thc63->pdwn)
    >>>>> + gpiod_set_value(thc63->pdwn, 0);
    >>>>> +
    >>>>> + if (thc63->oe)
    >>>>> + gpiod_set_value(thc63->oe, 1);
    >>>>> +
    >>>>> + return;
    >>>>> +
    >>>>> +error_vcc_enable:
    >>>>> + dev_err(thc63->dev, "Failed to enable regulator %s\n",
    >>>>> + thc63_reg_names[i]);
    >>>>> +
    >>>>> + for (i = i - 1; i >= 0; i--) {
    >>> Here, the loop will not end if you define i unsigned.
    >> True.
    >>
    >>> I know one can change the loop, to make it working with unsigned. But
    >>> this clearly shows that using unsigned is more risky.
    >>> What are advantages of unsigned vs int in this case. Are there some
    >>> guidelines/discussions about it?
    >> Some people consider signed integers harmful, as they may be converted
    >> silently by the compiler to the "larger" unsigned type when needed.
    >
    > Wow, it sounds crazy, shall we expect gigantic patchsets, converting all
    > occurrences of int to "unsigned int" ? :)

    No we shall not.

    > I know both types have their pros and cons and can behave unexpectedly
    > in corner cases, but I do not see why unsigned should be preferred over
    > signed in general, or in this particular case.

    When looping over array indices, and comparing with ARRAY_SIZE (which
    is unsigned), using "unsigned int" is preferred.

    However, in this case the error code relies on the index becoming negative,
    so a signed integer should be used.

    > I guess there were somewhere discussion about it, could you point me to
    > it if possible, to avoid unnecessary noise in this thread.

    Not here, but Google pointed me to
    http://blog.robertelder.org/signed-or-unsigned/

    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

    \
     
     \ /
      Last update: 2018-03-27 11:06    [W:5.864 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site