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: <508344ff-6cb7-0658-89ae-c6873079e1b3@amlogic.com>
Date:   Fri, 7 Jul 2023 12:07:59 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 2/2] pinctrl: Add driver support for Amlogic C3 SoCs
To:     Andy Shevchenko <andy@kernel.org>
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, 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: <20230706114522.2490655-1-huqiang.qin@amlogic.com>
 <20230706114522.2490655-3-huqiang.qin@amlogic.com>
 <ZKazwMpmsNzBuWA7@smile.fi.intel.com>
Content-Language: en-US
From:   Huqiang Qin <huqiang.qin@amlogic.com>
In-Reply-To: <ZKazwMpmsNzBuWA7@smile.fi.intel.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?UUxIQ3Fnc214VW93eVJjMkhJeExYNnZhWmpIWTdTRTJBdnliWjQ5R0c1TEE0?=
 =?utf-8?B?b3ZYR0Myd3pzOVF5WGNGVVRXZzB5OC9ZRWJGU1hLelBaUm1PTEg3RW16Nkxk?=
 =?utf-8?B?NVQ4OE90OHIxUndxdUs2Y2U1TFg2Q0VnNzFlcTdHSGZrTzZweE9zdWJQV0xl?=
 =?utf-8?B?TC92NW4vekU0cUZjUmQ2TWtONTNIOU83QTAxSnhYVXBKUWIxWXY5cW9kTHJE?=
 =?utf-8?B?dkQwdXNyaXh5OTk3ZnNCbWY1clRYVUxoWmNZQml5NS9Ib2x4YWFmU2FJTk9C?=
 =?utf-8?B?a0xOQVYzNTdPc0NjMzFlVnVPeVRwZlB4ZG1aWGJaVUJBQS8xZkRkbzZtUS9D?=
 =?utf-8?B?UmJXSzlmWkJ5MDZCVnE5MlkvemhYTHhCWE4yMG9Lbzh3UGROanlFRWhBRXF0?=
 =?utf-8?B?RnpTN2ZHdUc2VzNEUHpiTDhPR3hicVo0STVRcWd0MnJyaSsrTEhWcTladnd1?=
 =?utf-8?B?aVhFdk9hYjJoYjluY1dCQmdzZkhCeXlXRFJKT3AyYlRoQ21xUWxXWTRMTjNa?=
 =?utf-8?B?blJqN0VYZit2NW5WZ2MxR1RGMXZ5T3lKWGg5REovWTF1K0Nnc0dqTzVWY0R3?=
 =?utf-8?B?SXlWM2RMSUpGdU1ITi9rRms0Ync4dUpyRzNOVGJMY2xoTUZDakk2VGpFZzVR?=
 =?utf-8?B?WElVNGl4Zi9YaC9ocHYvNHRNNDJpWHJZTWx5T1loczlaa2xCYjBpYVpXdU16?=
 =?utf-8?B?RG9LQkc3ZkFReG9ldFY0cEJPdzdsQ0hpSnpxRUdvNlpPaTlsdGNCT1lqcE5X?=
 =?utf-8?B?cW55Z0JOWmJYdlIxTk5TTDNkMnorNmxDMmQ2aG9BMFVGWDF6TVdobU5SOXFa?=
 =?utf-8?B?SWNkTVNSeGtlditucVVlREZvVUFDU014cWZsSlAxNnNpMUM0d0NEaVRncVZC?=
 =?utf-8?B?dTZHeDllNkd6YzFzMnIxc0NJeStXUlU5NHFabldVSTRpd0VmNXNsTEdGdnEr?=
 =?utf-8?B?eDAzZU0wM3VQL3UvSDltWGpuZUhDR2tvUTRNSThQK1gxTkhZb1hndzllSEkr?=
 =?utf-8?B?NGMwaDI3TEo1dmdHdVV5OEU5OUw3UTgvb05QbFlTaVZyVll0UEh2TXNYSTBw?=
 =?utf-8?B?d1c5NzVuMGNqd0UzSHpobmtzbXJrUkhnbXpmMzJkK3BzN1ZzRVF2WXpUejBQ?=
 =?utf-8?B?Mkg0eVJMbGRTRlBXenhSOTFTVzZwbXE0SGZDcGdjdHR4aEhNZzgwcUcwbjhU?=
 =?utf-8?B?NG5OOXkyQkZhaVhuWjV5V3g5WkdXbTBOK3FSRE10clJySjZwT1dCNGN4Q2FD?=
 =?utf-8?B?ZTZsYnZNK05BQmdlL0xOQSsrQTk5U2s0YVJLMlYwTTdmKzNEVWFpdW1sMXQ5?=
 =?utf-8?B?SUF0VXd5MTlOTnlqdDhPR1pGTjdUWUxITzlVMUV4aGxSamdnaTIvYm8zTFl6?=
 =?utf-8?B?ejNCNFRCZkNtMFM4M04vQ1ViRyt1UVNwYnI2T0xsMlBST3IzUHREVGVZWmYv?=
 =?utf-8?B?bXUxY3orWkhOcFh2SzVtblZxQUh1bEtvcmg3NlZISk5hRlhnTEpGcWpWSFlt?=
 =?utf-8?B?TEhQZ0IrUEZvQ3ViUk85a2R1QkdZZFVWdHNLR20rS1Vud1QzaDVzUW45dXBG?=
 =?utf-8?B?L1BVcEdDOHh2eDZUUkczZHVmTktwS3lrSVd3aGlubmZZM3VFbHJXdjBrb0xT?=
 =?utf-8?B?bFc1QlFGL2ZnOFMyNHFPd2d1UnlTbGpLam00Szl3TXAvM3JTTWZudVN3RUZz?=
 =?utf-8?B?M3hQOWJKQXNwOWUwYS9MSXZNS01LNWs0cVU3WnM5Sm9uc0VGWDc4UWZXR2hu?=
 =?utf-8?B?VjRGWlRkMHNEYld0U2xDSkMrRS82REpSTGxXUVhLanVSOEVPbmxJTUR3QkpT?=
 =?utf-8?B?MEJnM014Ui9Ja2p2bGRScElrMkZ5aHN2QUVCZTN3Ym1HcGkwVmVQKytmcGxI?=
 =?utf-8?B?SG5zWTdqVjBxdmQ3UjZGZEEwcklXeFhhN2JWL0RwV1ZvblJHVUZmT1E4Vk9z?=
 =?utf-8?B?ZTdsU2E0SkRkNkU2eGFyTFNVMmJ5bktveWFBZmFkd3R6UEJQNmdUVVVLS3lF?=
 =?utf-8?B?TXc3RCtOMWRqaXNtUStYUVFrVjNWTEQvVDkydFNpM1EySi9GVkduODYrZXF2?=
 =?utf-8?B?SG1iMnN3RzJ1ZTVjRWF0bG9SdFN5Wm5PTTl3OXpNbjdqZlNZR2xheU5iQ01r?=
 =?utf-8?B?VkNHZkl5c0lBMUQrSTRKWWVFY3R6c0c2b3FNbUxadVVCN0ZHbVUvOG5wQjRD?=
 =?utf-8?B?N0E9PQ==?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a9204bd0-7a4f-478b-601c-08db7e9fc41f
