diff mbox series

[v3,08/11] serial: sc16is7xx: fix regression with GPIO configuration

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

Commit Message

Hugo Villeneuve May 25, 2023, 4:03 a.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.

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

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

We also now reduce the number of exported GPIOs according to the
modem-status-line-port 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")
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
 drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 21 deletions(-)

Comments

Andy Shevchenko May 25, 2023, 11:19 a.m. UTC | #1 
Thu, May 25, 2023 at 12:03:22AM -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.

> This new patch allows to specify GPIO or modem control line function
> in the device tree, and for each of the ports (A or B).

Imperative mood as stated in documentation, please.
Like "Allow to specify...".

> This is done by using the new device-tree property named
> "modem-control-line-ports" (property added in separate patch).
> 
> We also now reduce the number of exported GPIOs according to the
> modem-status-line-port 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

...

> +#ifdef CONFIG_GPIOLIB

I'm wondering if we can avoid adding new ifdefferies...

> +	s->gpio_configured = devtype->nr_gpio;

The name of the variable is a bit vague WRT its content.
Shouldn't be as simple as the rvalue, i.e. s->nr_gpio?

> +#endif /* CONFIG_GPIOLIB */

...

> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u)

The driver so far is agnostic to property provider. Please keep it that way,
i.e. no of_ APIs.

> +			if (u < devtype->nr_uart) {

Hmm... What other can it be?

> +				/* Use GPIO lines as modem control lines */
> +				if (u == 0)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +				else if (u == 1)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +
> +#ifdef CONFIG_GPIOLIB
> +				if (s->gpio_configured >=
> +				    SC16IS7XX_GPIOS_PER_BANK)

On one line it will be better to read. Esp. taking into account the above remark.

> +					s->gpio_configured -=
> +						SC16IS7XX_GPIOS_PER_BANK;

Ditto.

> +#endif /* CONFIG_GPIOLIB */
> +			}
Ilpo Järvinen May 25, 2023, 12:03 p.m. UTC | #2 
On Thu, 25 May 2023, 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.
> 
> This new patch allows to specify GPIO or modem control line function
> in the device tree, and for each of the ports (A or B).
> 
> This is done by using the new device-tree property named
> "modem-control-line-ports" (property added in separate patch).
> 
> We also now reduce the number of exported GPIOs according to the
> modem-status-line-port 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")
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index a5d8af0f6da0..97ec532a0a19 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -236,7 +236,8 @@
>  
>  /* IOControl register bits (Only 75x/76x) */
>  #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;
> +	int				gpio_configured;
>  #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)
> @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev,
>  		return -ENOMEM;
>  	}
>  
> +#ifdef CONFIG_GPIOLIB
> +	s->gpio_configured = devtype->nr_gpio;
> +#endif /* CONFIG_GPIOLIB */
> +
>  	/* Always ask for fixed clock rate from a property. */
>  	device_property_read_u32(dev, "clock-frequency", &uartclk);
>  
> @@ -1473,12 +1474,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);
> @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev,
>  					 prop, p, u)
>  			if (u < devtype->nr_uart)
>  				s->p[u].irda_mode = true;
> +
> +		val = 0;
> +
> +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> +					 prop, p, u)
> +			if (u < devtype->nr_uart) {

Reverse logic + use continue. It will help with the indentation levels.

> +				/* Use GPIO lines as modem control lines */
> +				if (u == 0)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> +				else if (u == 1)
> +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> +
> +#ifdef CONFIG_GPIOLIB
> +				if (s->gpio_configured >=
> +				    SC16IS7XX_GPIOS_PER_BANK)
> +					s->gpio_configured -=
> +						SC16IS7XX_GPIOS_PER_BANK;
> +#endif /* CONFIG_GPIOLIB */
> +			}

Please use braces for of_property_for_each_u32 block.

