MIME-Version: 1.0
References: <20211214022100.14841-1-qianggui.song@amlogic.com> <20211214022100.14841-4-qianggui.song@amlogic.com>
In-Reply-To: <20211214022100.14841-4-qianggui.song@amlogic.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 14 Dec 2021 13:57:06 +0200
Message-ID: <CAHp75VczRGaKY_O3SV9xx-G95Rk8uFPwNFuVE1iy+HAxgL+oVg@mail.gmail.com>
Subject: Re: [PATCH 3/3] pinctrl: meson: add pinctrl driver support for
 Meson-S4 Soc
To:     Qianggui Song <qianggui.song@amlogic.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        linux-amlogic <linux-amlogic@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Dec 14, 2021 at 6:27 AM Qianggui Song <qianggui.song@amlogic.com> wrote:
>
> Add new pinctrl driver for Amlogic's Meson-S4 SoC which share the
> same register laytout as the previous Meson-A1.

layout

...

> +config PINCTRL_MESON_S4
> +       tristate "Meson s4 Soc pinctrl driver"
> +       depends on ARM64
> +       select PINCTRL_MESON_AXG_PMX

> +       default y

Why is it needed on other (non-MESON_S4) SoCs?

...

> +static const char * const i2c1_groups[] = {
> +       "i2c1_sda_c", "i2c1_scl_c",
> +       "i2c1_sda_d", "i2c1_scl_d",
> +       "i2c1_sda_h", "i2c1_scl_h",
> +       "i2c1_sda_x", "i2c1_scl_x"

In this and all the rest similar cases leave a comma.

> +};

...

> +static const struct of_device_id meson_s4_pinctrl_dt_match[] = {
> +       {
> +               .compatible = "amlogic,meson-s4-periphs-pinctrl",
> +               .data = &meson_s4_periphs_pinctrl_data,
> +       },

> +       { },

When it's a terminator entry, no comma is needed.

> +};

...

> +static struct platform_driver meson_s4_pinctrl_driver = {
> +       .probe  = meson_pinctrl_probe,
> +       .driver = {
> +               .name   = "meson-s4-pinctrl",
> +               .of_match_table = meson_s4_pinctrl_dt_match,
> +       },
> +};

> +
> +module_platform_driver(meson_s4_pinctrl_driver);

Swap above two lines, first module_...() followed by a blank line.

> +MODULE_LICENSE("Dual BSD/GPL");

-- 
With Best Regards,
Andy Shevchenko