linux-kernel - Re: [PATCH v4 03/15] pinctrl: bcm: add bcm63xx base code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Mar 2021 12:43:57 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Álvaro Fernández Rojas <noltari@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
        Jonas Gorski <jonas.gorski@...il.com>,
        Necip Fazil Yildiran <fazilyildiran@...il.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 03/15] pinctrl: bcm: add bcm63xx base code

On Thu, Mar 4, 2021 at 10:57 AM Álvaro Fernández Rojas
<noltari@...il.com> wrote:
>
> Add a helper for registering BCM63XX pin controllers.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
> Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>

This SoB is in a strange place.

The order is wrong taking into account the From header (committer). So,
it's not clear who is the author, who is a co-developer, and who is
the committer (one person may utilize few roles).
Check for the rest of the series as well (basically this is the rule
of thumb to recheck entire code for the comment you have got at any
single place of it).

...

> +static const struct of_device_id bcm63xx_gpio_of_match[] = {
> +       { .compatible = "brcm,bcm6318-gpio", },
> +       { .compatible = "brcm,bcm6328-gpio", },
> +       { .compatible = "brcm,bcm6358-gpio", },
> +       { .compatible = "brcm,bcm6362-gpio", },
> +       { .compatible = "brcm,bcm6368-gpio", },
> +       { .compatible = "brcm,bcm63268-gpio", },

> +       { /* sentinel */ },

Comma is not needed in terminator line

> +};

...

> +       dev_info(dev, "registered\n");

Unneeded noise.

...

> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/regmap.h>

The rule of thumb is to include only the headers that the below code
is direct user of.

The above are not used anyhow, while missed types.h and several
forward declarations.

> +#include "../core.h"

Seems the same.

> +#define BCM63XX_BANK_GPIOS 32
> +
> +struct bcm63xx_pinctrl_soc {
> +       struct pinctrl_ops *pctl_ops;
> +       struct pinmux_ops *pmx_ops;
> +
> +       const struct pinctrl_pin_desc *pins;
> +       unsigned npins;
> +
> +       unsigned int ngpios;
> +};
> +
> +struct bcm63xx_pinctrl {
> +       struct device *dev;
> +       struct regmap *regs;
> +
> +       struct pinctrl_desc pctl_desc;
> +       struct pinctrl_dev *pctl_dev;
> +
> +       void *driver_data;
> +};


--
With Best Regards,
Andy Shevchenko