Re: [PATCH V7 2/3] pinctrl: Add support pin control for UP board CPLD/FPGA

[larry <larry.lai@yunjingtech.com>]

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