Re: [PATCH 3/3] pinctrl: meson: add pinctrl driver support for Meson-S4 Soc

[qianggui.song <Qianggui.Song@amlogic.com>]

Hi,Andy

On 2021/12/14 下午7:57, Andy Shevchenko wrote:
> 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
>
> ...
awkward
>
>> +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?
>
> ...

to keep consistent with previous chips configuration, maybe another  
patch set to   remove all default y

for all previous 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.
Okay.
>
>> +};
> ...
>
>> +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.
will fix it  in next patch
>
>> +};
> ...
>
>> +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.
Okay.
>
>> +MODULE_LICENSE("Dual BSD/GPL");

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic