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

From: Huqiang Qin <huqiang.qin@amlogic.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linus.walleij@linaro.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	neil.armstrong@linaro.org, khilman@baylibre.com,
	jbrunet@baylibre.com, martin.blumenstingl@googlemail.com,
	brgl@bgdev.pl, andy@kernel.org, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 2/2] pinctrl: Add driver support for Amlogic C3 SoCs
Date: Mon, 10 Jul 2023 17:32:26 +0800	[thread overview]
Message-ID: <6afe63d3-5cfe-1cfe-0326-0c51c6a526ef@amlogic.com> (raw)
In-Reply-To: <CAHp75VevmTiv-OYVTn3bF_8UT9SKNU2oTrzS-AM1zoWefeJ=3w@mail.gmail.com>

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

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