Re: [PATCH V3 2/2] pinctrl: Add driver support for Amlogic C3 SoCs

[Huqiang Qin]

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