Hi Andy Shevchenko,
On 2023/7/6 20:29, Andy Shevchenko wrote:
>
> On Thu, Jul 06, 2023 at 07:45:22PM +0800, Huqiang Qin wrote:
>> Add new pinctrl driver for Amlogic C3 SoCs which share the
>
> a new
Okay
>
>> same register layout as the previous Amloigc S4
>
> Missing period at the end.
Okay
>
> ...
>
>> +config PINCTRL_AMLOGIC_C3
>> + tristate "Amlogic C3 SoC pinctrl driver"
>> + depends on ARM64
>> + select PINCTRL_MESON_AXG_PMX
>> + default y
>
> This default seems a bad cargo cult. Why ARM64 kernel should have all them be
> opt-out, instead of opt-in? Shouldn't this be a distro problem?
The Kconfig structure is as follows:
menuconfig PINCTRL_MESON
tristate "Amlogic SoC pinctrl drivers"
depends on ARCH_MESON || COMPILE_TEST
...
if PINCTRL_MESON
...
config PINCTRL_AMLOGIC_C3
tristate "Amlogic C3 SoC pinctrl driver"
depends on ARM64
select PINCTRL_MESON_AXG_PMX
default y
endif
When ARCH_MESON is not selected, all pinctrl drivers of Amlogic will not be compiled by default
>
> ...
>
>> + MESON_PIN(GPIO_TEST_N)
>
> Is it real one?
Yes, it's real GPIO and supports interrupts, similar to Amlogic S4 SoC
>
>> +};
>
> ...
>
>> + /* Bank A func6 */
>> + GROUP(spi_a_mosi_a, 6),
>> + GROUP(gen_clk_a4, 6),
>> + GROUP(clk12_24_a, 6)
>
> Leave trailing comma here as it's not a terminator.
Okay
>
> ...
>
>> +static const char * const i2c2_groups[] = {
>> + "i2c2_sda", "i2c2_scl"
>
> Ditto.
Okay
>
>> +};
>> +
>> +static const char * const i2c3_groups[] = {
>> + "i2c3_sda_c", "i2c3_scl_c",
>> + "i2c3_sda_x", "i2c3_scl_x",
>> + "i2c3_sda_d", "i2c3_scl_d"
>
> Ditto.
Okay
>
>> +};
>
> ...
>
>> +#ifdef CONFIG_OF
>
> Drop this ugly ifdeffery.
Okay
>
>> +static const struct of_device_id c3_pinctrl_dt_match[] = {
>> + {
>> + .compatible = "amlogic,c3-periphs-pinctrl",
>> + .data = &c3_periphs_pinctrl_data,
>> + },
>> + { },
>
> No comma for the terminator.
Okay
>
>> +};
>> +MODULE_DEVICE_TABLE(of, c3_pinctrl_dt_match);
>> +#endif /* CONFIG_OF */
>> +
>> +static struct platform_driver c3_pinctrl_driver = {
>> + .probe = meson_pinctrl_probe,
>> + .driver = {
>> + .name = "amlogic-c3-pinctrl",
>
>> + .of_match_table = of_match_ptr(c3_pinctrl_dt_match),
>
> Drop the rather problematic of_match_ptr().
Okay
>
>> + },
>> +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
Best Regards,
Huqiang Qin
