From: Hugo Villeneuve <[email protected]>
Add macro to hold the maximum number of UART ports per IC/device.
Signed-off-by: Hugo Villeneuve <[email protected]>
---
drivers/tty/serial/sc16is7xx.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
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);
--
2.39.2
On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <[email protected]>
>
> 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.
--
With Best Regards,
Andy Shevchenko
On Wed, 20 Dec 2023 17:50:34 +0200
Andy Shevchenko <[email protected]> wrote:
> On Tue, Dec 19, 2023 at 12:18:53PM -0500, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <[email protected]>
> >
> > 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
On Thu, Dec 21, 2023 at 11:41:03AM -0500, Hugo Villeneuve wrote:
> On Wed, 20 Dec 2023 17:50:34 +0200
> Andy Shevchenko <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Thu, 21 Dec 2023 18:55:17 +0200
Andy Shevchenko <[email protected]> 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 <[email protected]> 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