Re: [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton super io chips

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Jul 1, 2022 at 11:15 AM Henning Schild
<henning.schild@xxxxxxxxxxx> 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?

...

> +#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()?

-- 
With Best Regards,
Andy Shevchenko