Hi Andy, Thanks for your reviewer, for your question, please kindly to check our comments with ">>" beginning. -----Original Message----- From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> Sent: Tuesday, November 14, 2023 10:19 PM To: larry.lai <larry.lai@xxxxxxxxxxxxxxx> Cc: lee@xxxxxxxxxx; linus.walleij@xxxxxxxxxx; pavel@xxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-leds@xxxxxxxxxxxxxxx; GaryWang 汪之逸 <GaryWang@xxxxxxxxxxxx>; musa.lin@xxxxxxxxxxxxxxx; jack.chang@xxxxxxxxxxxxxxx; noah.hung@xxxxxxxxxxxxxxx Subject: Re: [PATCH V7 2/3] pinctrl: Add support pin control for UP board CPLD/FPGA 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. -- With Best Regards, Andy Shevchenko
