Hi Andy Shevchenko,
On 2023/7/10 15:33, Andy Shevchenko wrote:
> ...
>
>> +#include <dt-bindings/gpio/amlogic-c3-gpio.h>
>
> Seems some headers are missing. The rule of thumb is to include
> headers the code uses directly.
> Moreover, using a "proxy" header is not the best idea.
>
> kernel.h // ARRAY_SIZE()
> mod_devicetable.h // of_device_id
> module.h
> platform_device.h
>
> pinctrl/pinctrl.h
>
>> +#include "pinctrl-meson.h"
>> +#include "pinctrl-meson-axg-pmx.h"
>
> With the above, it might be that existing inclusions become unused, so
> drop them in such a case.
According to Amlogic's pinctrl driver structure, the code to realize
the function is mainly in pinctrl-meson.c, and pinctrl-amlogic-c3.c
is only used as a data file to describe the pins of the chip, and for
similar data files, all need to include pinctrl-meson.h and
pinctrl-meson-axg-pmx.h header files, such as A1, G12A, S4 and so on.
The following header files are included in pinctrl-meson.h:
linux/gpio/driver.h
linux/pinctrl/pinctrl.h
linux/platform_device.h
linux/regmap.h
linux/types.h
linux/module.h
Here is a case for each dependent header file:
struct gpio_chip chip; ---- linux/gpio/driver.h
struct pinctrl_desc desc; ---- linux/pinctrl/pinctrl.h
struct platform_device *pdev; ---- linux/platform_device.h
struct regmap *reg_mux; ---- linux/regmap.h
Basic types of linux ---- linux/types.h
MODULE_DEVICE_TABLE() ---- linux/module.h
Since every data file will use them, let's put it in the header file,
so as to reduce duplication of code.
> ...
>
>> +static struct meson_pmx_func c3_periphs_functions[] = {
>> + FUNCTION(gpio_periphs),
>> + FUNCTION(uart_a),
>> + FUNCTION(uart_b),
>> + FUNCTION(uart_c),
>> + FUNCTION(uart_d),
>> + FUNCTION(uart_e),
>> + FUNCTION(i2c0),
>> + FUNCTION(i2c1),
>> + FUNCTION(i2c2),
>> + FUNCTION(i2c3),
>> + FUNCTION(i2c_slave),
>> + 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(pwm_i),
>> + FUNCTION(pwm_j),
>> + FUNCTION(pwm_k),
>> + FUNCTION(pwm_l),
>> + FUNCTION(pwm_m),
>> + FUNCTION(pwm_n),
>> + FUNCTION(pwm_c_hiz),
>> + FUNCTION(ir_out),
>> + FUNCTION(ir_in),
>> + FUNCTION(jtag_a),
>> + FUNCTION(jtag_b),
>> + FUNCTION(gen_clk),
>> + FUNCTION(clk12_24),
>> + FUNCTION(clk_32k_in),
>> + FUNCTION(emmc),
>> + FUNCTION(nand),
>> + FUNCTION(spif),
>> + FUNCTION(spi_a),
>> + FUNCTION(spi_b),
>> + FUNCTION(sdcard),
>> + FUNCTION(sdio),
>> + FUNCTION(pdm),
>> + FUNCTION(eth),
>> + FUNCTION(mclk_0),
>> + FUNCTION(mclk_1),
>> + FUNCTION(tdm),
>> + FUNCTION(lcd)
>
> + trailing comma. The rule of thumb is to add a comma when it's not a
> terminator entry.
Okay, I'll double check and add them.
Best Regards,
Huqiang Qin
