[v7,5/9] serial: sc16is7xx: fix regression with GPIO configuration

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
changed the function of the GPIOs pins to act as modem control
lines without any possibility of selecting GPIO function.

As a consequence, applications that depends on GPIO lines configured
by default as GPIO pins no longer work as expected.

Also, the change to select modem control lines function was done only
for channel A of dual UART variants (752/762). This was not documented
in the log message.

Allow to specify GPIO or modem control line function in the device
tree, and for each of the ports (A or B).

Do so by using the new device-tree property named
"modem-control-line-ports" (property added in separate patch).

When registering GPIO chip controller, mask-out GPIO pins declared as
modem control lines according to this new "modem-control-line-ports"
DT property.

Boards that need to have GPIOS configured as modem control lines
should add that property to their device tree. Here is a list of
boards using the sc16is7xx driver in their device tree and that may
need to be modified:
    arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
    mips/boot/dts/ingenic/cu1830-neo.dts
    mips/boot/dts/ingenic/cu1000-neo.dts

Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
 1 file changed, 82 insertions(+), 21 deletions(-)

Hi Hugo,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9e87b63ed37e202c77aa17d4112da6ae0c7c097c]

url:    https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-fix-broken-port-0-uart-init/20230602-232811
base:   9e87b63ed37e202c77aa17d4112da6ae0c7c097c
patch link:    https://lore.kernel.org/r/20230602152626.284324-6-hugo%40hugovil.com
patch subject: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration
config: microblaze-randconfig-s052-20230531 (https://download.01.org/0day-ci/archive/20230603/202306030535.SjzsDJSD-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 12.3.0
reproduce:
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/24626643fe711f447b04a2421ef68e8e8cce86d1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Hugo-Villeneuve/serial-sc16is7xx-fix-broken-port-0-uart-init/20230602-232811
        git checkout 24626643fe711f447b04a2421ef68e8e8cce86d1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/tty/serial/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306030535.SjzsDJSD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_probe':
>> drivers/tty/serial/sc16is7xx.c:1450:12: warning: variable 'mctrl_mask' set but not used [-Wunused-but-set-variable]
    1450 |         u8 mctrl_mask;
         |            ^~~~~~~~~~


vim +/mctrl_mask +1450 drivers/tty/serial/sc16is7xx.c

  1443	
  1444	static int sc16is7xx_probe(struct device *dev,
  1445				   const struct sc16is7xx_devtype *devtype,
  1446				   struct regmap *regmap, int irq)
  1447	{
  1448		unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
  1449		unsigned int val;
> 1450		u8 mctrl_mask;
  1451		u32 uartclk = 0;
  1452		int i, ret;
  1453		struct sc16is7xx_port *s;
  1454	
  1455		if (IS_ERR(regmap))
  1456			return PTR_ERR(regmap);
  1457	
  1458		/*
  1459		 * This device does not have an identification register that would
  1460		 * tell us if we are really connected to the correct device.
  1461		 * The best we can do is to check if communication is at all possible.
  1462		 */
  1463		ret = regmap_read(regmap,
  1464				  SC16IS7XX_LSR_REG << SC16IS7XX_REG_SHIFT, &val);
  1465		if (ret < 0)
  1466			return -EPROBE_DEFER;
  1467	
  1468		/* Alloc port structure */
  1469		s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
  1470		if (!s) {
  1471			dev_err(dev, "Error allocating port structure\n");
  1472			return -ENOMEM;
  1473		}
  1474	
  1475		/* Always ask for fixed clock rate from a property. */
  1476		device_property_read_u32(dev, "clock-frequency", &uartclk);
  1477	
  1478		s->clk = devm_clk_get_optional(dev, NULL);
  1479		if (IS_ERR(s->clk))
  1480			return PTR_ERR(s->clk);
  1481	
  1482		ret = clk_prepare_enable(s->clk);
  1483		if (ret)
  1484			return ret;
  1485	
  1486		freq = clk_get_rate(s->clk);
  1487		if (freq == 0) {
  1488			if (uartclk)
  1489				freq = uartclk;
  1490			if (pfreq)
  1491				freq = *pfreq;
  1492			if (freq)
  1493				dev_dbg(dev, "Clock frequency: %luHz\n", freq);
  1494			else
  1495				return -EINVAL;
  1496		}
  1497	
  1498		s->regmap = regmap;
  1499		s->devtype = devtype;
  1500		dev_set_drvdata(dev, s);
  1501		mutex_init(&s->efr_lock);
  1502	
  1503		kthread_init_worker(&s->kworker);
  1504		s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
  1505					      "sc16is7xx");
  1506		if (IS_ERR(s->kworker_task)) {
  1507			ret = PTR_ERR(s->kworker_task);
  1508			goto out_clk;
  1509		}
  1510		sched_set_fifo(s->kworker_task);
  1511	
  1512		/* reset device, purging any pending irq / data */
  1513		regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
  1514				SC16IS7XX_IOCONTROL_SRESET_BIT);
  1515	
  1516		for (i = 0; i < devtype->nr_uart; ++i) {
  1517			s->p[i].line		= i;
  1518			/* Initialize port data */
  1519			s->p[i].port.dev	= dev;
  1520			s->p[i].port.irq	= irq;
  1521			s->p[i].port.type	= PORT_SC16IS7XX;
  1522			s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
  1523			s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
  1524			s->p[i].port.iobase	= i;
  1525			s->p[i].port.membase	= (void __iomem *)~0;
  1526			s->p[i].port.iotype	= UPIO_PORT;
  1527			s->p[i].port.uartclk	= freq;
  1528			s->p[i].port.rs485_config = sc16is7xx_config_rs485;
  1529			s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
  1530			s->p[i].port.ops	= &sc16is7xx_ops;
  1531			s->p[i].old_mctrl	= 0;
  1532			s->p[i].port.line	= sc16is7xx_alloc_line();
  1533	
  1534			if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
  1535				ret = -ENOMEM;
  1536				goto out_ports;
  1537			}
  1538	
  1539			/* Disable all interrupts */
  1540			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
  1541			/* Disable TX/RX */
  1542			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
  1543					     SC16IS7XX_EFCR_RXDISABLE_BIT |
  1544					     SC16IS7XX_EFCR_TXDISABLE_BIT);
  1545	
  1546			/* Initialize kthread work structs */
  1547			kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
  1548			kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
  1549			kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
  1550			/* Register port */
  1551			uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
  1552	
  1553			/* Enable EFR */
  1554			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
  1555					     SC16IS7XX_LCR_CONF_MODE_B);
  1556	
  1557			regcache_cache_bypass(s->regmap, true);
  1558	
  1559			/* Enable write access to enhanced features */
  1560			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
  1561					     SC16IS7XX_EFR_ENABLE_BIT);
  1562	
  1563			regcache_cache_bypass(s->regmap, false);
  1564	
  1565			/* Restore access to general registers */
  1566			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
  1567	
  1568			/* Go to suspend mode */
  1569			sc16is7xx_power(&s->p[i].port, 0);
  1570		}
  1571	
  1572		if (dev->of_node) {
  1573			struct property *prop;
  1574			const __be32 *p;
  1575			u32 u;
  1576	
  1577			of_property_for_each_u32(dev->of_node, "irda-mode-ports",
  1578						 prop, p, u)
  1579				if (u < devtype->nr_uart)
  1580					s->p[u].irda_mode = true;
  1581		}
  1582	
  1583		mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
  1584