X-MS-Exchange-CrossTenant-AuthSource: KL1PR03MB7221.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jul 2023 04:08:06.2592
 (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: 3gwKenrljWCQKMO/haa2/UnSD3VN6jZrTZ8R+p26jxldP9T2MkOL5t+7Pg5cTUHOOFu3i8W5pust+9FNzXTTCg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TYZPR03MB5215
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_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED 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/6 20:29, Andy Shevchenko wrote:
> 
> On Thu, Jul 06, 2023 at 07:45:22PM +0800, Huqiang Qin wrote:
>> Add new pinctrl driver for Amlogic C3 SoCs which share the
> 
> a new

Okay

> 
>> same register layout as the previous Amloigc S4
> 
> Missing period at the end.

Okay

> 
> ...
> 
>> +config PINCTRL_AMLOGIC_C3
>> +     tristate "Amlogic C3 SoC pinctrl driver"
>> +     depends on ARM64
>> +     select PINCTRL_MESON_AXG_PMX
>> +     default y
> 
> This default seems a bad cargo cult. Why ARM64 kernel should have all them be
> opt-out, instead of opt-in? Shouldn't this be a distro problem?

The Kconfig structure is as follows:

menuconfig PINCTRL_MESON
	tristate "Amlogic SoC pinctrl drivers"
	depends on ARCH_MESON || COMPILE_TEST
	...

if PINCTRL_MESON
...
config PINCTRL_AMLOGIC_C3
	tristate "Amlogic C3 SoC pinctrl driver"
	depends on ARM64
	select PINCTRL_MESON_AXG_PMX
	default y

endif

When ARCH_MESON is not selected, all pinctrl drivers of Amlogic will not be compiled by default

> 
> ...
> 
>> +     MESON_PIN(GPIO_TEST_N)
> 
> Is it real one?

Yes, it's real GPIO and supports interrupts, similar to Amlogic S4 SoC

> 
>> +};
> 
> ...
> 
>> +     /* Bank A func6 */
>> +     GROUP(spi_a_mosi_a,             6),
>> +     GROUP(gen_clk_a4,               6),
>> +     GROUP(clk12_24_a,               6)
> 
> Leave trailing comma here as it's not a terminator.

Okay

> 
> ...
> 
>> +static const char * const i2c2_groups[] = {
>> +     "i2c2_sda", "i2c2_scl"
> 
> Ditto.

Okay

> 
>> +};
>> +
>> +static const char * const i2c3_groups[] = {
>> +     "i2c3_sda_c", "i2c3_scl_c",
>> +     "i2c3_sda_x", "i2c3_scl_x",
>> +     "i2c3_sda_d", "i2c3_scl_d"
> 
> Ditto.

Okay

> 
>> +};
> 
> ...
> 
>> +#ifdef CONFIG_OF
> 
> Drop this ugly ifdeffery.

Okay

> 
>> +static const struct of_device_id c3_pinctrl_dt_match[] = {
>> +     {
>> +             .compatible = "amlogic,c3-periphs-pinctrl",
>> +             .data = &c3_periphs_pinctrl_data,
>> +     },
>> +     { },
> 
> No comma for the terminator.

Okay

> 
>> +};
>> +MODULE_DEVICE_TABLE(of, c3_pinctrl_dt_match);
>> +#endif /* CONFIG_OF */
>> +
>> +static struct platform_driver c3_pinctrl_driver = {
>> +     .probe  = meson_pinctrl_probe,
>> +     .driver = {
>> +             .name = "amlogic-c3-pinctrl",
> 
>> +             .of_match_table = of_match_ptr(c3_pinctrl_dt_match),
> 
> Drop the rather problematic of_match_ptr().

Okay

> 
>> +     },
>> +};
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
> 


Best Regards,
Huqiang Qin