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

Message ID 20230601201844.3739926-6-hugo@hugovil.com
State New
Headers
Series serial: sc16is7xx: fix GPIO regression and rs485 improvements |

Commit Message

Hugo Villeneuve June 1, 2023, 8:18 p.m. UTC
  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>
---
 drivers/tty/serial/sc16is7xx.c | 102 ++++++++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 21 deletions(-)
  

Comments

Andy Shevchenko June 1, 2023, 9:30 p.m. UTC | #1
Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> 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

Almost good, a few remarks and if addressed as suggested,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you!

...

> +	if (!s->gpio_valid_mask)

I would use == 0, but it's up to you. Both will work equally.

> +		return 0;

...

> +static int sc16is7xx_setup_mctrl_ports(struct device *dev)

Not sure why int if you always return an unsigned value.
Otherwise return an error code when it's no defined mask
and check it in the caller.

> +{
> +	struct sc16is7xx_port *s = dev_get_drvdata(dev);
> +	int i;
> +	int ret;
> +	int count;
> +	u32 mctrl_port[2];
> +	u8 mctrl_mask = 0;

I would return 0 directly in the first two cases and split an assignment closer
to the first user.

> +	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
> +	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> +		return mctrl_mask;

		return 0;

> +	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
> +					     mctrl_port, count);
> +	if (ret)
> +		return mctrl_mask;

		return 0;


	mctrl_mask = 0;

> +	for (i = 0; i < count; i++) {
> +		/* Use GPIO lines as modem control lines */
> +		if (mctrl_port[i] == 0)
> +			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +		else if (mctrl_port[i] == 1)
> +			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +	}

> +	if (!mctrl_mask)
> +		return mctrl_mask;

Maybe positive one?

	if (mctrl_mask)
		regmap_update_bits(...);

> +	regmap_update_bits(s->regmap,
> +			   SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> +			   SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> +			   SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask);
> +
> +	return mctrl_mask;
> +}

...

>  	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
>  	unsigned int val;
> +	u8 mctrl_mask = 0;

This assignment is redundant, so you simply can define it

>  	u32 uartclk = 0;

	u8 mctrl_mask;

>  	int i, ret;
>  	struct sc16is7xx_port *s;
  
Hugo Villeneuve June 2, 2023, 12:41 a.m. UTC | #2
On Fri, 2 Jun 2023 00:30:14 +0300
andy.shevchenko@gmail.com wrote:

> Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> > 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
> 
> Almost good, a few remarks and if addressed as suggested,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> Thank you!
> 
> ...
> 
> > +	if (!s->gpio_valid_mask)
> 
> I would use == 0, but it's up to you. Both will work equally.

Hi,
done.

> > +		return 0;
> 
> ...
> 
> > +static int sc16is7xx_setup_mctrl_ports(struct device *dev)
> 
> Not sure why int if you always return an unsigned value.
> Otherwise return an error code when it's no defined mask
> and check it in the caller.

Changed return type to u8.

 
> > +{
> > +	struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > +	int i;
> > +	int ret;
> > +	int count;
> > +	u32 mctrl_port[2];
> > +	u8 mctrl_mask = 0;
> 
> I would return 0 directly in the first two cases and split an assignment closer
> to the first user.
> 
> > +	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
> > +	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> > +		return mctrl_mask;
> 
> 		return 0;

Done.


> > +	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
> > +					     mctrl_port, count);
> > +	if (ret)
> > +		return mctrl_mask;
> 
> 		return 0;

Done.


> 	mctrl_mask = 0;

Done.

 
> > +	for (i = 0; i < count; i++) {
> > +		/* Use GPIO lines as modem control lines */
> > +		if (mctrl_port[i] == 0)
> > +			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +		else if (mctrl_port[i] == 1)
> > +			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +	}
> 
> > +	if (!mctrl_mask)
> > +		return mctrl_mask;
> 
> Maybe positive one?
> 	if (mctrl_mask)
> 		regmap_update_bits(...);

I used negative to save on indentation, but it also fits by converting it to positive, so done.

 
> > +	regmap_update_bits(s->regmap,
> > +			   SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > +			   SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> > +			   SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask);
> > +
> > +	return mctrl_mask;
> > +}
> 
> ...
> 
> >  	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
> >  	unsigned int val;
> > +	u8 mctrl_mask = 0;
> 
> This assignment is redundant, so you simply can define it
> 
> >  	u32 uartclk = 0;
> 
> 	u8 mctrl_mask;

