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:   Fri, 1 Jul 2022 13:12:33 +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: <20220701131233.68b0f507@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?0sa8OVQDf8hWMP1HYRnvhtt6EPfPxIM5PqWAIr1ckylb/mxOhgtxHDnusmGE?=
 =?us-ascii?Q?hK8hjcICt7kXN61YV0Uuf3ATvU2hBfaFlLRmFizQXEBOpWY4OcPlt1c15lK8?=
 =?us-ascii?Q?ISKaIhko6PtvX3R8OU/AgLxyl9Iv364PyGNlHKOUYFJ+68oINwiojHSbnKwd?=
 =?us-ascii?Q?u0sMSmGWy1F/WhMOfZpnEMRRIQNq/S9J0/0o+n8eEhsjlVscr3kAHznpAhpX?=
 =?us-ascii?Q?loi1k3Qw912SelXimKtBReXdFBivJtz7eRiHrpFWlQDmlgrUsFEKRiuZIdf1?=
 =?us-ascii?Q?RR5pKPTBqLpsrE2cuewqswlQyeltue5TtnbrtUbYCA1WIN4lfAZsyBFKhmVN?=
 =?us-ascii?Q?iAhpXAee3ftW+hF84kfUQ2lSGlV+uQN74ekDlFWk0PAdAhqiCmZthKaC+A8c?=
 =?us-ascii?Q?+NTheHB6VhWmwbHthsb2V89gDkOGFjO7K25mN6E6Xtpl4GCnGyDEKkI+y6l+?=
 =?us-ascii?Q?z9CQXALQ0ojBu2nqn0a4yd2YuA8wuVUYlMwxsi56r3ejEwzVy8CSP+PUOneo?=
 =?us-ascii?Q?1CHeRfcVHYfqHUnq7sKquAgYdqyAxBpNzshz1D4PDVW7iSz68u8ozKqIavAE?=
 =?us-ascii?Q?AXdN/d2ekwThxZ3g8WtRYB4mTlSLczfHg1VwuGlCwI2Pq2GTW9TdrKkqrWeP?=
 =?us-ascii?Q?9wguuC1aspF0yVc65d4KZVm+d0YjvjnQBi9q1lX9tAZfAoClG8tgnH1N/DIB?=
 =?us-ascii?Q?7qYS2jugxr7FxZgpLZIkTWwi25Z8UObgmmoK3649dSuYZN2cJBeTN7kwajGo?=
 =?us-ascii?Q?rxRSmff/u5q6SVVbNc3TEa+APBo6OxSCzkE+PdI27zn+6vovO/C5vwP9UtkW?=
 =?us-ascii?Q?uHpIPKZkdwm9rAG/irBE0PyEDdzhlH1ql3RBML+1nblVLAq8OtO4eROjimLf?=
 =?us-ascii?Q?BKzd9tp4Sn5XxVfzwogY8casPnYNpImvvZyjveKGecpPLdHiDlgD0e16pAtu?=
 =?us-ascii?Q?nUF9lRwXPRWRxxfd8txqWfdI3WNf/9lulW3ZkGJk7YWKmMSWdTEpFXN33URf?=
 =?us-ascii?Q?8WX+mWuSbJ5LmWNJXeo5zC60lBmVZxjpEiLJnSONIUdeGoNB81vIC7X7kVWQ?=
 =?us-ascii?Q?hd5JVDZ0Vu+AFJON8hbkccJ4D/LuIhfN1bSjCpyt/d9tCxMY8+EzuBPO4qra?=
 =?us-ascii?Q?JoSiM2cq2mzE9L38DSb3FVSYI63JQMyIBxN6OdhFI2Z+4Hq4qeS6odXjmO1k?=
 =?us-ascii?Q?igh1Fir8B5Jcq6C4Dnt7rmD7GfBwfsmWdM8kMk8F1sVZxiNCIgHKlXJXmnTf?=
 =?us-ascii?Q?tUo1oQyQJhV4zEFzTdIfKAmJ9RYBO8NwGVhmzgptJfJyAaQV2GwPlYGmv8zB?=
 =?us-ascii?Q?Tp8sGAToY/FScNdLw7bTFZoLO5nXnKOyychepFIWjOdwZHnMSUm7efBp+MSv?=
 =?us-ascii?Q?2Fe1QtUbUNz7bSIGMOe5mKl2bUYjicACkVxEL5/edWuwCKv0YoAqYhJarBS2?=
 =?us-ascii?Q?YqPLqp9bFw/BshTo7UoIWO8FaVMSexP1yQxv1WJ+GFSz+ciRz8hxQObjXtbf?=
 =?us-ascii?Q?VnfZMGMCxghu+GxCrzkdSY5yWl1Cgm5E/1QV6/qFqdZUn0wvtqULLplqNgsr?=
 =?us-ascii?Q?o/1Hvj4gnNxnbnJUQCxP174ZesOqOvlYncsWNhBCQNhndD8hJeajL2wJQ+oc?=
 =?us-ascii?Q?Vg=3D=3D?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f1f1abee-b589-472c-c380-08da5b529a7d
X-MS-Exchange-CrossTenant-AuthSource: PA4PR10MB5780.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jul 2022 11:12:36.4832
 (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: 4sxZeANAh9SqcGZDziytmKs+FpeGdLV93lKPQyAwH8jKqIJ0PQ6ZiEDwQ1CBRLaC90TQOXxM/iWkvKKHL9vWfBbNjjh5Hs3OYVh1Jwro3jc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR10MB3386
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?

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