On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> changed the function of the GPIOs pins to act as modem control
> lines without any possibility of selecting GPIO function.
> 
> As a consequence, applications that depends on GPIO lines configured
> by default as GPIO pins no longer work as expected.
> 
> Also, the change to select modem control lines function was done only
> for channel A of dual UART variants (752/762). This was not documented
> in the log message.
> 
> Allow to specify GPIO or modem control line function in the device
> tree, and for each of the ports (A or B).
> 
> Do so by using the new device-tree property named
> "modem-control-line-ports" (property added in separate patch).
> 
> When registering GPIO chip controller, mask-out GPIO pins declared as
> modem control lines according to this new "modem-control-line-ports"
> DT property.
> 
> Boards that need to have GPIOS configured as modem control lines
> should add that property to their device tree. Here is a list of
> boards using the sc16is7xx driver in their device tree and that may
> need to be modified:
>     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>     mips/boot/dts/ingenic/cu1830-neo.dts
>     mips/boot/dts/ingenic/cu1000-neo.dts
> 
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
> Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
> Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
> Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
> Cc: <stable@vger.kernel.org> # 6.1.x
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
>  1 file changed, 82 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7d50674d2d0e..edc83f5f6340 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -236,7 +236,8 @@
>  
>  /* IOControl register bits (Only 750/760) */
>  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
>  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
>  
>  /* EFCR register bits */
> @@ -301,12 +302,12 @@
>  /* Misc definitions */
>  #define SC16IS7XX_FIFO_SIZE		(64)
>  #define SC16IS7XX_REG_SHIFT		2
> +#define SC16IS7XX_GPIOS_PER_BANK	4
>  
>  struct sc16is7xx_devtype {
>  	char	name[10];
>  	int	nr_gpio;
>  	int	nr_uart;
> -	int	has_mctrl;
>  };
>  
>  #define SC16IS7XX_RECONF_MD		(1 << 0)
> @@ -336,6 +337,7 @@ struct sc16is7xx_port {
>  	struct clk			*clk;
>  #ifdef CONFIG_GPIOLIB
>  	struct gpio_chip		gpio;
> +	unsigned long			gpio_valid_mask;
>  #endif
>  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
>  	struct kthread_worker		kworker;
> @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
>  	.name		= "SC16IS74X",
>  	.nr_gpio	= 0,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 0,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is750_devtype = {
>  	.name		= "SC16IS750",
> -	.nr_gpio	= 4,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is752_devtype = {
>  	.name		= "SC16IS752",
> -	.nr_gpio	= 0,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 2,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is760_devtype = {
>  	.name		= "SC16IS760",
> -	.nr_gpio	= 4,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 1,
> -	.has_mctrl	= 1,
>  };
>  
>  static const struct sc16is7xx_devtype sc16is762_devtype = {
>  	.name		= "SC16IS762",
> -	.nr_gpio	= 0,
> +	.nr_gpio	= 8,
>  	.nr_uart	= 2,
> -	.has_mctrl	= 1,
>  };
>  
>  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
>  	return 0;
>  }
>  
> -static int sc16is7xx_setup_gpio_chip(struct device *dev)
> +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
> +					  unsigned long *valid_mask,
> +					  unsigned int ngpios)
> +{
> +	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> +
> +	*valid_mask = s->gpio_valid_mask;
> +
> +	return 0;
> +}
> +
> +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
>  {
>  	struct sc16is7xx_port *s = dev_get_drvdata(dev);
>  
>  	if (!s->devtype->nr_gpio)
>  		return 0;
>  
> +	switch (mctrl_mask) {
> +	case 0:
> +		s->gpio_valid_mask = GENMASK(7, 0);
> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> +		s->gpio_valid_mask = GENMASK(3, 0);
> +		break;
> +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> +		s->gpio_valid_mask = GENMASK(7, 4);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (s->gpio_valid_mask == 0)
> +		return 0;
> +
>  	s->gpio.owner		 = THIS_MODULE;
>  	s->gpio.parent		 = dev;
>  	s->gpio.label		 = dev_name(dev);
> +	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
>  	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
>  	s->gpio.get		 = sc16is7xx_gpio_get;
>  	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
> @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
>  }
>  #endif
>  
> +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)

This returns what, mctrl?  If so, please document that, it doesn't look
obvious.  And as the kernel test robot reported, you do nothing with the
return value so why compute it?

And you have a real port here, no need to pass in a "raw" struct device,
right?

thanks,

greg k-h

On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:

...

> > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
>
> This returns what, mctrl?  If so, please document that, it doesn't look
> obvious.

Good suggestion. Because I also stumbled over the returned type.

>  And as the kernel test robot reported, you do nothing with the
> return value so why compute it?

It seems that the entire function and respective call has to be moved
under #ifdef CONFIG_GPIOLIB.

> And you have a real port here, no need to pass in a "raw" struct device,
> right?

On Sun, 4 Jun 2023 09:47:44 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > changed the function of the GPIOs pins to act as modem control
> > lines without any possibility of selecting GPIO function.
> > 
> > As a consequence, applications that depends on GPIO lines configured
> > by default as GPIO pins no longer work as expected.
> > 
> > Also, the change to select modem control lines function was done only
> > for channel A of dual UART variants (752/762). This was not documented
> > in the log message.
> > 
> > Allow to specify GPIO or modem control line function in the device
> > tree, and for each of the ports (A or B).
> > 
> > Do so by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > When registering GPIO chip controller, mask-out GPIO pins declared as
> > modem control lines according to this new "modem-control-line-ports"
> > DT property.
> > 
> > Boards that need to have GPIOS configured as modem control lines
> > should add that property to their device tree. Here is a list of
> > boards using the sc16is7xx driver in their device tree and that may
> > need to be modified:
> >     arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
> >     mips/boot/dts/ingenic/cu1830-neo.dts
> >     mips/boot/dts/ingenic/cu1000-neo.dts
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > Cc: <stable@vger.kernel.org> # 6.1.x: 35210b22 dt-bindings: sc16is7xx: Add property to change GPIO function
> > Cc: <stable@vger.kernel.org> # 6.1.x: 7d61ca47 serial: sc16is7xx: refactor GPIO controller registration
> > Cc: <stable@vger.kernel.org> # 6.1.x: 322470ed serial: sc16is7xx: mark IOCONTROL register as volatile
> > Cc: <stable@vger.kernel.org> # 6.1.x: a0077362 serial: sc16is7xx: fix broken port 0 uart init
> > Cc: <stable@vger.kernel.org> # 6.1.x
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 103 ++++++++++++++++++++++++++-------
> >  1 file changed, 82 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 7d50674d2d0e..edc83f5f6340 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -236,7 +236,8 @@
> >  
> >  /* IOControl register bits (Only 750/760) */
> >  #define SC16IS7XX_IOCONTROL_LATCH_BIT	(1 << 0) /* Enable input latching */
> > -#define SC16IS7XX_IOCONTROL_MODEM_BIT	(1 << 1) /* Enable GPIO[7:4] as modem pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT	(1 << 1) /* Enable GPIO[7:4] as modem A pins */
> > +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT	(1 << 2) /* Enable GPIO[3:0] as modem B pins */
> >  #define SC16IS7XX_IOCONTROL_SRESET_BIT	(1 << 3) /* Software Reset */
> >  
> >  /* EFCR register bits */
> > @@ -301,12 +302,12 @@
> >  /* Misc definitions */
> >  #define SC16IS7XX_FIFO_SIZE		(64)
> >  #define SC16IS7XX_REG_SHIFT		2
> > +#define SC16IS7XX_GPIOS_PER_BANK	4
> >  
> >  struct sc16is7xx_devtype {
> >  	char	name[10];
> >  	int	nr_gpio;
> >  	int	nr_uart;
> > -	int	has_mctrl;
> >  };
> >  
> >  #define SC16IS7XX_RECONF_MD		(1 << 0)
> > @@ -336,6 +337,7 @@ struct sc16is7xx_port {
> >  	struct clk			*clk;
> >  #ifdef CONFIG_GPIOLIB
> >  	struct gpio_chip		gpio;
> > +	unsigned long			gpio_valid_mask;
> >  #endif
> >  	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
> >  	struct kthread_worker		kworker;
> > @@ -447,35 +449,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
> >  	.name		= "SC16IS74X",
> >  	.nr_gpio	= 0,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 0,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is750_devtype = {
> >  	.name		= "SC16IS750",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is752_devtype = {
> >  	.name		= "SC16IS752",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is760_devtype = {
> >  	.name		= "SC16IS760",
> > -	.nr_gpio	= 4,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 1,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static const struct sc16is7xx_devtype sc16is762_devtype = {
> >  	.name		= "SC16IS762",
> > -	.nr_gpio	= 0,
> > +	.nr_gpio	= 8,
> >  	.nr_uart	= 2,
> > -	.has_mctrl	= 1,
> >  };
> >  
> >  static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> > @@ -1350,16 +1347,45 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
> >  	return 0;
> >  }
> >  
> > -static int sc16is7xx_setup_gpio_chip(struct device *dev)
> > +static int sc16is7xx_gpio_init_valid_mask(struct gpio_chip *chip,
> > +					  unsigned long *valid_mask,
> > +					  unsigned int ngpios)
> > +{
> > +	struct sc16is7xx_port *s = gpiochip_get_data(chip);
> > +
> > +	*valid_mask = s->gpio_valid_mask;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sc16is7xx_setup_gpio_chip(struct device *dev, u8 mctrl_mask)
> >  {
> >  	struct sc16is7xx_port *s = dev_get_drvdata(dev);
> >  
> >  	if (!s->devtype->nr_gpio)
> >  		return 0;
> >  
> > +	switch (mctrl_mask) {
> > +	case 0:
> > +		s->gpio_valid_mask = GENMASK(7, 0);
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_A_BIT:
> > +		s->gpio_valid_mask = GENMASK(3, 0);
> > +		break;
> > +	case SC16IS7XX_IOCONTROL_MODEM_B_BIT:
> > +		s->gpio_valid_mask = GENMASK(7, 4);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	if (s->gpio_valid_mask == 0)
> > +		return 0;
> > +
> >  	s->gpio.owner		 = THIS_MODULE;
> >  	s->gpio.parent		 = dev;
> >  	s->gpio.label		 = dev_name(dev);
> > +	s->gpio.init_valid_mask	 = sc16is7xx_gpio_init_valid_mask;
> >  	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
> >  	s->gpio.get		 = sc16is7xx_gpio_get;
> >  	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
> > @@ -1371,6 +1397,44 @@ static int sc16is7xx_setup_gpio_chip(struct device *dev)
> >  }
> >  #endif
> >  
> > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> 
> This returns what, mctrl?  If so, please document that, it doesn't look
> obvious.  And as the kernel test robot reported, you do nothing with the
> return value so why compute it?

Hi Greg,
I will  the following comment to document return value:

/*
 * Configure ports designated to operate as modem control lines.
 * Return mask of ports configured as modem control lines.
 */
static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)


The kernel test robot identified a case, when CONFIG_GPIOLIB is not defined, where the value returned by sc16is7xx_setup_mctrl_ports() is not used. But the function sc16is7xx_setup_mctrl_ports() still need to be called to configure the modem status line ports correctly in that case.

And if CONFIG_GPIOLIB is defined, the value is definitely used (and needed).

Here is what I suggest to silence the warning:

	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);

#ifdef CONFIG_GPIOLIB
	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
	if (ret)
		goto out_thread;
#else
	(void) mctrl_mask;
#endif

I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...


> And you have a real port here, no need to pass in a "raw" struct device,
> right?

The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
struct sc16is7xx_port from it:

    struct sc16is7xx_port *s = dev_get_drvdata(dev);

Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:

static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
{
	struct device *dev = &s->p[0].port.dev;

But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...

Hugo.

On Sun, 4 Jun 2023 14:57:31 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> 
> ...
> 
> > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> >
> > This returns what, mctrl?  If so, please document that, it doesn't look
> > obvious.
> 
> Good suggestion. Because I also stumbled over the returned type.
> 
> >  And as the kernel test robot reported, you do nothing with the
> > return value so why compute it?
> 
> It seems that the entire function and respective call has to be moved
> under #ifdef CONFIG_GPIOLIB.

Hi,
it cannot. See my explanations in response to Greg's comments.

Hugo.

On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> Here is what I suggest to silence the warning:
> 
> 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> 
> #ifdef CONFIG_GPIOLIB
> 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> 	if (ret)
> 		goto out_thread;
> #else
> 	(void) mctrl_mask;
> #endif

Eeek,  no, please no...

First off, please don't put #ifdef in .c files if at all possible.
Secondly, that (void) craziness is just that.  Rework this to not be an
issue some other way please.

> I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...

Sure, that sounds best.

> > And you have a real port here, no need to pass in a "raw" struct device,
> > right?
> 
> The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> struct sc16is7xx_port from it:
> 
>     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> 
> Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> 
> static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> {
> 	struct device *dev = &s->p[0].port.dev;
> 
> But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...

You should never need a "raw" struct device for stuff (if so, something
is really odd).  Except for error messages, but that's not really a big
deal, right?

Don't pass around struct device in a driver, use the real types as you
know you have it and it saves odd casting around and it just doesn't
look safe at all to do so.

And if you have that crazy s->p.... stuff in multiple places, then
perhaps you might want to rethink the structure somehow?  Or at the very
least, write an inline function to get it when needed.

Also, meta comment, you might want to use some \n characters in your
emails, your lines are really long :)

thanks,

greg k-h

On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
>
> On Sun, 4 Jun 2023 14:57:31 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> >
> > ...
> >
> > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> > >
> > > This returns what, mctrl?  If so, please document that, it doesn't look
> > > obvious.
> >
> > Good suggestion. Because I also stumbled over the returned type.
> >
> > >  And as the kernel test robot reported, you do nothing with the
> > > return value so why compute it?
> >
> > It seems that the entire function and respective call has to be moved
> > under #ifdef CONFIG_GPIOLIB.
>
> Hi,
> it cannot. See my explanations in response to Greg's comments.

Then as Greg suggested, store in the structure and make this function
to return an error code (with int), with this amendment you don't need
to add a comment about the returned variable anymore.

On Sun, 4 Jun 2023 20:29:58 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> > Here is what I suggest to silence the warning:
> > 
> > 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> > 
> > #ifdef CONFIG_GPIOLIB
> > 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> > 	if (ret)
> > 		goto out_thread;
> > #else
> > 	(void) mctrl_mask;
> > #endif
> 
> Eeek,  no, please no...
> 
> First off, please don't put #ifdef in .c files if at all possible.

Hi Greg,
Andy also made a similar comment, but couldn't suggest a valid
alternative when I asked him what to do about that.

Just as a sidenote, I didn't add those #ifdef, they were already
present in the driver in multiple places.

What would be your suggestion to get rid of those #ifdef, simply delete
them all?

If you suggest me what to do, I will be happy to submit a
future patch after this series is finalized to clean that aspect.


> Secondly, that (void) craziness is just that.  Rework this to not be an
> issue some other way please.
> 
> > I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...
> 
> Sure, that sounds best.

Ok, I will do that.


> > > And you have a real port here, no need to pass in a "raw" struct device,
> > > right?
> > 
> > The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> > struct sc16is7xx_port from it:
> > 
> >     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > 
> > Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> > 
> > static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> > {
> > 	struct device *dev = &s->p[0].port.dev;
> > 
> > But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...
> 
> You should never need a "raw" struct device for stuff (if so, something
> is really odd).  Except for error messages, but that's not really a big
> deal, right?

> Don't pass around struct device in a driver, use the real types as you
> know you have it and it saves odd casting around and it just doesn't
> look safe at all to do so.

If you look at the patch, you will see that I need "struct device *dev"
at two places in the sc16is7xx_setup_mctrl_ports() function to read the
device properties:

...
+static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
...
+	count = device_property_count_u32(dev,...
...
+	ret = device_property_read_u32_array(dev,
...

I do not understand why this is odd?


> And if you have that crazy s->p.... stuff in multiple places, the
> perhaps you might want to rethink the structure somehow?  Or at the very
> least, write an inline function to get it when needed.

I am not sure what you mean by that, since again that "crazy" stuff is
already used everywhere in this driver?


> Also, meta comment, you might want to use some \n characters in your
> emails, your lines are really long :)

Strange, I use sylpheed as a mail client, and the option "Wrap lines at
72 characters" is enabled by default, but somehow you must also check
the box "Wrap on input" for it to work, not very intuitive :) Thanks for
pointing that to me.

Hugo.

On Sun, 4 Jun 2023 22:31:04 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 4, 2023 at 8:45 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> >
> > On Sun, 4 Jun 2023 14:57:31 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> > > On Sun, Jun 4, 2023 at 10:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Fri, Jun 02, 2023 at 11:26:21AM -0400, Hugo Villeneuve wrote:
> > >
> > > ...
> > >
> > > > > +static u8 sc16is7xx_setup_mctrl_ports(struct device *dev)
> > > >
> > > > This returns what, mctrl?  If so, please document that, it doesn't look
> > > > obvious.
> > >
> > > Good suggestion. Because I also stumbled over the returned type.
> > >
> > > >  And as the kernel test robot reported, you do nothing with the
> > > > return value so why compute it?
> > >
> > > It seems that the entire function and respective call has to be moved
> > > under #ifdef CONFIG_GPIOLIB.
> >
> > Hi,
> > it cannot. See my explanations in response to Greg's comments.
> 
> Then as Greg suggested, store in the structure and make this function
> to return an error code (with int), with this amendment you don't need
> to add a comment about the returned variable anymore.

Hi,
Yes, that is what I have done for V8. Simplifies/clean things a lot.

Hugo.

On Tue, 20 Jun 2023 12:16:45 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Tue, 20 Jun 2023 18:45:51 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 
> > ...
> > 
> > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > of what we discussed?
> > > > > >
> > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > >
> > > > > Hi Andy,
> > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > review them. Can you confirm if the changes are correct?
> > > > >
> > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > real impact on the code generation itself, but maybe you can review and
> > > > > confirm if tags are ok or not, based on commit message and also
> > > > > additional commit message.
> > > >
> > > > Both are fine to me.
> > >
> > > Hi,
> > > Ok, thank you for reviewing this.
> > >
> > > I guess now we are good to go with this series if the stable tags and
> > > patches order are good after Greg's review?
> > 
> > Taking into account that we are at rc7, and even with Fixes tags in
> > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > up to him how to proceed with that. Note, he usually has thousands of
> > patches in backlog, you might need to respin it after the above
> > mentioned rc1.
> 
> Ok, understood.
> 
> Let's wait then.

Hi Andy/Greg,
we are now at v6.5-rc2 and I still do not see any of our patches in
linus or gregkh_tty repos.

Is there something missing from my part (or someone else) to go forward
with integrating these patches (v8) for v6.5?

Thank you,
Hugo.

On Wed, Jul 19, 2023 at 02:40:48PM -0400, Hugo Villeneuve wrote:
> On Tue, 20 Jun 2023 12:16:45 -0400
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> 
> > On Tue, 20 Jun 2023 18:45:51 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 
> > > On Tue, Jun 20, 2023 at 6:42 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > On Tue, 20 Jun 2023 18:35:48 +0300
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > On Tue, Jun 20, 2023 at 6:33 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > On Tue, 20 Jun 2023 18:18:12 +0300
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Tue, Jun 20, 2023 at 5:08 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > > > > > > On Sun, 4 Jun 2023 22:31:04 +0300
> > > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > 
> > > ...
> > > 
> > > > > > > > did you have a chance to look at V8 (sent two weks ago) which fixed all
> > > > > > > > of what we discussed?
> > > > > > >
> > > > > > > The patch 6 already has my tag, anything specific you want me to do?
> > > > > >
> > > > > > Hi Andy,
> > > > > > I forgot to remove your "Reviewed-by: Andy..." tag before sending V8
> > > > > > since there were some changes involved in patch 6 and I wanted you to
> > > > > > review them. Can you confirm if the changes are correct?
> > > > > >
> > > > > > I also added a new patch "remove obsolete out_thread label". It has no
> > > > > > real impact on the code generation itself, but maybe you can review and
> > > > > > confirm if tags are ok or not, based on commit message and also
> > > > > > additional commit message.
> > > > >
> > > > > Both are fine to me.
> > > >
> > > > Hi,
> > > > Ok, thank you for reviewing this.
> > > >
> > > > I guess now we are good to go with this series if the stable tags and
> > > > patches order are good after Greg's review?
> > > 
> > > Taking into account that we are at rc7, and even with Fixes tags in
> > > your series I think Greg might take this after v6.5-0rc1 is out. It's
> > > up to him how to proceed with that. Note, he usually has thousands of
> > > patches in backlog, you might need to respin it after the above
> > > mentioned rc1.
> > 
> > Ok, understood.
> > 
> > Let's wait then.
> 
> Hi Andy/Greg,
> we are now at v6.5-rc2 and I still do not see any of our patches in
> linus or gregkh_tty repos.
> 
> Is there something missing from my part (or someone else) to go forward
> with integrating these patches (v8) for v6.5?

My queue is huge right now, please be patient, I want to have them all
handled by the end of next week...

You can always help out by reviewing other patches on the mailing list
to reduce my review load.

thanks,

greg k-h