diff mbox series

[3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)

Message ID 20240206214208.2141067-4-hugo@hugovil.com
State New
Headers
Series serial: sc16is7xx: split into core and I2C/SPI parts |

Commit Message

Hugo Villeneuve Feb. 6, 2024, 9:42 p.m. UTC 
  From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Before, sc16is7xx_lines was checked for a free (zero) bit first, and then
later it was set only if UART port registration succeeded.

Now that sc16is7xx_lines is shared for the I2C and SPI drivers, make sure
it is reserved and modified atomically, and use a new variable to hold the
status of UART port regisration.

Remove need to check for previous port registration in sc16is7xx_remove(),
because if sc16is7xx_probe() succeeded, we are guaranteed to have
successfully registered both ports.

Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

kernel test robot Feb. 7, 2024, 5:23 p.m. UTC | #1 
Hi Hugo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 52b56990d214c7403b20f691ac61861a37c0f0db]

url:    https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-simplify-and-improve-Kconfig-help-text/20240207-061642
base:   52b56990d214c7403b20f691ac61861a37c0f0db
patch link:    https://lore.kernel.org/r/20240206214208.2141067-4-hugo%40hugovil.com
patch subject: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)
config: i386-buildonly-randconfig-003-20240207 (https://download.01.org/0day-ci/archive/20240208/202402080139.jQSw8VKg-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/202402080139.jQSw8VKg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402080139.jQSw8VKg-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/tty/serial/sc16is7xx.c:1539:23: error: call to undeclared function 'find_and_set_bit'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1539 |                 s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
         |                                     ^
   drivers/tty/serial/sc16is7xx.c:1539:23: note: did you mean 'test_and_set_bit'?
   include/asm-generic/bitops/instrumented-atomic.h:68:29: note: 'test_and_set_bit' declared here
      68 | static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
         |                             ^
   1 error generated.


vim +/find_and_set_bit +1539 drivers/tty/serial/sc16is7xx.c

  1462	
  1463	int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
  1464			    struct regmap *regmaps[], int irq)
  1465	{
  1466		unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
  1467		unsigned int val;
  1468		u32 uartclk = 0;
  1469		int i, ret;
  1470		struct sc16is7xx_port *s;
  1471		bool port_registered[SC16IS7XX_MAX_PORTS];
  1472	
  1473		for (i = 0; i < devtype->nr_uart; i++)
  1474			if (IS_ERR(regmaps[i]))
  1475				return PTR_ERR(regmaps[i]);
  1476	
  1477		/*
  1478		 * This device does not have an identification register that would
  1479		 * tell us if we are really connected to the correct device.
  1480		 * The best we can do is to check if communication is at all possible.
  1481		 *
  1482		 * Note: regmap[0] is used in the probe function to access registers
  1483		 * common to all channels/ports, as it is guaranteed to be present on
  1484		 * all variants.
  1485		 */
  1486		ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
  1487		if (ret < 0)
  1488			return -EPROBE_DEFER;
  1489	
  1490		/* Alloc port structure */
  1491		s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
  1492		if (!s) {
  1493			dev_err(dev, "Error allocating port structure\n");
  1494			return -ENOMEM;
  1495		}
  1496	
  1497		/* Always ask for fixed clock rate from a property. */
  1498		device_property_read_u32(dev, "clock-frequency", &uartclk);
  1499	
  1500		s->clk = devm_clk_get_optional(dev, NULL);
  1501		if (IS_ERR(s->clk))
  1502			return PTR_ERR(s->clk);
  1503	
  1504		ret = clk_prepare_enable(s->clk);
  1505		if (ret)
  1506			return ret;
  1507	
  1508		freq = clk_get_rate(s->clk);
  1509		if (freq == 0) {
  1510			if (uartclk)
  1511				freq = uartclk;
  1512			if (pfreq)
  1513				freq = *pfreq;
  1514			if (freq)
  1515				dev_dbg(dev, "Clock frequency: %luHz\n", freq);
  1516			else
  1517				return -EINVAL;
  1518		}
  1519	
  1520		s->devtype = devtype;
  1521		dev_set_drvdata(dev, s);
  1522	
  1523		kthread_init_worker(&s->kworker);
  1524		s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
  1525					      "sc16is7xx");
  1526		if (IS_ERR(s->kworker_task)) {
  1527			ret = PTR_ERR(s->kworker_task);
  1528			goto out_clk;
  1529		}
  1530		sched_set_fifo(s->kworker_task);
  1531	
  1532		/* reset device, purging any pending irq / data */
  1533		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
  1534			     SC16IS7XX_IOCONTROL_SRESET_BIT);
  1535	
  1536		for (i = 0; i < devtype->nr_uart; ++i) {
  1537			port_registered[i] = false;
  1538	
> 1539			s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
  1540							     SC16IS7XX_MAX_DEVS);
  1541			if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
  1542				ret = -ERANGE;
  1543				goto out_ports;
  1544			}
  1545	
  1546			/* Initialize port data */
  1547			s->p[i].port.dev	= dev;
  1548			s->p[i].port.irq	= irq;
  1549			s->p[i].port.type	= PORT_SC16IS7XX;
  1550			s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
  1551			s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
  1552			s->p[i].port.iobase	= i;
  1553			/*
  1554			 * Use all ones as membase to make sure uart_configure_port() in
  1555			 * serial_core.c does not abort for SPI/I2C devices where the
  1556			 * membase address is not applicable.
  1557			 */
  1558			s->p[i].port.membase	= (void __iomem *)~0;
  1559			s->p[i].port.iotype	= UPIO_PORT;
  1560			s->p[i].port.uartclk	= freq;
  1561			s->p[i].port.rs485_config = sc16is7xx_config_rs485;
  1562			s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
  1563			s->p[i].port.ops	= &sc16is7xx_ops;
  1564			s->p[i].old_mctrl	= 0;
  1565			s->p[i].regmap		= regmaps[i];
  1566	
  1567			mutex_init(&s->p[i].efr_lock);
  1568	
  1569			ret = uart_get_rs485_mode(&s->p[i].port);
  1570			if (ret)
  1571				goto out_ports;
  1572	
  1573			/* Disable all interrupts */
  1574			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
  1575			/* Disable TX/RX */
  1576			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
  1577					     SC16IS7XX_EFCR_RXDISABLE_BIT |
  1578					     SC16IS7XX_EFCR_TXDISABLE_BIT);
  1579	
  1580			/* Initialize kthread work structs */
  1581			kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
  1582			kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
  1583			kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
  1584	
  1585			/* Register port */
  1586			ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
  1587			if (ret)
  1588				goto out_ports;
  1589	
  1590			port_registered[i] = true;
  1591	
  1592			/* Enable EFR */
  1593			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
  1594					     SC16IS7XX_LCR_CONF_MODE_B);
  1595	
  1596			regcache_cache_bypass(regmaps[i], true);
  1597	
  1598			/* Enable write access to enhanced features */
  1599			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
  1600					     SC16IS7XX_EFR_ENABLE_BIT);
  1601	
  1602			regcache_cache_bypass(regmaps[i], false);
  1603	
  1604			/* Restore access to general registers */
  1605			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
  1606	
  1607			/* Go to suspend mode */
  1608			sc16is7xx_power(&s->p[i].port, 0);
  1609		}
  1610	
  1611		sc16is7xx_setup_irda_ports(s);
  1612	
  1613		ret = sc16is7xx_setup_mctrl_ports(s, regmaps[0]);
  1614		if (ret)
  1615			goto out_ports;
  1616
