diff mbox series

[09/18] serial: sc16is7xx: add macro for max number of UART ports

Message ID 20231219171903.3530985-10-hugo@hugovil.com
State New
Headers show
Series serial: sc16is7xx: fixes, cleanups and improvements | expand

Commit Message

Hugo Villeneuve Dec. 19, 2023, 5:18 p.m. UTC
From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Add macro to hold the maximum number of UART ports per IC/device.

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

Comments

Andy Shevchenko Dec. 20, 2023, 3:50 p.m. UTC | #1
On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Add macro to hold the maximum number of UART ports per IC/device.

...

> -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)

ARRAY_SIZE() is more robust than this. What if you change to support different
devices where this won't be as defined?

>  		return;

...

> -	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
>  		return 0;

Ditto.

...

> +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);

Not sure about this, perhaps it's fine.

Otherwise looks reasonable.
Hugo Villeneuve Dec. 21, 2023, 4:41 p.m. UTC | #2
On Wed, 20 Dec 2023 17:50:34 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Add macro to hold the maximum number of UART ports per IC/device.
> 
> ...
> 
> > -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> 
> ARRAY_SIZE() is more robust than this. What if you change to support different
> devices where this won't be as defined?

Hi,
not sure that I understand your point, because SC16IS7XX_MAX_PORTS is
the maximum for all devices supported by this driver. The irda_port
array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS,
even if the device that we are probing has only one port for example.

But I can change it back to ARRAY_SIZE(irda_port) if you want.

> 
> >  		return;
> 
> ...
> 
> > -	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> >  		return 0;
> 
> Ditto.
> 
> ...
> 
> > +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
> 
> Not sure about this, perhaps it's fine.

This check is only there if we add support for a new device and we
incorrectly set nr_uart to an incorrect value, which will cause other
problems anyway, of course :)

This could be removed.

Hugo Villeneuve
Andy Shevchenko Dec. 21, 2023, 4:55 p.m. UTC | #3
On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:50:34 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:

...

> > > -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> > > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> > 
> > ARRAY_SIZE() is more robust than this. What if you change to support different
> > devices where this won't be as defined?
> 
> not sure that I understand your point, because SC16IS7XX_MAX_PORTS is
> the maximum for all devices supported by this driver. The irda_port
> array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS,
> even if the device that we are probing has only one port for example.

For current models of the device, yes. Who knows the future?
Also, ARRAY_SIZE() make it less points to update if ever needed.

> But I can change it back to ARRAY_SIZE(irda_port) if you want.

Please change it back.

> > >  		return;

...

> > > +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
> > 
> > Not sure about this, perhaps it's fine.
> 
> This check is only there if we add support for a new device and we
> incorrectly set nr_uart to an incorrect value, which will cause other
> problems anyway, of course :)
> 
> This could be removed.

Let's remove. We can add it back in case something like this (quite unlikely)
happens.
Hugo Villeneuve Dec. 21, 2023, 5 p.m. UTC | #4
On Thu, 21 Dec 2023 18:55:17 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote:
> > On Wed, 20 Dec 2023 17:50:34 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> 
> ...
> 
> > > > -	if (count < 0 || count > ARRAY_SIZE(irda_port))
> > > > +	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
> > > 
> > > ARRAY_SIZE() is more robust than this. What if you change to support different
> > > devices where this won't be as defined?
> > 
> > not sure that I understand your point, because SC16IS7XX_MAX_PORTS is
> > the maximum for all devices supported by this driver. The irda_port
> > array always has a fixed number of elements set to SC16IS7XX_MAX_PORTS,
> > even if the device that we are probing has only one port for example.
> 
> For current models of the device, yes. Who knows the future?
> Also, ARRAY_SIZE() make it less points to update if ever needed.
> 
> > But I can change it back to ARRAY_SIZE(irda_port) if you want.
> 
> Please change it back.
> 
> > > >  		return;
> 
> ...
> 
> > > > +	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
> > > 
> > > Not sure about this, perhaps it's fine.
> > 
> > This check is only there if we add support for a new device and we
> > incorrectly set nr_uart to an incorrect value, which will cause other
> > problems anyway, of course :)
> > 
> > This could be removed.
> 
> Let's remove. We can add it back in case something like this (quite unlikely)
> happens.

Ok, will do both for v2.

Hugo Villeneuve
diff mbox series

Patch

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 3fb99b6929f3..29844e2eefc5 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -28,6 +28,7 @@ 
 
 #define SC16IS7XX_NAME			"sc16is7xx"
 #define SC16IS7XX_MAX_DEVS		8
+#define SC16IS7XX_MAX_PORTS		2 /* Maximum number of UART ports per IC. */
 
 /* SC16IS7XX register definitions */
 #define SC16IS7XX_RHR_REG		(0x00) /* RX FIFO */
@@ -1396,11 +1397,11 @@  static void sc16is7xx_setup_irda_ports(struct sc16is7xx_port *s)
 	int i;
 	int ret;
 	int count;
-	u32 irda_port[2];
+	u32 irda_port[SC16IS7XX_MAX_PORTS];
 	struct device *dev = s->p[0].port.dev;
 
 	count = device_property_count_u32(dev, "irda-mode-ports");
-	if (count < 0 || count > ARRAY_SIZE(irda_port))
+	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
 		return;
 
 	ret = device_property_read_u32_array(dev, "irda-mode-ports",
@@ -1423,11 +1424,11 @@  static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s,
 	int i;
 	int ret;
 	int count;
-	u32 mctrl_port[2];
+	u32 mctrl_port[SC16IS7XX_MAX_PORTS];
 	struct device *dev = s->p[0].port.dev;
 
 	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
-	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+	if (count < 0 || count > SC16IS7XX_MAX_PORTS)
 		return 0;
 
 	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
@@ -1471,6 +1472,8 @@  static int sc16is7xx_probe(struct device *dev,
 	int i, ret;
 	struct sc16is7xx_port *s;
 
+	WARN_ON(devtype->nr_uart > SC16IS7XX_MAX_PORTS);
+
 	for (i = 0; i < devtype->nr_uart; i++)
 		if (IS_ERR(regmaps[i]))
 			return PTR_ERR(regmaps[i]);
@@ -1730,7 +1733,7 @@  static unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id)
 static int sc16is7xx_spi_probe(struct spi_device *spi)
 {
 	const struct sc16is7xx_devtype *devtype;
-	struct regmap *regmaps[2];
+	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
 	unsigned int i;
 	int ret;
 
@@ -1800,7 +1803,7 @@  MODULE_ALIAS("spi:sc16is7xx");
 static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
 {
 	const struct sc16is7xx_devtype *devtype;
-	struct regmap *regmaps[2];
+	struct regmap *regmaps[SC16IS7XX_MAX_PORTS];
 	unsigned int i;
 
 	devtype = (struct sc16is7xx_devtype *)i2c_get_match_data(i2c);