Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <e21cbadf-84a9-93b0-ea07-924c17df8eb4@yunjingtech.com>
Date:   Fri, 1 Dec 2023 16:40:57 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V7 2/3] pinctrl: Add support pin control for UP board
 CPLD/FPGA
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     lee@kernel.org, linus.walleij@linaro.org, pavel@ucw.cz,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-leds@vger.kernel.org, GaryWang@aaeon.com.tw,
        musa.lin@yunjingtech.com, jack.chang@yunjingtech.com,
        noah.hung@yunjingtech.com
References: <20231031015119.29756-1-larry.lai@yunjingtech.com>
 <20231031015119.29756-3-larry.lai@yunjingtech.com>
 <ZVOBz8-tahhrVmO-@smile.fi.intel.com>
From:   larry <larry.lai@yunjingtech.com>
In-Reply-To: <ZVOBz8-tahhrVmO-@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?eUZUajFzSk1HMEJCL2tKV3RtWHorZExFOTVMM0dUSGlqVjRNVGovdHFGbTZM?=
 =?utf-8?B?bXZ0QzA1QmpUREt6VXd1VmhMSnVrUXR2ckZYZXdQd3hCY2NybEVVbDFKZWJl?=
 =?utf-8?B?aDBVRmVzMCtqRS9adk9Fcys4ZFJaMVJuRGluZ0pma3llbnNKcGZIa3ZKb3Vk?=
 =?utf-8?B?SC9kWFZFSmxwQlNkTzdtWk92TkpydXRFd1Rra09zcVJCZ2RFanJISTdCc1BM?=
 =?utf-8?B?VkpGdDhXbjQ2RG5MM1RSdzRPeUE5OHg2Z1ZqMkNvM25UYXFSUDFsZkZ5VFVi?=
 =?utf-8?B?OUtlN1RnaGZJODljU1ZzeVROckJTMm95b3FoOEtnYTBVblpUUWtxNXRqaDQw?=
 =?utf-8?B?VmpyaVFucFNuemZ5UWZLNGllT09mVklyLzdOQ05tS08yaDBVWDV5UXNSaHEr?=
 =?utf-8?B?THk2NFVCOFUvcFB1d0lrZGN3Z1h2RjJIYTBaaHhLd2gvS05HbXZNUUFuRStq?=
 =?utf-8?B?NWZsSTQ5M3pEMW5MVWpYc2NuWjhvemVjTklSRDFFMzc0T0J0QzFVTGtUbUJH?=
 =?utf-8?B?STZkUWFsZHpTQ1Q5VDkvdUJTSGluUktlbkpud01jZzREQk5WK1ZnUkZSbXBo?=
 =?utf-8?B?OFlZUlBka3ROaGhBWlB2bU4wTFM2WUtic1FKZG9LTWtnekJEWXIxQWk0bDRU?=
 =?utf-8?B?QzZpR2JBUUx4d2JjcUlrOFVYTDdKalFrblFZMXpFVWJod1kyRlg5ZzVXNW16?=
 =?utf-8?B?b3EzQVB0ZTlJSlhoL3JoWEZqL3JWOEdWTU1tcytkMU5GbWRzcFgzNzlGMlln?=
 =?utf-8?B?UTYyTDJBYzN3REtLOWJYMDFqdTZTK2NiNlFQNjUzbGc0ZlZ3VE5DVm9kSFhr?=
 =?utf-8?B?S2FvNm1icHVSNFdGUGNDK1hUcHdJdEdJdE5Lc2ZwUmcrekxsV2d2TnhkWC9h?=
 =?utf-8?B?OEdEdGJGNFBJMkh3cFNWS0Zxdk5OMWVFZTE3LzZnZXM3VHJaNW1UYzhCcHNa?=
 =?utf-8?B?UG1jUUduaTlxOHFCcjI2S2VzK2VXYjFjWEd2YWhCTEw0OVpUeGlESW1WT2NV?=
 =?utf-8?B?MVlYMWxpUjNBMk9VTDN3c1F5V3M4T25UbVF2bVd1ckpxVjExWkpQQjJ0ZHo3?=
 =?utf-8?B?VTlMdmJ0bDBUTmhVbHE1dWJSemRtU2pWQVVEZ20zQWNCTTZaUDdGSHlLZzdQ?=
 =?utf-8?B?eXlpK0Q1cC9QY1VwQXlHdmQ0VUxUVDBscENLdzBHR0JGNlRjdmFnOTZXU1FJ?=
 =?utf-8?B?czFYeXZFUFVMV3h4WWlaRDI0VVZBdkRHaHdadlBlWFVsVy9qZ3RWUU1YWTZ4?=
 =?utf-8?B?eDZCM1NiOXZWaWV5dllDZXB1cTgzaVNtME1tQ1dSQTMycmxmTzNuUmVjVkl3?=
 =?utf-8?B?dW5SQ2FGcC8rb3ZxbHZKTWwyQXR4RjdNL2JUbDk2dmJVZHZ3alZXQUVycmZH?=
 =?utf-8?B?Y1h3dU1ncCtScVJzeFZIMEkyOUlPcjgxVktMeTMzTWFKVDk0TmpuNVFVd1lM?=
 =?utf-8?B?QllLM2tKMklucFVoVTgwenFiQm5xMEVKa09pUllVeXpHYmxPRm5ReEx6ZUVL?=
 =?utf-8?B?T05rQmpFWUFnOGkvWDlEcjAvOFUzeFpwcUxma2ZodTJobFZ5WVVIaGtpMGMr?=
 =?utf-8?B?Q3lYdHJvbXcyRWx1MUpmazV0SmVoYVd4dnE1M08wTnU4b1d3Q0pWNkpBZUtY?=
 =?utf-8?B?OE5pQlFmbnV1dzhWUkl1b1gwbGhkckpvUlJMZjc4REI3WGpOSkhlWjU5OVZ4?=
 =?utf-8?B?aHVhOFc4VU5BR0dSd0RHS2k4bWxHVnhxWWIxbFpiNmtsdHpSNHZlU3pnSHpr?=
 =?utf-8?B?QUtvL3AxeDVvckwrYkk1OFB2cmZIeWllQnBEbThJRkE3R3R0K3dyc2dITzBp?=
 =?utf-8?B?TUtWRVdSMzdXWFNweHo3aGNaYk9WUXluWUM2NXBVVmNYWmZndTQzbVFydWZB?=
 =?utf-8?B?WXBpQ3RoL0dPQ2FYdUtGSi9TOXV5d1ZHeDQrU0NNSkQxTjRvZkt3SjhyNU56?=
 =?utf-8?B?QnF6SnkxTXlQTVkzSXlpVnpNc1NMVkJNWDUvTEp4NmRJdVpDTzN5VzZ2RDRG?=
 =?utf-8?B?SmhmY09ieDkzSVFnN1Btdy9RNFpiUzNua04xQ09jbTJkcitPSG1MMTIwTWZH?=
 =?utf-8?B?VnViRWxiQTZQYzNRZEFqSS9Va0VVNzVGOE5YSmZxMW1QSDQrMFJ2a2Q2MCto?=
 =?utf-8?B?S3RUdzI2dWRBSEFWeGRzdmtiVllRaHA2V2NuSEMzSk9yOEFsMmJZN01xS1dU?=
 =?utf-8?B?U0E9PQ==?=