kernel test robot Feb. 7, 2024, 11:25 p.m. UTC | #2 
Hi Hugo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 52b56990d214c7403b20f691ac61861a37c0f0db]

url:    https://github.com/intel-lab-lkp/linux/commits/Hugo-Villeneuve/serial-sc16is7xx-simplify-and-improve-Kconfig-help-text/20240207-061642
base:   52b56990d214c7403b20f691ac61861a37c0f0db
patch link:    https://lore.kernel.org/r/20240206214208.2141067-4-hugo%40hugovil.com
patch subject: [PATCH 3/4] serial: sc16is7xx: split into core and I2C/SPI parts (sc16is7xx_lines)
config: i386-randconfig-001-20240207 (https://download.01.org/0day-ci/archive/20240208/202402080730.RUiQXpMA-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240208/202402080730.RUiQXpMA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402080730.RUiQXpMA-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/tty/serial/sc16is7xx.c: In function 'sc16is7xx_probe':
>> drivers/tty/serial/sc16is7xx.c:1539:37: error: implicit declaration of function 'find_and_set_bit'; did you mean 'test_and_set_bit'? [-Werror=implicit-function-declaration]
    1539 |                 s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
         |                                     ^~~~~~~~~~~~~~~~
         |                                     test_and_set_bit
   cc1: some warnings being treated as errors


vim +1539 drivers/tty/serial/sc16is7xx.c

  1462	
  1463	int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
  1464			    struct regmap *regmaps[], int irq)
  1465	{
  1466		unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
  1467		unsigned int val;
  1468		u32 uartclk = 0;
  1469		int i, ret;
  1470		struct sc16is7xx_port *s;
  1471		bool port_registered[SC16IS7XX_MAX_PORTS];
  1472	
  1473		for (i = 0; i < devtype->nr_uart; i++)
  1474			if (IS_ERR(regmaps[i]))
  1475				return PTR_ERR(regmaps[i]);
  1476	
  1477		/*
  1478		 * This device does not have an identification register that would
  1479		 * tell us if we are really connected to the correct device.
  1480		 * The best we can do is to check if communication is at all possible.
  1481		 *
  1482		 * Note: regmap[0] is used in the probe function to access registers
  1483		 * common to all channels/ports, as it is guaranteed to be present on
  1484		 * all variants.
  1485		 */
  1486		ret = regmap_read(regmaps[0], SC16IS7XX_LSR_REG, &val);
  1487		if (ret < 0)
  1488			return -EPROBE_DEFER;
  1489	
  1490		/* Alloc port structure */
  1491		s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
  1492		if (!s) {
  1493			dev_err(dev, "Error allocating port structure\n");
  1494			return -ENOMEM;
  1495		}
  1496	
  1497		/* Always ask for fixed clock rate from a property. */
  1498		device_property_read_u32(dev, "clock-frequency", &uartclk);
  1499	
  1500		s->clk = devm_clk_get_optional(dev, NULL);
  1501		if (IS_ERR(s->clk))
  1502			return PTR_ERR(s->clk);
  1503	
  1504		ret = clk_prepare_enable(s->clk);
  1505		if (ret)
  1506			return ret;
  1507	
  1508		freq = clk_get_rate(s->clk);
  1509		if (freq == 0) {
  1510			if (uartclk)
  1511				freq = uartclk;
  1512			if (pfreq)
  1513				freq = *pfreq;
  1514			if (freq)
  1515				dev_dbg(dev, "Clock frequency: %luHz\n", freq);
  1516			else
  1517				return -EINVAL;
  1518		}
  1519	
  1520		s->devtype = devtype;
  1521		dev_set_drvdata(dev, s);
  1522	
  1523		kthread_init_worker(&s->kworker);
  1524		s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
  1525					      "sc16is7xx");
  1526		if (IS_ERR(s->kworker_task)) {
  1527			ret = PTR_ERR(s->kworker_task);
  1528			goto out_clk;
  1529		}
  1530		sched_set_fifo(s->kworker_task);
  1531	
  1532		/* reset device, purging any pending irq / data */
  1533		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
  1534			     SC16IS7XX_IOCONTROL_SRESET_BIT);
  1535	
  1536		for (i = 0; i < devtype->nr_uart; ++i) {
  1537			port_registered[i] = false;
  1538	
> 1539			s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
  1540							     SC16IS7XX_MAX_DEVS);
  1541			if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
  1542				ret = -ERANGE;
  1543				goto out_ports;
  1544			}
  1545	
  1546			/* Initialize port data */
  1547			s->p[i].port.dev	= dev;
  1548			s->p[i].port.irq	= irq;
  1549			s->p[i].port.type	= PORT_SC16IS7XX;
  1550			s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
  1551			s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
  1552			s->p[i].port.iobase	= i;
  1553			/*
  1554			 * Use all ones as membase to make sure uart_configure_port() in
  1555			 * serial_core.c does not abort for SPI/I2C devices where the
  1556			 * membase address is not applicable.
  1557			 */
  1558			s->p[i].port.membase	= (void __iomem *)~0;
  1559			s->p[i].port.iotype	= UPIO_PORT;
  1560			s->p[i].port.uartclk	= freq;
  1561			s->p[i].port.rs485_config = sc16is7xx_config_rs485;
  1562			s->p[i].port.rs485_supported = sc16is7xx_rs485_supported;
  1563			s->p[i].port.ops	= &sc16is7xx_ops;
  1564			s->p[i].old_mctrl	= 0;
  1565			s->p[i].regmap		= regmaps[i];
  1566	
  1567			mutex_init(&s->p[i].efr_lock);
  1568	
  1569			ret = uart_get_rs485_mode(&s->p[i].port);
  1570			if (ret)
  1571				goto out_ports;
  1572	
  1573			/* Disable all interrupts */
  1574			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_IER_REG, 0);
  1575			/* Disable TX/RX */
  1576			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
  1577					     SC16IS7XX_EFCR_RXDISABLE_BIT |
  1578					     SC16IS7XX_EFCR_TXDISABLE_BIT);
  1579	
  1580			/* Initialize kthread work structs */
  1581			kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
  1582			kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
  1583			kthread_init_delayed_work(&s->p[i].ms_work, sc16is7xx_ms_proc);
  1584	
  1585			/* Register port */
  1586			ret = uart_add_one_port(&sc16is7xx_uart, &s->p[i].port);
  1587			if (ret)
  1588				goto out_ports;
  1589	
  1590			port_registered[i] = true;
  1591	
  1592			/* Enable EFR */
  1593			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
  1594					     SC16IS7XX_LCR_CONF_MODE_B);
  1595	
  1596			regcache_cache_bypass(regmaps[i], true);
  1597	
  1598			/* Enable write access to enhanced features */
  1599			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFR_REG,
  1600					     SC16IS7XX_EFR_ENABLE_BIT);
  1601	
  1602			regcache_cache_bypass(regmaps[i], false);
  1603	
  1604			/* Restore access to general registers */
  1605			sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG, 0x00);
  1606	
  1607			/* Go to suspend mode */
  1608			sc16is7xx_power(&s->p[i].port, 0);
  1609		}
  1610	
  1611		sc16is7xx_setup_irda_ports(s);
  1612	
  1613		ret = sc16is7xx_setup_mctrl_ports(s, regmaps[0]);
  1614		if (ret)
  1615			goto out_ports;
  1616
