lkml.org 
[lkml]   [2022]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@gmail.com>:

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

I ended up doing nothing here, code did not really look simpler.

For all the other points, except for the SoBs and authorship i did
something in v2.

Henning


\
 
 \ /
  Last update: 2022-07-04 15:10    [W:0.063 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site