Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6afe63d3-5cfe-1cfe-0326-0c51c6a526ef@amlogic.com>
Date:   Mon, 10 Jul 2023 17:32:26 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH V2 2/2] pinctrl: Add driver support for Amlogic C3 SoCs
Content-Language: en-US
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
References: <20230710042812.2007928-1-huqiang.qin@amlogic.com>
 <20230710042812.2007928-3-huqiang.qin@amlogic.com>
 <CAHp75VevmTiv-OYVTn3bF_8UT9SKNU2oTrzS-AM1zoWefeJ=3w@mail.gmail.com>
From:   Huqiang Qin <huqiang.qin@amlogic.com>
In-Reply-To: <CAHp75VevmTiv-OYVTn3bF_8UT9SKNU2oTrzS-AM1zoWefeJ=3w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WGcyMUcwbUZuSTFkRy9tU0tNQm1zRUpZMWlkTTFiSU9ibVNJL2c1aGprMFFv?=
 =?utf-8?B?VTZ2ZmZRb2tFV0luMW9Memg4S09vSlBNM3k5c2ZYeHBGMEplQ3pNY01TYVE5?=
 =?utf-8?B?VStwUGg1Y2Vnd0N6MXJzN1VSZkFkakdibldsZzJES0h0N05BM1F4cnBCaUdJ?=
 =?utf-8?B?R3RoaXd1dXdFS2l0S0hoRGx0L01hNjBNVzM3djhWSFJhZ3NVMlhqUmVzTDZE?=
 =?utf-8?B?bnJSTWhOekQxZFk3V0ZlZERhY3Bqem5YUEFKc2J6WWZuQUJqNkdVbDNZdDNH?=
 =?utf-8?B?NWxZbnIvSWNyR3ZqWFczTG8wY1ZVRE14eC93Ni9wa3NCMFgza29RNUMwaGEx?=
 =?utf-8?B?WjZBWWlhclQ2VmFBVGc2RWdoMU5VWjJIam1zOFFxTXRXdEZzeTc5cE5DdVpv?=
 =?utf-8?B?RjNWSXcyQW1RUWpPVjEyVVNuWWdtTHNKSjRwNUJGN1VaangvUTQ4R0pjV29N?=
 =?utf-8?B?ZzN3SjJpNHVjc29ONE9jUVQ5b015MCtDVGVOZnRydjQ0M3B2cTdvS21MK0lB?=
 =?utf-8?B?Tm9tSnBlR0dGWHdMQzNGU3h5bmNSc3Z1UlB1bTA1UTVCdkJTeTdjSVNzZFQz?=
 =?utf-8?B?NHIrSi9GTk9WMTlIVmtYeml4VmlEbjZFdHd1VFA0a0FkVmpuWlpIby84d28w?=
 =?utf-8?B?SUt5QXF4RkxmV2IvYkdzYjB5RFdBb3FkWHBTZldhR1hrOERmb3RHdW5wYlZB?=
 =?utf-8?B?K21xTnRrUWR4V0wxT1NZMnhRMS9hS2IzNmR0VFVxYi95S3RiY0N3TDZnWEdZ?=
 =?utf-8?B?T0RhREdpbVRDbERzSzFjZ0Y2T2pkL1RHUERaWnZwSWdod3VzS2FpQlRVeUla?=
 =?utf-8?B?TkhDU3dlTTVhaUk1R2RDNm5GTTh0Y0FIZG00TFZvNHBGSk5sQ2VyNkd1b2Rn?=
 =?utf-8?B?NUIrZlFXaDhkTUQrWHp0cjBSQmRtbDlGeHpCTUlvV1lJOXZtUHpuNHA4cUZM?=
 =?utf-8?B?Q2NVNHBVN2M0Vlg3ZTlldFlGNkh5WkRrQlAvUzlXM2l3TnNPUTRyMVIvaHF0?=
 =?utf-8?B?SWlsNFZTSU5WL0Y1d3BjQ0VVM0ZNZWRwb2VYRGlCNEhhU2REbmVheWFiMm9s?=
 =?utf-8?B?QVppWldudEFJSkpCVzJUSnVBMXlsVkFxR0NQT3hvQVN6SGdTUy9YeDFqZlVy?=
 =?utf-8?B?cEJmYnpTNXB6Q1g3SjhFak9QejVEZTRmSW1yZnVUVXRiZEFYemdSbzNtVXZ0?=
 =?utf-8?B?WitwN0owT1drRWQvQTIyMWdwN2MyRTQranZXZktCSDYyNjV5U2Rtall0bVNR?=
 =?utf-8?B?Ulo4aXFwYndoclZFVHFrd1UzL0NTeThobjROc2loblVsRXlwaEc5MWNRcm9C?=
 =?utf-8?B?eUJhT2JVN2diR01ON0hJMkxQbWxtUW9FSnNtNnd0T0Zid2dRWXh5TnRzS0Qv?=
 =?utf-8?B?SzdFVnE2aDhYcHFLa0duWkNHV3NMMDQwNENmdFVZbkMxSUI3eDFuaFJlNE94?=
 =?utf-8?B?Qm93ZVRSMW1vY0dsT3pSTjhYWWVWNmhQS25ESHNGZmEvenVITkNnMmNlUUdw?=
 =?utf-8?B?UzhvcFMrMzFCVW1Oei9zaThWakZNdUVRcEFFMERqY3NPWnhsQlBqZWJvNDJz?=
 =?utf-8?B?c2hORklmTWU1UmdDOWNOZnFsK21rVWpmM0hpKzYwQUJ0aFFpRkVodEtVNldY?=
 =?utf-8?B?Wk03U3hRbWxMbndtUWtaSjdSYU5meFgzTGFBMGp2aDdKaWpDT0t1UWpBeGNB?=
 =?utf-8?B?Uml4R3pmc3ppQjEvV0oyRDNMbDg5OGtKcUFkdm9CUkdUYXJtNHlId1hOdDgy?=
 =?utf-8?B?ZGJVNGtDQlp5VlozYlhSOVMvWjJWSHZ0YU5CellVZHJkdkZUT1hqdEtJL0NN?=
 =?utf-8?B?UjhVOWN6S0RwTXRkWTlONUxiMHlNeWU5dTVZczZDSlVnK0RQWEY3RkthS25F?=
 =?utf-8?B?YVV0MTY3VHgyeDBCQm14WnJFRkNJZUV1dEFqOXBRTlkwMjNhUGUweVVsNm1m?=
 =?utf-8?B?YXVSVG9aazNrK1p3QTBsK0JOcVdqeEZNdFBxdnZSZ3RBa0dnenM2YUQxSkJr?=
 =?utf-8?B?TlJvLysrRnRXUzg2eWJqdC9KeTZVU2hod2JGaS83NjRPYm83QlI2Tm8wemdP?=
 =?utf-8?B?akJTR3NWNWVUOEZPMVRTdE05dCthalpoMWNQOThCZSt0N2FmSVc2M1RwM3BS?=
 =?utf-8?B?VzRQSW5wenlXZEx3c2drU1BmazZidVdaMnpaOW55Q3pNRGdicmR1ZmFoeTBr?=
 =?utf-8?B?aVE9PQ==?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc743c7f-9c00-4dc6-7edb-08db81289574
X-MS-Exchange-CrossTenant-AuthSource: KL1PR03MB7221.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jul 2023 09:32:32.3773
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0df2add9-25ca-4b3a-acb4-c99ddf0b1114
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: I0WOSR4I7D2QRT1zlSCcoi1IUppCi6oMJDKl1jUX08GjnBpi3okX6wS5gx0z3b9sIPM+XhP3TEh71z+w8CkULg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TY0PR03MB6630
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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