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: <20220710102110.39748-1-tmaimon77@gmail.com> <20220710102110.39748-3-tmaimon77@gmail.com>
 <CAHp75VdaT8WjoHYgmUY+mKVaUivLGGeaRJAkwfRjHspPAmw_XQ@mail.gmail.com> <CAP6Zq1iQL2R67dbdQiXW_JUWrGne9xp-9yQ97vQOb4RveQE7Yw@mail.gmail.com>
In-Reply-To: <CAP6Zq1iQL2R67dbdQiXW_JUWrGne9xp-9yQ97vQOb4RveQE7Yw@mail.gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 12 Jul 2022 14:47:01 +0200
Message-ID: <CAHp75VfYbFYcp6eKQsSJ9-0Wz-9=UaM8ERWyOkcXfWvinZHB7w@mail.gmail.com>
Subject: Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver
To:     Tomer Maimon <tmaimon77@gmail.com>
Cc:     Avi Fishman <avifishman70@gmail.com>,
        Tali Perry <tali.perry1@gmail.com>,
        Joel Stanley <joel@jms.id.au>,
        Patrick Venture <venture@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Benjamin Fair <benjaminfair@google.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        =?UTF-8?Q?Jonathan_Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        zhengbin13@huawei.com, OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 12, 2022 at 1:33 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> On Sun, 10 Jul 2022 at 22:36, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jul 10, 2022 at 12:44 PM Tomer Maimon <tmaimon77@gmail.com> wrote:

Please, remove unneeded context when replying!

...

> > > +       if (pincfg[pin].flag & SLEWLPC) {
> > > +               switch (arg) {
> > > +               case 0:
> > > +                       regmap_update_bits(gcr_regmap, NPCM8XX_GCR_SRCNT,
> > > +                                          SRCNT_ESPI, 0);
> > > +                       return 0;
> > > +               case 1:
> > > +                       regmap_update_bits(gcr_regmap, NPCM8XX_GCR_SRCNT,
> > > +                                          SRCNT_ESPI, SRCNT_ESPI);
> > > +                       return 0;
> > > +               default:
> > > +                       return -EINVAL;
> > > +               }
> > > +       }
> > > +
> > > +       return -EINVAL;
> >
> > Why not to use usual pattern, i.e.
> >
> >   if (error_condition)
> >     return -EINVAL;
> What do you mean? like if (arg>1) return -EINVAL? It just seems more readable.

  if (!(pincfg[pin].flag & SLEWLPC))
    return -EINVAL;

  switch(...) {
    ...
  }

> > here and everywhere in the similar cases?
> can you point me to which more cases you mean?

Any you find that follows this pattern. Actually the rule of thumb is
to address all places in the code even if reviewer has given a comment
against one occurrence of something.

...

> > > +               val = ioread32(bank->base + NPCM8XX_GP_N_ODSC)
> > > +               & pinmask;
> >
> > What was happened to indentation? Check entire file for indentation to be okay.
> Sorry, I didn't understand, could you explain the comment again?

Indentation is a code formatting technique that allows the text to be
more readable. In particular when lines are split they should
logically follow what code does and point to the code relation. Here
you have '&' on the next line with indentation starting at the 'val's
column. This is not readable and confusing. In this case formatting on
one line fixes all issues. Possible alternative is to clearly show how
the 'val' is being modified:

   val = ioread32(...);
   val &= mask;

But see above about the amount of LoCs.

...

> > > +                       } else if ((nanosecs > 3496) && (nanosecs <= 4136)) {
> > > +                               iowrite32(0x60, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> > > +                       } else if ((nanosecs > 4136) && (nanosecs <= 5025)) {
> > > +                               iowrite32(0x70, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> >
> > With switch-case with ranges it will be much more visible what's going
> > on. Also think about it, maybe you can use some formula instead? Or
> > table (array of integers) approach where index will show the lowest
> > range value.
> There it can be described in a formula. Will be done with switch-case

I'm not sure I follow. If you can use a formula, use it!

...

> > > +               if (ret) {
> > > +                       dev_err(dev, "bgpio_init() failed\n");
> > > +                       return ret;
> > > +               }
> >
> > Use
> >
> >   return dev_err_probe(...)
> Why it is better to use dev_err_probe?

(beside deferred probe, which may be not the case here)
- standardized format
- less LoCs

> I am not sure that the error will be EPROBE_DEFER, all the failure
> cases the driver returned the
> error in the code.

...and it's fine to use dev_err_probe() as stated in its documentation.

> > In ->probe() and satellite functions.

-- 
With Best Regards,
Andy Shevchenko