Hi Linus Walleij, Thank you for your reply. On 2023/7/17 5:20, Linus Walleij wrote: >> Add a new pinctrl driver for Amlogic C3 SoCs which share >> the same register layout as the previous Amloigc S4. > > How is the spelling of amlogic there in the end. > Okay, thanks for pointing out the problem, I'll correct it. >> Signed-off-by: Huqiang Qin <huqiang.qin@xxxxxxxxxxx> >> --- >> >> V1 -> V2: >> Added a comma to the last item of the array and a period to >> the commit message. > > Andy had more comments about the header inclusion. Please > include all used headers directly as requested, I think it's a good > idea and avoids confusing compile problems. Yes, this is a good idea, but when many files must include the same batch of header files (seems to have become a kind of template), it seems like a good choice for us to put them all in pinctrl-meson.h (actually been doing this a long time ago). Attached is my last reply to Andy: > 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. > If you still think it needs to be changed, I will add them in the next version patch. Thank you, looking forward to your reply. Best Regards, Huqiang Qin
