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;
Date:   Mon, 4 Jul 2022 15:08:42 +0200
From:   Henning Schild <henning.schild@siemens.com>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Linus Walleij <linus.walleij@linaro.org>,
        Tasanakorn Phaipool <tasanakorn@gmail.com>,
        Sheng-Yuan Huang <syhuang3@nuvoton.com>,
        Kuan-Wei Ho <cwho@nuvoton.com>
Subject: Re: [PATCH 1/1] gpio: nct6116d: add new driver for several Nuvoton
 super io chips
Message-ID: <20220704150842.4e56179b@md1za8fc.ad001.siemens.net>
In-Reply-To: <CAHp75VcoN6eekZXPK8Kpw4aaJN7jfirnUH+1Q0JTEyLSKwrB0w@mail.gmail.com>
References: <20220701091412.20718-1-henning.schild@siemens.com>
        <20220701091412.20718-2-henning.schild@siemens.com>
        <CAHp75VcoN6eekZXPK8Kpw4aaJN7jfirnUH+1Q0JTEyLSKwrB0w@mail.gmail.com>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AgaMGpVfdGDKFAfS3nOYYnPfZmch0CZMzJ5c+6VYVZfHtUIIWzeXcdzhLsba?=
 =?us-ascii?Q?RbJBYY7LfZ+VegCrnmQGNrq4CvwphI79Gr6udJyo3BL1jgMHK/C9DEaHjgeU?=
 =?us-ascii?Q?k7htVgE04Nzcb3QLOq1RJccYcpu2cX1japGym22S9Mv50qF07eJfDuc2zFao?=
 =?us-ascii?Q?snQ6LtSo/GGy2Jk5qwDTQUC1sAre1FhrW0rMacVepj9CkFcKfXpntaXVX4w3?=
 =?us-ascii?Q?xKus+MtncWRjCR5a9joJoBdFQYOqntxtfKOEV3ImL2kVvVIrKrgtAVz1RJmh?=
 =?us-ascii?Q?nJCgDUfYb0lEMCL6yF5QG/EKRG3Z3w2mwIxP+rCohGkJlP547MuOe+hovPyW?=
 =?us-ascii?Q?FgkCBIQEKkNiUEPOYmoamrtaBqWd1KNuy2DWRN/w8mx3ForzXh8iU8sqzpGE?=
 =?us-ascii?Q?39B0aCuXzSNlCKCfzNmbE2a0ImvYAPlI0FbJbPGCbmg+Epjd7U586B1PdNFB?=
 =?us-ascii?Q?NdGK0+XGaMtdJpd1ndUj8yVOEe9jFNA45lVs8tzWnuWwiAtm4T0ZdaWcybf6?=
 =?us-ascii?Q?J/P8GCcvmaCA3udxG5qigK+v3adOLbYs/RuN01n6Xr5ff8twtpYwgtoWFVEs?=
 =?us-ascii?Q?BrcNJtq9K8O0QnCo782RblQC/1njrkXszi46Jo3N5SswD0qmNWhX1u/44HeN?=
 =?us-ascii?Q?8X4AinCQ4+mml0jK2I4LnvVII2MFT2ZA0fn/CgDTQcV/Dy0eZd4lbDupqcVt?=
 =?us-ascii?Q?MI2MmmjWWsTIaG1fzNTejJrkRWn7BrErRuLGZAIB6umtkiAdXVj+8GrN/12u?=
 =?us-ascii?Q?MofGitQ533GJONdq7wF5q+s6hBeKSOR2mtJ8qZxyp5eCKKBumlZZ+Z037BMw?=
 =?us-ascii?Q?D5ZTPPePTxwp07tJgYN4TyzeVLa0im/r8ZVUwg5+mNjdkGlKafEe7k709I5w?=
 =?us-ascii?Q?YDVqG5n3It7CJfWZQni2/0rD9/XvUFdp1TZwTB8DPngvQTV/oO9QLKa+IztS?=
 =?us-ascii?Q?Wn9B3b5su0rdIuSM2QQwZwW70bP927NOOq5smOm3vT3wtdEVFM/BHENss94k?=
 =?us-ascii?Q?vkecFB1PwXic0eS119I0ewnCCkn2Ri6pNSsYLtfpy6mPcHNpwBbREnt8M/Z+?=
 =?us-ascii?Q?VmFkQ0+I9wQ2yxqtcWkRQjF+ujaRvtlPUeC6Isn1iRXv8FzVPz1F9CsTxWVL?=
 =?us-ascii?Q?w8JnkvK4jd9NqPZ5MEInOz2WO8YOvwq2IURCPQwanKVnIeyDDZfQzeIdfASK?=
 =?us-ascii?Q?tqQ+scTAHIR9VMZF0oP1Sjw4Jj4lsHOOT9dtJfgyaU4em7U64WLqSj/BZ1zC?=
 =?us-ascii?Q?vAWHzxZbcU4LWOXy3uenS1jmy7ydzGG93403FGuUYDuQ8giDAfXa6VaIsloK?=
 =?us-ascii?Q?4pdANGbfjlLk3mUyEeZ4n6pSXSKyKRpjduYLOTVt4UI++Z4gzdC2YvM527kn?=
 =?us-ascii?Q?cL0C9qRZHwrVrnvWdK4bZVtstK7h2PqsqMcit7NwztEsthxxYjBgVyCKeOvK?=
 =?us-ascii?Q?U1kQ7b4T9nYgPZpXcggt0XENL4+bLMbgro3ZcW8oUbJpEJx2EwaKhFt/kMaw?=
 =?us-ascii?Q?F+qQ1649TWrE1uZww9lqJ3PqSCkmqN1n38EXjRorGd2bdNUlheBSB1Oz4zfD?=
 =?us-ascii?Q?+NT6tLXFb9xxOP5TQ4vsRy/mzxxm5BoyAtHsO3EZ5dkdqQpXFlYZXpZum5So?=
 =?us-ascii?Q?cQ=3D=3D?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ecbec4f2-edc1-4622-97bd-08da5dbe5e03
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5780.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jul 2022 13:09:03.1777
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 38ae3bcd-9579-4fd4-adda-b42e1495d55a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 8p02ck2Zfp5GR7kvtIghSlT4L8gSgv6lSS2g67EuQzRhpdjFoz0e9mnRf8E1N07LG3TelYiHzA5eJkF6WY0NjuHwlHz3VUcDQp93hIGKJrA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3PR10MB4221
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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