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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e71a3cd-dc40-c3c3-8fe7-5c95096cc462@amlogic.com>
Date:   Mon, 17 Jul 2023 15:24:54 +0800
From:   Huqiang Qin <huqiang.qin@...ogic.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, neil.armstrong@...aro.org,
        khilman@...libre.com, jbrunet@...libre.com,
        martin.blumenstingl@...glemail.com, brgl@...ev.pl, andy@...nel.org,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 2/2] pinctrl: Add driver support for Amlogic C3 SoCs

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@...ogic.com>
>> ---
>>
>> 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