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 ? > +#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. > +#include "core.h" ... > +#include "intel/pinctrl-intel.h" I do not think it's correct use of the header. ... > +/* 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? ... > +/* 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! ... 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. -- With Best Regards, Andy Shevchenko
