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

[Andy Shevchenko]

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