Hugo Villeneuve Feb. 8, 2024, 3:26 p.m. UTC | #3 
On Tue,  6 Feb 2024 16:42:07 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:

> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Before, sc16is7xx_lines was checked for a free (zero) bit first, and then
> later it was set only if UART port registration succeeded.
> 
> Now that sc16is7xx_lines is shared for the I2C and SPI drivers, make sure
> it is reserved and modified atomically, and use a new variable to hold the
> status of UART port regisration.
> 
> Remove need to check for previous port registration in sc16is7xx_remove(),
> because if sc16is7xx_probe() succeeded, we are guaranteed to have
> successfully registered both ports.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 5b53c88b7133..5073ebfc45bf 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1468,6 +1468,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	u32 uartclk = 0;
>  	int i, ret;
>  	struct sc16is7xx_port *s;
> +	bool port_registered[SC16IS7XX_MAX_PORTS];
>  
>  	for (i = 0; i < devtype->nr_uart; i++)
>  		if (IS_ERR(regmaps[i]))
> @@ -1533,8 +1534,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		     SC16IS7XX_IOCONTROL_SRESET_BIT);
>  
>  	for (i = 0; i < devtype->nr_uart; ++i) {
> -		s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines,
> -							SC16IS7XX_MAX_DEVS);
> +		port_registered[i] = false;

