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@xxxxxxxxx> >> Signed-off-by: Gary Wang <garywang@xxxxxxxxxxxx> >> Signed-off-by: larry.lai <larry.lai@xxxxxxxxxxxxxxx> > > 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