Done.

I will send a V7 soon with your Reviewed-by tag.

Thank you,
Hugo.

 
> >  	int i, ret;
> >  	struct sc16is7xx_port *s;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
  
Hugo Villeneuve June 2, 2023, 1:25 a.m. UTC | #3
On Thu, 1 Jun 2023 20:41:40 -0400
Hugo Villeneuve <hugo@hugovil.com> wrote:

> On Fri, 2 Jun 2023 00:30:14 +0300
> andy.shevchenko@gmail.com wrote:
> 
> > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> > > 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
> > 
> > Almost good, a few remarks and if addressed as suggested,
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > 
> > Thank you!
> > 
> > ...
> > 
> > > +	if (!s->gpio_valid_mask)
> > 
> > I would use == 0, but it's up to you. Both will work equally.
> 
> Hi,
> done.
> 
> > > +		return 0;
> > 
> > ...
> > 
> > > +static int sc16is7xx_setup_mctrl_ports(struct device *dev)
> > 
> > Not sure why int if you always return an unsigned value.
> > Otherwise return an error code when it's no defined mask
> > and check it in the caller.
> 
> Changed return type to u8.
> 
>  
> > > +{
> > > +	struct sc16is7xx_port *s = dev_get_drvdata(dev);
> > > +	int i;
> > > +	int ret;
> > > +	int count;
> > > +	u32 mctrl_port[2];
> > > +	u8 mctrl_mask = 0;
> > 
> > I would return 0 directly in the first two cases and split an assignment closer
> > to the first user.
> > 
> > > +	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
> > > +	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
> > > +		return mctrl_mask;
> > 
> > 		return 0;
> 
> Done.
> 
> 
> > > +	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
> > > +					     mctrl_port, count);
> > > +	if (ret)
> > > +		return mctrl_mask;
> > 
> > 		return 0;
> 
> Done.
> 
> 
> > 	mctrl_mask = 0;
> 
> Done.
> 
>  
> > > +	for (i = 0; i < count; i++) {
> > > +		/* Use GPIO lines as modem control lines */
> > > +		if (mctrl_port[i] == 0)
> > > +			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > +		else if (mctrl_port[i] == 1)
> > > +			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > +	}
> > 
> > > +	if (!mctrl_mask)
> > > +		return mctrl_mask;
> > 
> > Maybe positive one?
> > 	if (mctrl_mask)
> > 		regmap_update_bits(...);
> 
> I used negative to save on indentation, but it also fits by converting it to positive, so done.
> 
>  
> > > +	regmap_update_bits(s->regmap,
> > > +			   SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > > +			   SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> > > +			   SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask);
> > > +
> > > +	return mctrl_mask;
> > > +}
> > 
> > ...
> > 
> > >  	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
> > >  	unsigned int val;
> > > +	u8 mctrl_mask = 0;
> > 
> > This assignment is redundant, so you simply can define it
> > 
> > >  	u32 uartclk = 0;
> > 
> > 	u8 mctrl_mask;
> 
> Done.
> 
> I will send a V7 soon with your Reviewed-by tag.

Hi Andy,
Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:).

Hugo.
  
Andy Shevchenko June 2, 2023, 12:58 p.m. UTC | #4
On Fri, Jun 2, 2023 at 4:25 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Thu, 1 Jun 2023 20:41:40 -0400
> Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Fri, 2 Jun 2023 00:30:14 +0300
> > andy.shevchenko@gmail.com wrote:
> > > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:

...

> > > Maybe positive one?
> > >     if (mctrl_mask)
> > >             regmap_update_bits(...);
> >
> > I used negative to save on indentation, but it also fits by converting it to positive, so done.

I understand, but in this case it is slightly more weird to have
negative conditional and in either case return the value of the local
variable.

...

> Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:).

They look fine to me, but Greg is the maintainer, he decides if it's
really okay or not.
  