I just realised that this will not work. All port_registered[]
members need to be initialized before entering the loop.

I will add this (for V2) before the loop:

  memset(port_registered, 0, sizeof(port_registered));

Hugo Villeneuve


> +
> +		s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
> +						     SC16IS7XX_MAX_DEVS);
>  		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
>  			ret = -ERANGE;
>  			goto out_ports;
> @@ -1584,7 +1587,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		if (ret)
>  			goto out_ports;
>  
> -		set_bit(s->p[i].port.line, sc16is7xx_lines);
> +		port_registered[i] = true;
>  
>  		/* Enable EFR */
>  		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
> @@ -1642,9 +1645,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  #endif
>  
>  out_ports:
> -	for (i = 0; i < devtype->nr_uart; i++)
> -		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
> +	for (i = 0; i < devtype->nr_uart; i++) {
> +		clear_bit(s->p[i].port.line, sc16is7xx_lines);
> +		if (port_registered[i])
>  			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
> +	}
>  
>  	kthread_stop(s->kworker_task);
>  
> @@ -1667,8 +1672,8 @@ void sc16is7xx_remove(struct device *dev)
>  
>  	for (i = 0; i < s->devtype->nr_uart; i++) {
>  		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
> -		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
> -			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
> +		clear_bit(s->p[i].port.line, sc16is7xx_lines);
> +		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
>  		sc16is7xx_power(&s->p[i].port, 0);
>  	}
>  
> -- 
> 2.39.2
> 
>
diff mbox series