X-OriginatorOrg: yunjingtech.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 39ef334a-586b-47f8-522e-08dbf2493f24
X-MS-Exchange-CrossTenant-AuthSource: SEYPR06MB6507.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2023 08:41:01.0385
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: be2d5505-f7e6-4600-bbe2-b3201c91b344
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zBsi9UUAJoXFqVJWasxS2EkszlFLswdKK03I7X3WKdAostyqdLyuGhfrvi1PKQ/CZyx/1wIzeq64yrY3jTKgPdVBupmookACc/QC6RwlM94=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PSAPR06MB4517
X-Spam-Status: No, score=-2.3 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Fri, 01 Dec 2023 00:41:29 -0800 (PST)

Hi Andy,
   Sorry, replied Gary's mail again for plain text format & mail client
as Jones's suggestion.

On 14/11/2023 22:18, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 09:51:18AM +0800, larry.lai wrote:
>> The UP Squared board <http://www.upboard.com> implements certain
>> features (pin control) through an on-board FPGA.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Gary Wang <garywang@aaeon.com.tw>
>> Signed-off-by: larry.lai <larry.lai@yunjingtech.com>
> 
> Same comments as per previous patch.
> 
> ...
> 
>> +	help
>> +	  Pin controller for the FPGA GPIO lines on UP boards. Due to the
>> +	  hardware layout, these are meant to be controlled in tandem with their
>> +	  corresponding Intel SoC GPIOs.
> 
> + blank line here.
> 
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called pinctrl-upboard.
> 
> ...
> 
>> + * UP Board HAT pin controller driver
>> + * remapping native pin to RPI pin and set CPLD pin dir
> 
> Same comment to all the comments as per previous patch.
> 
> ...
> 
> + Missing bits.h, types.h and maybe others.
> 
>> +#include <linux/dmi.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/gpio/driver.h>
> 
>> +#include <linux/kernel.h>
> 
> array_size.h ?

