Re: [PATCH v3 2/3] pinctrl: meson: Add driver support for Amlogic A4 SoCs

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Christophe,
     Thanks for your advice. It will be added in the next version.

On 2024/10/18 23:51, Christophe JAILLET wrote:
> [你通常不会收到来自 christophe.jaillet@wanadoo.fr 的电子邮件。请访问 
> https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]
> 
> [ EXTERNAL EMAIL ]
> 
> Le 18/10/2024 à 10:10, Xianwei Zhao via B4 Relay a écrit :
>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>
>> Add a new pinctrl driver for Amlogic A4 SoCs which share
>> the same register layout as the previous Amlogic S4.
>>
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> ---
>>   drivers/pinctrl/meson/Kconfig              |    6 +
>>   drivers/pinctrl/meson/Makefile             |    1 +
>>   drivers/pinctrl/meson/pinctrl-amlogic-a4.c | 1253 
>> ++++++++++++++++++++++++++++
>>   3 files changed, 1260 insertions(+)
> 
> Hi,
> 
> a few nitpicks below.
> 
> ...
> 
>> +
>> +static struct meson_pmx_group a4_periphs_groups[] = {
> 
> I think that struct meson_pmx_group could be const.
> (same for a4_aobus_groups above)
> 
>> +     /* func0 as GPIO */
>> +     GPIO_GROUP(GPIOE_0),
>> +     GPIO_GROUP(GPIOE_1),
>> +
> 
> ...
> 
>> +static struct meson_pmx_func a4_periphs_functions[] = {
> 
> I think that struct meson_pmx_func could be const.
> (a4_aobus_functions above as well)
> 
>> +     FUNCTION(gpio_periphs),
>> +     FUNCTION(uart_a),
>> +     FUNCTION(uart_b),
>> +     FUNCTION(uart_d),
>> +     FUNCTION(uart_e),
>> +     FUNCTION(i2c0),
>> +     FUNCTION(i2c1),
>> +     FUNCTION(i2c2),
>> +     FUNCTION(i2c3),
>> +     FUNCTION(pwm_a),
>> +     FUNCTION(pwm_b),
>> +     FUNCTION(pwm_c),
>> +     FUNCTION(pwm_d),
>> +     FUNCTION(pwm_e),
>> +     FUNCTION(pwm_f),
>> +     FUNCTION(pwm_g),
>> +     FUNCTION(pwm_h),
>> +     FUNCTION(remote_out),
>> +     FUNCTION(remote_in),
>> +     FUNCTION(dcon_led),
>> +     FUNCTION(spinf),
>> +     FUNCTION(lcd),
>> +     FUNCTION(jtag_1),
>> +     FUNCTION(gen_clk),
>> +     FUNCTION(clk12_24),
>> +     FUNCTION(emmc),
>> +     FUNCTION(nand),
>> +     FUNCTION(spi_a),
>> +     FUNCTION(spi_b),
>> +     FUNCTION(pdm),
>> +     FUNCTION(sdio),
>> +     FUNCTION(eth),
>> +     FUNCTION(mic_mute),
>> +     FUNCTION(mclk),
>> +     FUNCTION(tdm),
>> +     FUNCTION(spdif_in),
>> +     FUNCTION(spdif_out)
>> +};
>> +
>> +static struct meson_bank a4_periphs_banks[] = {
> 
> I think that both struct meson_bank could be const.
> 
>> +     /* name  first  last  irq  pullen  pull  dir  out  in */
>> +     BANK_DS("E",  GPIOE_0,  GPIOE_1,  14,  15,
>> +             0x43,  0, 0x44,  0, 0x42,  0, 0x41,  0, 0x40,  0, 0x47,  
>> 0),
>> +     BANK_DS("D",  GPIOD_0, GPIOD_15,  16, 31,
>> +             0x33,  0, 0x34,  0, 0x32,  0, 0x31,  0, 0x30,  0, 0x37,  
>> 0),
>> +     BANK_DS("B",  GPIOB_0, GPIOB_13, 0, 13,
>> +             0x63,  0, 0x64,  0, 0x62,  0, 0x61,  0, 0x60,  0, 0x67,  
>> 0),
>> +     BANK_DS("X",  GPIOX_0, GPIOX_17, 55, 72,
>> +             0x13,  0, 0x14,  0, 0x12,  0, 0x11,  0, 0x10,  0, 0x17,  
>> 0),
>> +     BANK_DS("T",  GPIOT_0, GPIOT_22, 32, 54,
>> +             0x23,  0, 0x24,  0, 0x22,  0, 0x21,  0, 0x20,  0, 0x27,  
>> 0),
>> +};
>> +
>> +static struct meson_bank a4_aobus_banks[] = {
>> +     BANK_DS("AO", GPIOAO_0, GPIOAO_6,  0,  6,
>> +             0x3,   0,  0x4,  0,   0x2,  0,  0x1,  0,  0x0,  0,  0x7, 
>> 0),
>> +     BANK_DS("TEST_N", GPIO_TEST_N,    GPIO_TEST_N,   7, 7,
>> +             0x13,  0,  0x14,  0,  0x12, 0,  0x11,  0, 0x10, 0, 0x17, 
>> 0),
>> +};
>> +
>> +static struct meson_pmx_bank a4_periphs_pmx_banks[] = {
> 
> I think that both struct meson_pmx_bank could be const.
> 
>> +     /* name  first  lask  reg  offset */
>> +     BANK_PMX("E",  GPIOE_0,  GPIOE_1, 0x12,  0),
>> +     BANK_PMX("D",  GPIOD_0, GPIOD_15, 0x10,  0),
>> +     BANK_PMX("B",  GPIOB_0, GPIOB_13, 0x00,  0),
>> +     BANK_PMX("X",  GPIOX_0, GPIOX_17, 0x03,  0),
>> +     BANK_PMX("T",  GPIOT_0, GPIOT_22, 0x0b,  0),
>> +};
>> +
>> +static struct meson_pmx_bank a4_aobus_pmx_banks[] = {
>> +     BANK_PMX("AO", GPIOAO_0, GPIOAO_6, 0x00,  0),
>> +     BANK_PMX("TEST_N", GPIO_TEST_N, GPIO_TEST_N, 0x0,  28),
>> +};
>> +
>> +static struct meson_axg_pmx_data a4_periphs_pmx_banks_data = {
> 
> I think that both struct meson_axg_pmx_data could be const.
> 
>> +     .pmx_banks      = a4_periphs_pmx_banks,
>> +     .num_pmx_banks  = ARRAY_SIZE(a4_periphs_pmx_banks),
>> +};
>> +
>> +static struct meson_axg_pmx_data a4_aobus_pmx_banks_data = {
>> +     .pmx_banks      = a4_aobus_pmx_banks,
>> +     .num_pmx_banks  = ARRAY_SIZE(a4_aobus_pmx_banks),
>> +};
>> +
>> +static struct meson_pinctrl_data a4_periphs_pinctrl_data = {
> 
> I think that both struct meson_pinctrl_data could be const.
> 
>> +     .name           = "periphs-banks",
>> +     .pins           = a4_periphs_pins,
>> +     .groups         = a4_periphs_groups,
>> +     .funcs          = a4_periphs_functions,
>> +     .banks          = a4_periphs_banks,
>> +     .num_pins       = ARRAY_SIZE(a4_periphs_pins),
>> +     .num_groups     = ARRAY_SIZE(a4_periphs_groups),
>> +     .num_funcs      = ARRAY_SIZE(a4_periphs_functions),
>> +     .num_banks      = ARRAY_SIZE(a4_periphs_banks),
>> +     .pmx_ops        = &meson_axg_pmx_ops,
>> +     .pmx_data       = &a4_periphs_pmx_banks_data,
>> +     .parse_dt       = &meson_a1_parse_dt_extra,
>> +};
>> +
>> +static struct meson_pinctrl_data a4_aobus_pinctrl_data = {
>> +     .name           = "aobus-banks",
>> +     .pins           = a4_aobus_pins,
>> +     .groups         = a4_aobus_groups,
>> +     .funcs          = a4_aobus_functions,
>> +     .banks          = a4_aobus_banks,
>> +     .num_pins       = ARRAY_SIZE(a4_aobus_pins),
>> +     .num_groups     = ARRAY_SIZE(a4_aobus_groups),
>> +     .num_funcs      = ARRAY_SIZE(a4_aobus_functions),
>> +     .num_banks      = ARRAY_SIZE(a4_aobus_banks),
>> +     .pmx_ops        = &meson_axg_pmx_ops,
>> +     .pmx_data       = &a4_aobus_pmx_banks_data,
>> +     .parse_dt       = &meson_a1_parse_dt_extra,
>> +};
>> +
>> +static const struct of_device_id a4_pinctrl_dt_match[] = {
>> +     {
>> +             .compatible = "amlogic,a4-periphs-pinctrl",
>> +             .data = &a4_periphs_pinctrl_data,
>> +     },
>> +     {
>> +             .compatible = "amlogic,a4-aobus-pinctrl",
>> +             .data = &a4_aobus_pinctrl_data,
>> +     },
>> +     { },
> 
> Usually, there is no extra "," after a terinator item.
> 
>> +};
>> +MODULE_DEVICE_TABLE(of, a4_pinctrl_dt_match);
>> +
>> +static struct platform_driver a4_pinctrl_driver = {
>> +     .probe  = meson_pinctrl_probe,
>> +     .driver = {
>> +             .name   = "amlogic-a4-pinctrl",
>> +             .of_match_table = a4_pinctrl_dt_match,
>> +     },
>> +};
> 
> ...
> 
> CJ
> 

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