Hugo Villeneuve June 2, 2023, 1:06 p.m. UTC | #5
On Fri, 2 Jun 2023 15:58:43 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jun 2, 2023 at 4:25 AM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Thu, 1 Jun 2023 20:41:40 -0400
> > Hugo Villeneuve <hugo@hugovil.com> wrote:
> > > On Fri, 2 Jun 2023 00:30:14 +0300
> > > andy.shevchenko@gmail.com wrote:
> > > > Thu, Jun 01, 2023 at 04:18:40PM -0400, Hugo Villeneuve kirjoitti:
> 
> ...
> 
> > > > Maybe positive one?
> > > >     if (mctrl_mask)
> > > >             regmap_update_bits(...);
> > >
> > > I used negative to save on indentation, but it also fits by converting it to positive, so done.
> 
> I understand, but in this case it is slightly more weird to have
> negative conditional and in either case return the value of the local
> variable.

Yes.

> > Greg did not yet respond to my email about the proposed tags, but if the new order of the patches and the stable tags I added seems ok to you, I will resend V7. It will then probably easier for Greg to comment directly on V7 for the stable tags (Cc:).
> 
> They look fine to me, but Greg is the maintainer, he decides if it's
> really okay or not.

Then I will submit V7 now.

Thank you,
Hugo.
  

Patch

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7d50674d2d0e..ad6b9d613b33 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)
+		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,43 @@  static int sc16is7xx_setup_gpio_chip(struct device *dev)
 }
 #endif
 
+static int sc16is7xx_setup_mctrl_ports(struct device *dev)
+{
+	struct sc16is7xx_port *s = dev_get_drvdata(dev);
+	int i;
+	int ret;
+	int count;
+	u32 mctrl_port[2];
+	u8 mctrl_mask = 0;
+
+	count = device_property_count_u32(dev, "nxp,modem-control-line-ports");
+	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+		return mctrl_mask;
+
+	ret = device_property_read_u32_array(dev, "nxp,modem-control-line-ports",
+					     mctrl_port, count);
+	if (ret)
+		return mctrl_mask;
+
+	for (i = 0; i < count; i++) {
+		/* Use GPIO lines as modem control lines */
+		if (mctrl_port[i] == 0)
+			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+		else if (mctrl_port[i] == 1)
+			mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+	}
+
+	if (!mctrl_mask)
+		return mctrl_mask;
+
+	regmap_update_bits(s->regmap,
+			   SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+			   SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+			   SC16IS7XX_IOCONTROL_MODEM_B_BIT, mctrl_mask);
+
+	return mctrl_mask;
+}
+
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
 	.delay_rts_before_send = 1,
@@ -1383,6 +1446,7 @@  static int sc16is7xx_probe(struct device *dev,
 {
 	unsigned long freq = 0, *pfreq = dev_get_platdata(dev);
 	unsigned int val;
+	u8 mctrl_mask = 0;
 	u32 uartclk = 0;
 	int i, ret;
 	struct sc16is7xx_port *s;
@@ -1478,12 +1542,6 @@  static int sc16is7xx_probe(struct device *dev,
 				     SC16IS7XX_EFCR_RXDISABLE_BIT |
 				     SC16IS7XX_EFCR_TXDISABLE_BIT);
 
-		/* Use GPIO lines as modem status registers */
-		if (devtype->has_mctrl)
-			sc16is7xx_port_write(&s->p[i].port,
-					     SC16IS7XX_IOCONTROL_REG,
-					     SC16IS7XX_IOCONTROL_MODEM_BIT);
-
 		/* Initialize kthread work structs */
 		kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
 		kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
@@ -1521,8 +1579,10 @@  static int sc16is7xx_probe(struct device *dev,
 				s->p[u].irda_mode = true;
 	}
 
+	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
+
 #ifdef CONFIG_GPIOLIB
-	ret = sc16is7xx_setup_gpio_chip(dev);
+	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
 	if (ret)
 		goto out_thread;
 #endif
@@ -1547,7 +1607,7 @@  static int sc16is7xx_probe(struct device *dev,
 		return 0;
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 
 out_thread:
@@ -1573,7 +1633,7 @@  static void sc16is7xx_remove(struct device *dev)
 	int i;
 
 #ifdef CONFIG_GPIOLIB
-	if (s->devtype->nr_gpio)
+	if (s->gpio_valid_mask)
 		gpiochip_remove(&s->gpio);
 #endif