Patch

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5b53c88b7133..5073ebfc45bf 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1468,6 +1468,7 @@  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
+	bool port_registered[SC16IS7XX_MAX_PORTS];
 
 	for (i = 0; i < devtype->nr_uart; i++)
 		if (IS_ERR(regmaps[i]))
@@ -1533,8 +1534,10 @@  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		     SC16IS7XX_IOCONTROL_SRESET_BIT);
 
 	for (i = 0; i < devtype->nr_uart; ++i) {
-		s->p[i].port.line = find_first_zero_bit(sc16is7xx_lines,
-							SC16IS7XX_MAX_DEVS);
+		port_registered[i] = false;
+
+		s->p[i].port.line = find_and_set_bit(sc16is7xx_lines,
+						     SC16IS7XX_MAX_DEVS);
 		if (s->p[i].port.line >= SC16IS7XX_MAX_DEVS) {
 			ret = -ERANGE;
 			goto out_ports;
@@ -1584,7 +1587,7 @@  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		if (ret)
 			goto out_ports;
 
-		set_bit(s->p[i].port.line, sc16is7xx_lines);
+		port_registered[i] = true;
 
 		/* Enable EFR */
 		sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_LCR_REG,
@@ -1642,9 +1645,11 @@  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 #endif
 
 out_ports:
-	for (i = 0; i < devtype->nr_uart; i++)
-		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
+	for (i = 0; i < devtype->nr_uart; i++) {
+		clear_bit(s->p[i].port.line, sc16is7xx_lines);
+		if (port_registered[i])
 			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
+	}
 
 	kthread_stop(s->kworker_task);
 
@@ -1667,8 +1672,8 @@  void sc16is7xx_remove(struct device *dev)
 
 	for (i = 0; i < s->devtype->nr_uart; i++) {
 		kthread_cancel_delayed_work_sync(&s->p[i].ms_work);
-		if (test_and_clear_bit(s->p[i].port.line, sc16is7xx_lines))
-			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
+		clear_bit(s->p[i].port.line, sc16is7xx_lines);
+		uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 		sc16is7xx_power(&s->p[i].port, 0);
 	}