> +
> +		if (val)
> +			regmap_update_bits(
> +				s->regmap,
> +				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> +				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> +				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
>  	}
>  
>  #ifdef CONFIG_GPIOLIB
> -	if (devtype->nr_gpio) {
> +	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
> +
> +	if (s->gpio_configured) {
>  		/* Setup GPIO controller */
>  		s->gpio.owner		 = THIS_MODULE;
>  		s->gpio.parent		 = dev;
> @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		s->gpio.direction_output = sc16is7xx_gpio_direction_output;
>  		s->gpio.set		 = sc16is7xx_gpio_set;
>  		s->gpio.base		 = -1;
> -		s->gpio.ngpio		 = devtype->nr_gpio;
> +		s->gpio.ngpio		 = s->gpio_configured;
>  		s->gpio.can_sleep	 = 1;
>  		ret = gpiochip_add_data(&s->gpio, s);
>  		if (ret)
> @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		return 0;
>  
>  #ifdef CONFIG_GPIOLIB
> -	if (devtype->nr_gpio)
> +	if (s->gpio_configured)
>  		gpiochip_remove(&s->gpio);
>  
>  out_thread:
> @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev)
>  	int i;
>  
>  #ifdef CONFIG_GPIOLIB
> -	if (s->devtype->nr_gpio)
> +	if (s->gpio_configured)
>  		gpiochip_remove(&s->gpio);
>  #endif
>  
>
Hugo Villeneuve May 25, 2023, 3:02 p.m. UTC | #3 
On Thu, 25 May 2023 14:19:52 +0300
andy.shevchenko@gmail.com wrote:

> Thu, May 25, 2023 at 12:03:22AM -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.
> 
> > This new patch allows to specify GPIO or modem control line function
> > in the device tree, and for each of the ports (A or B).
> 
> Imperative mood as stated in documentation, please.
> Like "Allow to specify...".
> 
> > This is done by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > We also now reduce the number of exported GPIOs according to the
> > modem-status-line-port DT property.

Just noticed a mistake:
s/modem-status-line-port/modem-control-line-ports

> > 
> > 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
> 
> ...
> 
> > +#ifdef CONFIG_GPIOLIB
> 
> I'm wondering if we can avoid adding new ifdefferies...

I am simply following waht was already done in the existing driver.

Are you suggesting that we need to remove all these #defines? If not, what exactly do you suggest?


> > +	s->gpio_configured = devtype->nr_gpio;
> 
> The name of the variable is a bit vague WRT its content.
> Shouldn't be as simple as the rvalue, i.e. s->nr_gpio?

Maybe the name could be improved (and/or comments).

devtype->nr_gpio is the maximum "theoretical" number of GPIOs supported by the chip.

s->gpio_configured is the number of GPIOs that are configured or requested according to the presence (or not) of the modem-control-line-ports property.

I wanted to avoid using the same name to avoid potential confusion.

Maybe devtype->nr_gpio could be renamed to devtype->nr_gpio_max and s->gpio_configured to s->nr_gpio_requested or s->nr_gpio_configured?


> > +#endif /* CONFIG_GPIOLIB */
> 
> ...
> 
> > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > +					 prop, p, u)
> 
> The driver so far is agnostic to property provider. Please keep it that way,
> i.e. no of_ APIs.

The driver, before my patches, was already using the exact same function of_property_for_each_u32() to process the irda-mode-ports property, so I don't understand your comment.

But what do you suggest instead of of_property_for_each_u32()? And do we need to change it also for processing the irda-mode-ports property?


> > +			if (u < devtype->nr_uart) {
> 
> Hmm... What other can it be?

Again, this is similar to the handling of the irda-mode-ports property.

But I am not sure I understand your question/concern?

I think this check is important, because if someone puts the following property in a DT:

    nxp,modem-control-line-ports = <0 1>;

but the variant only supports 1 port, then the check is usefull, no?


> 
> > +				/* Use GPIO lines as modem control lines */
> > +				if (u == 0)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +				else if (u == 1)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +
> > +#ifdef CONFIG_GPIOLIB
> > +				if (s->gpio_configured >=
> > +				    SC16IS7XX_GPIOS_PER_BANK)
> 
> On one line it will be better to read. Esp. taking into account the above remark.

Fixed.

 
> > +					s->gpio_configured -=
> > +						SC16IS7XX_GPIOS_PER_BANK;
> 
> Ditto.

Fixed.

> 
> > +#endif /* CONFIG_GPIOLIB */
> > +			}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
>
Hugo Villeneuve May 25, 2023, 3:19 p.m. UTC | #4 
On Thu, 25 May 2023 15:03:41 +0300 (EEST)
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> On Thu, 25 May 2023, 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.
> > 
> > This new patch allows to specify GPIO or modem control line function
> > in the device tree, and for each of the ports (A or B).
> > 
> > This is done by using the new device-tree property named
> > "modem-control-line-ports" (property added in separate patch).
> > 
> > We also now reduce the number of exported GPIOs according to the
> > modem-status-line-port 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")
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 65 +++++++++++++++++++++++-----------
> >  1 file changed, 44 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index a5d8af0f6da0..97ec532a0a19 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -236,7 +236,8 @@
> >  
> >  /* IOControl register bits (Only 75x/76x) */
> >  #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;
> > +	int				gpio_configured;
> >  #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)
> > @@ -1396,6 +1393,10 @@ static int sc16is7xx_probe(struct device *dev,
> >  		return -ENOMEM;
> >  	}
> >  
> > +#ifdef CONFIG_GPIOLIB
> > +	s->gpio_configured = devtype->nr_gpio;
> > +#endif /* CONFIG_GPIOLIB */
> > +
> >  	/* Always ask for fixed clock rate from a property. */
> >  	device_property_read_u32(dev, "clock-frequency", &uartclk);
> >  
> > @@ -1473,12 +1474,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);
> > @@ -1514,10 +1509,38 @@ static int sc16is7xx_probe(struct device *dev,
> >  					 prop, p, u)
> >  			if (u < devtype->nr_uart)
> >  				s->p[u].irda_mode = true;
> > +
> > +		val = 0;
> > +
> > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > +					 prop, p, u)
> > +			if (u < devtype->nr_uart) {
> 
> Reverse logic + use continue. It will help with the indentation levels.

Good suggestion, done.

 
> > +				/* Use GPIO lines as modem control lines */
> > +				if (u == 0)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > +				else if (u == 1)
> > +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > +
> > +#ifdef CONFIG_GPIOLIB
> > +				if (s->gpio_configured >=
> > +				    SC16IS7XX_GPIOS_PER_BANK)
> > +					s->gpio_configured -=
> > +						SC16IS7XX_GPIOS_PER_BANK;
> > +#endif /* CONFIG_GPIOLIB */
> > +			}
> 
> Please use braces for of_property_for_each_u32 block.

