Message-ID: <20220701131233.68b0f507@md1za8fc.ad001.siemens.net>
Date:   Fri, 1 Jul 2022 13:12:33 +0200
From:   Henning Schild <henning.schild@...mens.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Linus Walleij <linus.walleij@...aro.org>,
        Tasanakorn Phaipool <tasanakorn@...il.com>,
        Sheng-Yuan Huang <syhuang3@...oton.com>,
        Kuan-Wei Ho <cwho@...oton.com>
Subject: Re: [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton
 super io chips

Am Fri, 1 Jul 2022 12:45:58 +0200
schrieb Andy Shevchenko <andy.shevchenko@...il.com>:

> On Fri, Jul 1, 2022 at 11:15 AM Henning Schild
> <henning.schild@...mens.com> wrote:
> >
> > This patch adds gpio support for several Nuvoton NCTXXX chips.
> > These super io chips offer multiple functions of which several
> > already have drivers in  
> 
> Super-I/O (to be consistent with the help in Kconfig, etc).
>
> 
> > the kernel, i.e. hwmon and wdt.  
> 
> watchdog
> 
> ...
> 
> Since you are talking about authorship in the cover letter, is it
> possible to get the original authorship to be preserved in the commit
> and authors / co-developers giving their SoB tags?

In fact i am afraid the original authors might stay silent. I do not
want to claim to be the author, i am just the one who polished for
mainline. And i do want to give the original authors the credit.

But i will not spoof git author or add SoBs, unless i get explicit
permission ... which might not happen.

A lot of the findings in the code below are actually what looks like
copied code from drivers/gpio/gpio-f7188x.c and
drivers/gpio/gpio-winbond.c but this being a new driver i could apply
the improvements here.

I would have to look over the proposed changes, would not want to touch
too much because i can only test one of the 4 chips.

regards,
Henning

> ...
> 
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/gpio/driver.h>  
> 
> Keep it sorted?
> 
> ...
> 
> > +#define SIO_ID_MASK            0xFFF0  
> 
> GENMASK() ?
> 
> ...
> 
> > +enum chips {
> > +       nct5104d,
> > +       nct6106d,
> > +       nct6116d,
> > +       nct6122d,
> > +};
> > +
> > +static const char * const nct6116d_names[] = {
> > +       "nct5104d",
> > +       "nct6106d",
> > +       "nct6116d",
> > +       "nct6122d",  
> 
> It would be slightly more flexible to use enum values as indices here:
> 
> [nct5104d] = "nct5104d",
> 
> > +};  
> 
> ...
> 
> > +               pr_err(DRVNAME "I/O address 0x%04x already in
> > use\n", base);  
> 
> Why not use pr_fmt() properly and drop DRVNAME here and in other
> pr_*(), if any?
> 
> ...
> 
> > +static int nct6116d_gpio_direction_in(struct gpio_chip *chip,
> > unsigned int offset); +static int nct6116d_gpio_get(struct
> > gpio_chip *chip, unsigned int offset); +static int
> > nct6116d_gpio_direction_out(struct gpio_chip *chip,
> > +                                    unsigned int offset, int
> > value); +static void nct6116d_gpio_set(struct gpio_chip *chip,
> > unsigned int offset, int value);  
> 
> Is it possible to avoid forward declarations?
> 
> ...
> 
> > +#define NCT6116D_GPIO_BANK(_base, _ngpio, _regbase, _label)
> >             \
> > +       {
> >     \
> > +               .chip = {
> >     \
> > +                       .label            = _label,
> >     \
> > +                       .owner            = THIS_MODULE,
> >     \
> > +                       .direction_input  =
> > nct6116d_gpio_direction_in, \
> > +                       .get              = nct6116d_gpio_get,
> >     \
> > +                       .direction_output =
> > nct6116d_gpio_direction_out,        \
> > +                       .set              = nct6116d_gpio_set,
> >     \  
> 
> .get_direction ?
> 
> > +                       .base             = _base,
> >     \
> > +                       .ngpio            = _ngpio,
> >     \
> > +                       .can_sleep        = false,
> >     \
> > +               },
> >     \
> > +               .regbase = _regbase,
> >     \
> > +       }  
> 
> ...
> 
> > +       int err;
> > +       struct nct6116d_gpio_bank *bank =
> > +               container_of(chip, struct nct6116d_gpio_bank,
> > chip);  
> 
> Can it be transformed to macro or inliner and then
> 
>        struct nct6116d_gpio_bank *bank = to_nct6116d_gpio_bank(chip);
> 
> > +       struct nct6116d_sio *sio = bank->data->sio;
> > +       u8 dir;  
> 
> Here and everywhere else, perhaps keep the reversed xmas tree order?
> 
> ...
> 
> > +               err = devm_gpiochip_add_data(&pdev->dev,
> > &bank->chip, bank);
> > +               if (err) {
> > +                       dev_err(&pdev->dev,
> > +                               "Failed to register gpiochip %d:
> > %d\n",
> > +                               i, err);
> > +                       return err;  
> 
> return dev_err_probe(...);
> 
> ...
> 
> > +       pr_info(DRVNAME ": Found %s at %#x chip id 0x%04x\n",
> > +                       nct6116d_names[sio->type],
> > +                       (unsigned int)addr,  
> 
> Casting in printf() very often means a wrong specifier in use.
> 
> > +                       devid);  
> 
> ...
> 
> > +       nct6116d_gpio_pdev = platform_device_alloc(DRVNAME, -1);
> > +       if (!nct6116d_gpio_pdev)
> > +               return -ENOMEM;
> > +
> > +       err = platform_device_add_data(nct6116d_gpio_pdev, sio,
> > sizeof(*sio));
> > +       if (err) {
> > +               pr_err(DRVNAME "Platform data allocation failed\n");
> > +               goto err;
> > +       }
> > +
> > +       err = platform_device_add(nct6116d_gpio_pdev);
> > +       if (err) {
> > +               pr_err(DRVNAME "Device addition failed\n");
> > +               goto err;
> > +       }  
> 
> Wonder, don't we have some shortcuts for these? Perhaps
> platform_device_register_full()?
> 

Powered by blists - more mailing lists