will remove.

> 
>> +#include <linux/mfd/upboard-fpga.h>
>> +#include <linux/module.h>
> 
>> +#include <linux/pinctrl/pinctrl.h>
>> +#include <linux/pinctrl/pinmux.h>
> 
> Move this...
> 
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/string.h>
> 
> ...here to be a group of pinctrl headers.

will do.

> 
> 
>> +#include "core.h"
> 
> ...
> 
> 
>> +#include "intel/pinctrl-intel.h"
> 
> I do not think it's correct use of the header.

see below with pad regs define.

> 
> ...
> 
>> +/* for older kernel lost DIRECTION_IN/OUT definition */
>> +#ifndef GPIO_LINE_DIRECTION_IN
>> +#define GPIO_LINE_DIRECTION_IN		1
>> +#define GPIO_LINE_DIRECTION_OUT		0
>> +#endif
> 
> Are you submitting this to older kernel here? No. Then why this?

used in our dkms driver, will remove for upstream.

> 
> ...
> 
>> +/* Offset from regs */
>> +#define REVID						0x000
>> +#define REVID_SHIFT					16
>> +#define REVID_MASK					GENMASK(31, 16)
>> +#define PADBAR						0x00c
>> +
>> +/* Offset from pad_regs */
>> +#define PADCFG0						0x000
>> +#define PADCFG0_RXEVCFG_SHIFT		25
>> +#define PADCFG0_RXEVCFG_MASK		GENMASK(26, 25)
>> +#define PADCFG0_RXEVCFG_LEVEL		0
>> +#define PADCFG0_RXEVCFG_EDGE		1
>> +#define PADCFG0_RXEVCFG_DISABLED	2
>> +#define PADCFG0_RXEVCFG_EDGE_BOTH	3
>> +#define PADCFG0_PREGFRXSEL			BIT(24)
>> +#define PADCFG0_RXINV				BIT(23)
>> +#define PADCFG0_GPIROUTIOXAPIC		BIT(20)
>> +#define PADCFG0_GPIROUTSCI			BIT(19)
>> +#define PADCFG0_GPIROUTSMI			BIT(18)
>> +#define PADCFG0_GPIROUTNMI			BIT(17)
>> +#define PADCFG0_PMODE_SHIFT			10
>> +#define PADCFG0_PMODE_MASK			GENMASK(13, 10)
>> +#define PADCFG0_PMODE_GPIO			0
>> +#define PADCFG0_GPIORXDIS			BIT(9)
>> +#define PADCFG0_GPIOTXDIS			BIT(8)
>> +#define PADCFG0_GPIORXSTATE			BIT(1)
>> +#define PADCFG0_GPIOTXSTATE			BIT(0)
>> +
>> +#define PADCFG1						0x004
>> +#define PADCFG1_TERM_UP				BIT(13)
>> +#define PADCFG1_TERM_SHIFT			10
>> +#define PADCFG1_TERM_MASK			GENMASK(12, 10)
>> +#define PADCFG1_TERM_20K			BIT(2)
>> +#define PADCFG1_TERM_5K				BIT(1)
>> +#define PADCFG1_TERM_1K				BIT(0)
>> +#define PADCFG1_TERM_833			(BIT(1) | BIT(0))
>> +
>> +#define PADCFG2						0x008
>> +#define PADCFG2_DEBEN				BIT(0)
>> +#define PADCFG2_DEBOUNCE_SHIFT		1
>> +#define PADCFG2_DEBOUNCE_MASK		GENMASK(4, 1)
>> +
>> +#define DEBOUNCE_PERIOD_NSEC		31250
>> +
>> +/* Additional features supported by the hardware */
>> +#define PINCTRL_FEATURE_DEBOUNCE	BIT(0)
>> +#define PINCTRL_FEATURE_1K_PD		BIT(1)
> 
> Huh?! No way it should be here in _any_ form!

will remove, but we need set pad mode at runtime for HAP pins GPIO,
it's not a good way but get register offset from intel_pinctrl structure
can reduce data for each boards.

> 
> ...
> 
> I'm done with review as design wise this one is broken. Please, redesign and
> reimplement. Also split this per platform addition (as suggested for MFD),
> it will be easier to review.

will try to modify as your suggestion.

>
Best Regards,
Larry