Done

 
> > +
> > +		if (val)
> > +			regmap_update_bits(
> > +				s->regmap,
> > +				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
> > +				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
> > +				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
> >  	}
> >  
> >  #ifdef CONFIG_GPIOLIB
> > -	if (devtype->nr_gpio) {
> > +	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
> > +
> > +	if (s->gpio_configured) {
> >  		/* Setup GPIO controller */
> >  		s->gpio.owner		 = THIS_MODULE;
> >  		s->gpio.parent		 = dev;
> > @@ -1527,7 +1550,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  		s->gpio.direction_output = sc16is7xx_gpio_direction_output;
> >  		s->gpio.set		 = sc16is7xx_gpio_set;
> >  		s->gpio.base		 = -1;
> > -		s->gpio.ngpio		 = devtype->nr_gpio;
> > +		s->gpio.ngpio		 = s->gpio_configured;
> >  		s->gpio.can_sleep	 = 1;
> >  		ret = gpiochip_add_data(&s->gpio, s);
> >  		if (ret)
> > @@ -1555,7 +1578,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  		return 0;
> >  
> >  #ifdef CONFIG_GPIOLIB
> > -	if (devtype->nr_gpio)
> > +	if (s->gpio_configured)
> >  		gpiochip_remove(&s->gpio);
> >  
> >  out_thread:
> > @@ -1581,7 +1604,7 @@ static void sc16is7xx_remove(struct device *dev)
> >  	int i;
> >  
> >  #ifdef CONFIG_GPIOLIB
> > -	if (s->devtype->nr_gpio)
> > +	if (s->gpio_configured)
> >  		gpiochip_remove(&s->gpio);
> >  #endif
> >  
> > 
> 
> -- 
>  i.
> 
>
Andy Shevchenko May 26, 2023, 6:34 p.m. UTC | #5 
Thu, May 25, 2023 at 11:02:55AM -0400, Hugo Villeneuve kirjoitti:
> On Thu, 25 May 2023 14:19:52 +0300
> andy.shevchenko@gmail.com wrote:
> > Thu, May 25, 2023 at 12:03:22AM -0400, Hugo Villeneuve kirjoitti:

...

> > I'm wondering if we can avoid adding new ifdefferies...
> 
> I am simply following waht was already done in the existing driver.
> 
> Are you suggesting that we need to remove all these #defines? If not, what
> exactly do you suggest?

I was wondering and have nothing to suggest here. It seems a burden we have to
cope with for now.

> > > +	s->gpio_configured = devtype->nr_gpio;
> > 
> > The name of the variable is a bit vague WRT its content.
> > Shouldn't be as simple as the rvalue, i.e. s->nr_gpio?
> 
> Maybe the name could be improved (and/or comments).
> 
> devtype->nr_gpio is the maximum "theoretical" number of GPIOs supported by
> the chip.
> 
> s->gpio_configured is the number of GPIOs that are configured or requested
> according to the presence (or not) of the modem-control-line-ports property.
> 
> I wanted to avoid using the same name to avoid potential confusion.
> 
> Maybe devtype->nr_gpio could be renamed to devtype->nr_gpio_max and
> s->gpio_configured to s->nr_gpio_requested or s->nr_gpio_configured?

Maybe, but first try the approach with valid mask being involved. It may be
that we won't need this variable at all.

...

> > > +		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
> > > +					 prop, p, u)
> > 
> > The driver so far is agnostic to property provider. Please keep it that way,
> > i.e. no of_ APIs.
> 
> The driver, before my patches, was already using the exact same function
> of_property_for_each_u32() to process the irda-mode-ports property, so I
> don't understand your comment.

This is unfortunate. I missed that one, but i don't care about IrDA so much.

> But what do you suggest instead of of_property_for_each_u32()? And do we need
> to change it also for processing the irda-mode-ports property?

device_property_read_u32_array().

Independently on the IrDA case, this one is more important and would have
consequences if we avoid agnostic APIs.

...

> > > +			if (u < devtype->nr_uart) {
> > 
> > Hmm... What other can it be?
> 
> Again, this is similar to the handling of the irda-mode-ports property.
> 
> But I am not sure I understand your question/concern?
> 
> I think this check is important, because if someone puts the following
> property in a DT:
> 
>     nxp,modem-control-line-ports = <0 1>;
> 
> but the variant only supports 1 port, then the check is usefull, no?

But you have below checks for u value. Wouldn't be enough?

> > > +				/* Use GPIO lines as modem control lines */
> > > +				if (u == 0)
> > > +					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
> > > +				else if (u == 1)
> > > +					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
> > > +
diff mbox series

Patch

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a5d8af0f6da0..97ec532a0a19 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -236,7 +236,8 @@ 
 
 /* IOControl register bits (Only 75x/76x) */
 #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;
+	int				gpio_configured;
 #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)
@@ -1396,6 +1393,10 @@  static int sc16is7xx_probe(struct device *dev,
 		return -ENOMEM;
 	}
 
+#ifdef CONFIG_GPIOLIB
+	s->gpio_configured = devtype->nr_gpio;
+#endif /* CONFIG_GPIOLIB */
+
 	/* Always ask for fixed clock rate from a property. */
 	device_property_read_u32(dev, "clock-frequency", &uartclk);
 
@@ -1473,12 +1474,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);
@@ -1514,10 +1509,38 @@  static int sc16is7xx_probe(struct device *dev,
 					 prop, p, u)
 			if (u < devtype->nr_uart)
 				s->p[u].irda_mode = true;
+
+		val = 0;
+
+		of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
+					 prop, p, u)
+			if (u < devtype->nr_uart) {
+				/* Use GPIO lines as modem control lines */
+				if (u == 0)
+					val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+				else if (u == 1)
+					val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+
+#ifdef CONFIG_GPIOLIB
+				if (s->gpio_configured >=
+				    SC16IS7XX_GPIOS_PER_BANK)
+					s->gpio_configured -=
+						SC16IS7XX_GPIOS_PER_BANK;
+#endif /* CONFIG_GPIOLIB */
+			}
+
+		if (val)
+			regmap_update_bits(
+				s->regmap,
+				SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+				SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+				SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
 	}
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio) {
+	dev_dbg(dev, "GPIOs to configure: %d\n", s->gpio_configured);
+
+	if (s->gpio_configured) {
 		/* Setup GPIO controller */
 		s->gpio.owner		 = THIS_MODULE;
 		s->gpio.parent		 = dev;
@@ -1527,7 +1550,7 @@  static int sc16is7xx_probe(struct device *dev,
 		s->gpio.direction_output = sc16is7xx_gpio_direction_output;
 		s->gpio.set		 = sc16is7xx_gpio_set;
 		s->gpio.base		 = -1;
-		s->gpio.ngpio		 = devtype->nr_gpio;
+		s->gpio.ngpio		 = s->gpio_configured;
 		s->gpio.can_sleep	 = 1;
 		ret = gpiochip_add_data(&s->gpio, s);
 		if (ret)
@@ -1555,7 +1578,7 @@  static int sc16is7xx_probe(struct device *dev,
 		return 0;
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio)
+	if (s->gpio_configured)
 		gpiochip_remove(&s->gpio);
 
 out_thread:
@@ -1581,7 +1604,7 @@  static void sc16is7xx_remove(struct device *dev)
 	int i;
 
 #ifdef CONFIG_GPIOLIB
-	if (s->devtype->nr_gpio)
+	if (s->gpio_configured)
 		gpiochip_remove(&s->gpio);
 #endif