[PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

The sc16is7xx_config_rs485() function is called only for the second
port (index 1, channel B), causing initialization problems for the
first port.

For the sc16is7xx driver, port->membase and port->mapbase are not set,
and their default values are 0. And we set port->iobase to the device
index. This means that when the first device is registered using the
uart_add_one_port() function, the following values will be in the port
structure:
    port->membase = 0
    port->mapbase = 0
    port->iobase  = 0

Therefore, the function uart_configure_port() in serial_core.c will
exit early because of the following check:
	/*
	 * If there isn't a port here, don't do anything further.
	 */
	if (!port->iobase && !port->mapbase && !port->membase)
		return;

Typically, I2C and SPI drivers do not set port->membase and
port->mapbase.

The max310x driver sets port->membase to ~0 (all ones). By
implementing the same change in this driver, uart_configure_port() is
now correctly executed for all ports.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 2e7e7c409cf2..8ae2afc76a9b 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
 		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
 		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
 		s->p[i].port.iobase	= i;
+		s->p[i].port.membase	= (void __iomem *)~0;
 		s->p[i].port.iotype	= UPIO_PORT;
 		s->p[i].port.uartclk	= freq;
 		s->p[i].port.rs485_config = sc16is7xx_config_rs485;
-- 
2.30.2

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> The sc16is7xx_config_rs485() function is called only for the second
> port (index 1, channel B), causing initialization problems for the
> first port.
> 
> For the sc16is7xx driver, port->membase and port->mapbase are not set,
> and their default values are 0. And we set port->iobase to the device
> index. This means that when the first device is registered using the
> uart_add_one_port() function, the following values will be in the port
> structure:
>     port->membase = 0
>     port->mapbase = 0
>     port->iobase  = 0
> 
> Therefore, the function uart_configure_port() in serial_core.c will
> exit early because of the following check:
> 	/*
> 	 * If there isn't a port here, don't do anything further.
> 	 */
> 	if (!port->iobase && !port->mapbase && !port->membase)
> 		return;
> 
> Typically, I2C and SPI drivers do not set port->membase and
> port->mapbase.
> 
> The max310x driver sets port->membase to ~0 (all ones). By
> implementing the same change in this driver, uart_configure_port() is
> now correctly executed for all ports.
> 
> Fixes: dfeae619d781 ("serial: sc16is7xx")

That commit is in a very old 3.x release.

> Cc: <stable@vger.kernel.org> # 6.1.x

But you say this should only go to 6.1.y?  Why?  What is wrong with the
older kernels?

> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 2e7e7c409cf2..8ae2afc76a9b 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
>  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
>  		s->p[i].port.iobase	= i;
> +		s->p[i].port.membase	= (void __iomem *)~0;

That's a magic value, some comment should be added here to explain why
setting all bits is ok.  Why does this work exactly?  You only say that
the max310x driver does this, but not why it does this at all.

confused,

greg k-h

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:52:26 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > The sc16is7xx_config_rs485() function is called only for the second
> > port (index 1, channel B), causing initialization problems for the
> > first port.
> > 
> > For the sc16is7xx driver, port->membase and port->mapbase are not set,
> > and their default values are 0. And we set port->iobase to the device
> > index. This means that when the first device is registered using the
> > uart_add_one_port() function, the following values will be in the port
> > structure:
> >     port->membase = 0
> >     port->mapbase = 0
> >     port->iobase  = 0
> > 
> > Therefore, the function uart_configure_port() in serial_core.c will
> > exit early because of the following check:
> > 	/*
> > 	 * If there isn't a port here, don't do anything further.
> > 	 */
> > 	if (!port->iobase && !port->mapbase && !port->membase)
> > 		return;
> > 
> > Typically, I2C and SPI drivers do not set port->membase and
> > port->mapbase.
> > 
> > The max310x driver sets port->membase to ~0 (all ones). By
> > implementing the same change in this driver, uart_configure_port() is
> > now correctly executed for all ports.
> > 
> > Fixes: dfeae619d781 ("serial: sc16is7xx")
> 
> That commit is in a very old 3.x release.
> 
> > Cc: <stable@vger.kernel.org> # 6.1.x
> 
> But you say this should only go to 6.1.y?  Why?  What is wrong with the
> older kernels?

Hi Greg,
I have read (and reread a couple of times)
Documentation/process/stable-kernel-rules.rst to try to understand how
to format the tags, but unfortunately it doesn't contain "Everything
you ever wanted to know about Linux -stable releases" as the title
claims :)

In particular, it doesn't explain or advise which older version we
should target, that is why since I was not sure I specified 6.1.y
because I could test it properly, but not v3.x.

Maybe it would be best to simply drop for now all the "Cc:
<stable@vger.kernel.org>" tags for this series, and following Option 2,
I send an email to stable@vger.kernel.org once the patches have been
merged to Linus' tree?


> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 2e7e7c409cf2..8ae2afc76a9b 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
> >  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
> >  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> >  		s->p[i].port.iobase	= i;
> > +		s->p[i].port.membase	= (void __iomem *)~0;
> 
> That's a magic value, some comment should be added here to explain why
> setting all bits is ok.  Why does this work exactly?  You only say that
> the max310x driver does this, but not why it does this at all.

I do not understand, because my commit log message is quite long
and, it seems to me, well documenting why this works the way it
does when calling uart_configure_port() in serial_core.c?

I say that the max310x driver also does this, because there is also no
comment in the max310x driver for using the (void __iomem *)~0;
construct. I also located the original commit message for the max310x
driver but no comments were usefull there also.

So, what about adding this comment:

/*
 * Use all ones as membase to make sure uart_configure_port() in
 * serial_core.c does not abort for SPI/I2C devices where the
 * membase address is not applicable.
 */
 s->p[i].port.membase	= (void __iomem *)~0;


Also, keep in mind that in the original discussion about that patch,
there was the following comment from Ilpo Järvinen:

------
This changelog was really well describing the problem! :-)
Yeah, some other solution should be devised. Maybe we should add
another .iotype for thse kind of devices. But I'm fine with this for
this fix. After editing the changelog, feel free to add:
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
------

If wou want, I could also add the same comment to the max310 driver?

Hugo.

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Greg KH]

On Tue, Aug 01, 2023 at 01:16:55PM -0400, Hugo Villeneuve wrote:
> On Mon, 31 Jul 2023 17:52:26 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > The sc16is7xx_config_rs485() function is called only for the second
> > > port (index 1, channel B), causing initialization problems for the
> > > first port.
> > > 
> > > For the sc16is7xx driver, port->membase and port->mapbase are not set,
> > > and their default values are 0. And we set port->iobase to the device
> > > index. This means that when the first device is registered using the
> > > uart_add_one_port() function, the following values will be in the port
> > > structure:
> > >     port->membase = 0
> > >     port->mapbase = 0
> > >     port->iobase  = 0
> > > 
> > > Therefore, the function uart_configure_port() in serial_core.c will
> > > exit early because of the following check:
> > > 	/*
> > > 	 * If there isn't a port here, don't do anything further.
> > > 	 */
> > > 	if (!port->iobase && !port->mapbase && !port->membase)
> > > 		return;
> > > 
> > > Typically, I2C and SPI drivers do not set port->membase and
> > > port->mapbase.
> > > 
> > > The max310x driver sets port->membase to ~0 (all ones). By
> > > implementing the same change in this driver, uart_configure_port() is
> > > now correctly executed for all ports.
> > > 
> > > Fixes: dfeae619d781 ("serial: sc16is7xx")
> > 
> > That commit is in a very old 3.x release.
> > 
> > > Cc: <stable@vger.kernel.org> # 6.1.x
> > 
> > But you say this should only go to 6.1.y?  Why?  What is wrong with the
> > older kernels?
> 
> Hi Greg,
> I have read (and reread a couple of times)
> Documentation/process/stable-kernel-rules.rst to try to understand how
> to format the tags, but unfortunately it doesn't contain "Everything
> you ever wanted to know about Linux -stable releases" as the title
> claims :)
> 
> In particular, it doesn't explain or advise which older version we
> should target, that is why since I was not sure I specified 6.1.y
> because I could test it properly, but not v3.x.

If you think this fixes an issue back to 3.x, then just leave it at
that, there's no need to have to test all of these.  If when I apply the
patch to the stable trees, and it does not go back to all of the
active versions specified by Fixes: then you will get an email saying
so and can handle it then if you want to.

> Maybe it would be best to simply drop for now all the "Cc:
> <stable@vger.kernel.org>" tags for this series, and following Option 2,
> I send an email to stable@vger.kernel.org once the patches have been
> merged to Linus' tree?

That will just mean more work for both of us, leave it as is, just drop
the "# 6.1.x" portion please.

> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > ---
> > >  drivers/tty/serial/sc16is7xx.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > index 2e7e7c409cf2..8ae2afc76a9b 100644
> > > --- a/drivers/tty/serial/sc16is7xx.c
> > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > @@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
> > >  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
> > >  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> > >  		s->p[i].port.iobase	= i;
> > > +		s->p[i].port.membase	= (void __iomem *)~0;
> > 
> > That's a magic value, some comment should be added here to explain why
> > setting all bits is ok.  Why does this work exactly?  You only say that
> > the max310x driver does this, but not why it does this at all.
> 
> I do not understand, because my commit log message is quite long
> and, it seems to me, well documenting why this works the way it
> does when calling uart_configure_port() in serial_core.c?
> 
> I say that the max310x driver also does this, because there is also no
> comment in the max310x driver for using the (void __iomem *)~0;
> construct. I also located the original commit message for the max310x
> driver but no comments were usefull there also.
> 
> So, what about adding this comment:
> 
> /*
>  * Use all ones as membase to make sure uart_configure_port() in
>  * serial_core.c does not abort for SPI/I2C devices where the
>  * membase address is not applicable.
>  */
>  s->p[i].port.membase	= (void __iomem *)~0;

Yes, that would be good, thank you.

> If wou want, I could also add the same comment to the max310 driver?

Yes please.

thanks,

greg k-h

Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve]

On Thu, 3 Aug 2023 09:54:37 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Aug 01, 2023 at 01:16:55PM -0400, Hugo Villeneuve wrote:
> > On Mon, 31 Jul 2023 17:52:26 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > The sc16is7xx_config_rs485() function is called only for the second
> > > > port (index 1, channel B), causing initialization problems for the
> > > > first port.
> > > > 
> > > > For the sc16is7xx driver, port->membase and port->mapbase are not set,
> > > > and their default values are 0. And we set port->iobase to the device
> > > > index. This means that when the first device is registered using the
> > > > uart_add_one_port() function, the following values will be in the port
> > > > structure:
> > > >     port->membase = 0
> > > >     port->mapbase = 0
> > > >     port->iobase  = 0
> > > > 
> > > > Therefore, the function uart_configure_port() in serial_core.c will
> > > > exit early because of the following check:
> > > > 	/*
> > > > 	 * If there isn't a port here, don't do anything further.
> > > > 	 */
> > > > 	if (!port->iobase && !port->mapbase && !port->membase)
> > > > 		return;
> > > > 
> > > > Typically, I2C and SPI drivers do not set port->membase and
> > > > port->mapbase.
> > > > 
> > > > The max310x driver sets port->membase to ~0 (all ones). By
> > > > implementing the same change in this driver, uart_configure_port() is
> > > > now correctly executed for all ports.
> > > > 
> > > > Fixes: dfeae619d781 ("serial: sc16is7xx")
> > > 
> > > That commit is in a very old 3.x release.
> > > 
> > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > 
> > > But you say this should only go to 6.1.y?  Why?  What is wrong with the
> > > older kernels?
> > 
> > Hi Greg,
> > I have read (and reread a couple of times)
> > Documentation/process/stable-kernel-rules.rst to try to understand how
> > to format the tags, but unfortunately it doesn't contain "Everything
> > you ever wanted to know about Linux -stable releases" as the title
> > claims :)
> > 
> > In particular, it doesn't explain or advise which older version we
> > should target, that is why since I was not sure I specified 6.1.y
> > because I could test it properly, but not v3.x.
> 
> If you think this fixes an issue back to 3.x, then just leave it at
> that, there's no need to have to test all of these.  If when I apply the
> patch to the stable trees, and it does not go back to all of the
> active versions specified by Fixes: then you will get an email saying
> so and can handle it then if you want to.
> 
> > Maybe it would be best to simply drop for now all the "Cc:
> > <stable@vger.kernel.org>" tags for this series, and following Option 2,
> > I send an email to stable@vger.kernel.org once the patches have been
> > merged to Linus' tree?
> 
> That will just mean more work for both of us, leave it as is, just drop
> the "# 6.1.x" portion please.
> 
> > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > ---
> > > >  drivers/tty/serial/sc16is7xx.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > index 2e7e7c409cf2..8ae2afc76a9b 100644
> > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > @@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
> > > >  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
> > > >  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> > > >  		s->p[i].port.iobase	= i;
> > > > +		s->p[i].port.membase	= (void __iomem *)~0;
> > > 
> > > That's a magic value, some comment should be added here to explain why
> > > setting all bits is ok.  Why does this work exactly?  You only say that
> > > the max310x driver does this, but not why it does this at all.
> > 
> > I do not understand, because my commit log message is quite long
> > and, it seems to me, well documenting why this works the way it
> > does when calling uart_configure_port() in serial_core.c?
> > 
> > I say that the max310x driver also does this, because there is also no
> > comment in the max310x driver for using the (void __iomem *)~0;
> > construct. I also located the original commit message for the max310x
> > driver but no comments were usefull there also.
> > 
> > So, what about adding this comment:
> > 
> > /*
> >  * Use all ones as membase to make sure uart_configure_port() in
> >  * serial_core.c does not abort for SPI/I2C devices where the
> >  * membase address is not applicable.
> >  */
> >  s->p[i].port.membase	= (void __iomem *)~0;
> 
> Yes, that would be good, thank you.
> 
> > If wou want, I could also add the same comment to the max310 driver?
> 
> Yes please.

Hi Greg,
I will send a separate patch specifically for this.

Thank you, Hugo.

[PATCH v9 02/10] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Bit SRESET (3) is cleared when a reset operation is completed. Having
the IOCONTROL register as non-volatile will always read SRESET as 1,
which is incorrect.

Also, if IOCONTROL register is not a volatile register, the upcoming
patch "serial: sc16is7xx: fix regression with GPIO configuration"
doesn't work when setting some shared GPIO lines as modem control
lines.

Therefore mark IOCONTROL register as a volatile register.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 8ae2afc76a9b..306ae512b38a 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
 	case SC16IS7XX_TXLVL_REG:
 	case SC16IS7XX_RXLVL_REG:
 	case SC16IS7XX_IOSTATE_REG:
+	case SC16IS7XX_IOCONTROL_REG:
 		return true;
 	default:
 		break;
-- 
2.30.2

Re: [PATCH v9 02/10] serial: sc16is7xx: mark IOCONTROL register as volatile

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:34AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> Bit SRESET (3) is cleared when a reset operation is completed. Having
> the IOCONTROL register as non-volatile will always read SRESET as 1,
> which is incorrect.
> 
> Also, if IOCONTROL register is not a volatile register, the upcoming
> patch "serial: sc16is7xx: fix regression with GPIO configuration"
> doesn't work when setting some shared GPIO lines as modem control
> lines.
> 
> Therefore mark IOCONTROL register as a volatile register.
> 
> Cc: <stable@vger.kernel.org> # 6.1.x

Why 6.1.y?  What commit does this fix?

> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 8ae2afc76a9b..306ae512b38a 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
>  	case SC16IS7XX_TXLVL_REG:
>  	case SC16IS7XX_RXLVL_REG:
>  	case SC16IS7XX_IOSTATE_REG:
> +	case SC16IS7XX_IOCONTROL_REG:
>  		return true;
>  	default:
>  		break;

Is this the same as this change:
	https://lore.kernel.org/all/20230724034727.17335-1-hui.wang@canonical.com/

confused,

greg k-h

Re: [PATCH v9 02/10] serial: sc16is7xx: mark IOCONTROL register as volatile

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:50:40 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:34AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > Bit SRESET (3) is cleared when a reset operation is completed. Having
> > the IOCONTROL register as non-volatile will always read SRESET as 1,
> > which is incorrect.
> > 
> > Also, if IOCONTROL register is not a volatile register, the upcoming
> > patch "serial: sc16is7xx: fix regression with GPIO configuration"
> > doesn't work when setting some shared GPIO lines as modem control
> > lines.
> > 
> > Therefore mark IOCONTROL register as a volatile register.
> > 
> > Cc: <stable@vger.kernel.org> # 6.1.x
> 
> Why 6.1.y?  What commit does this fix?
> 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 8ae2afc76a9b..306ae512b38a 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -488,6 +488,7 @@ static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
> >  	case SC16IS7XX_TXLVL_REG:
> >  	case SC16IS7XX_RXLVL_REG:
> >  	case SC16IS7XX_IOSTATE_REG:
> > +	case SC16IS7XX_IOCONTROL_REG:
> >  		return true;
> >  	default:
> >  		break;
> 
> Is this the same as this change:
> 	https://lore.kernel.org/all/20230724034727.17335-1-hui.wang@canonical.com/
> 
> confused,

Hi Greg,
yes this is the same.

You simply accepted an exact equivalent of my patch by someone else in
your tree, no confusion there.

I will remove this patch from my series and rebase it on your tree
gregkh_tty/tty-next.

Hugo.

[PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

In preparation for upcoming patch "fix regression with GPIO
configuration". To facilitate review and make code more modular.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 32d43d00a583..5b0aeef9d534 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -332,6 +332,7 @@ struct sc16is7xx_one {
 
 struct sc16is7xx_port {
 	const struct sc16is7xx_devtype	*devtype;
+	struct device			*dev;
 	struct regmap			*regmap;
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
@@ -1349,6 +1350,25 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 
 	return 0;
 }
+
+static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
+{
+	if (!s->devtype->nr_gpio)
+		return 0;
+
+	s->gpio.owner		 = THIS_MODULE;
+	s->gpio.parent		 = s->dev;
+	s->gpio.label		 = dev_name(s->dev);
+	s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
+	s->gpio.get		 = sc16is7xx_gpio_get;
+	s->gpio.direction_output = sc16is7xx_gpio_direction_output;
+	s->gpio.set		 = sc16is7xx_gpio_set;
+	s->gpio.base		 = -1;
+	s->gpio.ngpio		 = s->devtype->nr_gpio;
+	s->gpio.can_sleep	 = 1;
+
+	return gpiochip_add_data(&s->gpio, s);
+}
 #endif
 
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
@@ -1412,6 +1432,7 @@ static int sc16is7xx_probe(struct device *dev,
 
 	s->regmap = regmap;
 	s->devtype = devtype;
+	s->dev = dev;
 	dev_set_drvdata(dev, s);
 	mutex_init(&s->efr_lock);
 
@@ -1502,22 +1523,9 @@ static int sc16is7xx_probe(struct device *dev,
 	}
 
 #ifdef CONFIG_GPIOLIB
-	if (devtype->nr_gpio) {
-		/* Setup GPIO cotroller */
-		s->gpio.owner		 = THIS_MODULE;
-		s->gpio.parent		 = dev;
-		s->gpio.label		 = dev_name(dev);
-		s->gpio.direction_input	 = sc16is7xx_gpio_direction_input;
-		s->gpio.get		 = sc16is7xx_gpio_get;
-		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.can_sleep	 = 1;
-		ret = gpiochip_add_data(&s->gpio, s);
-		if (ret)
-			goto out_ports;
-	}
+	ret = sc16is7xx_setup_gpio_chip(s);
+	if (ret)
+		goto out_ports;
 #endif
 
 	/*
-- 
2.30.2

Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> In preparation for upcoming patch "fix regression with GPIO
> configuration". To facilitate review and make code more modular.

I would much rather the issue be fixed _before_ the code is refactored,
unless it is impossible to fix it without the refactor?

> 
> Cc: <stable@vger.kernel.org> # 6.1.x

What commit id does this fix?

> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> ---
>  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 32d43d00a583..5b0aeef9d534 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -332,6 +332,7 @@ struct sc16is7xx_one {
>  
>  struct sc16is7xx_port {
>  	const struct sc16is7xx_devtype	*devtype;
> +	struct device			*dev;

Why is this pointer needed?

Why is it grabbed and yet the reference count is never incremented?  Who
owns the reference count and when will it go away?

And what device is this?  The parent?  Current device?  What type of
device is it?  And why is it needed?

Using "raw" devices is almost never something a driver should do, they
are only passed into functions by the driver core, but then the driver
should instantly turn them into the "real" structure.


thanks,

greg k-h

Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:55:42 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > In preparation for upcoming patch "fix regression with GPIO
> > configuration". To facilitate review and make code more modular.
> 
> I would much rather the issue be fixed _before_ the code is refactored,
> unless it is impossible to fix it without the refactor?

Hi Greg,
normally I would agree, but the refactor in this case helps a lot to
address some issues raised by you and Andy in V7 of this series.

Maybe I could merge it with the actual patch "fix regression with GPIO
configuration"?


> > Cc: <stable@vger.kernel.org> # 6.1.x
> 
> What commit id does this fix?

It doesn't fix anything, but I tought that I needed this tag since
this patch is a prerequisite for the next patch in the series, which
would be applied to stable kernels. I will remove this tag (assuming
the patch stays as it is, depending on your answer to the above
question).

 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > ---
> >  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 32d43d00a583..5b0aeef9d534 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -332,6 +332,7 @@ struct sc16is7xx_one {
> >  
> >  struct sc16is7xx_port {
> >  	const struct sc16is7xx_devtype	*devtype;
> > +	struct device			*dev;
> 
> Why is this pointer needed?
> 
> Why is it grabbed and yet the reference count is never incremented?  Who
> owns the reference count and when will it go away?
> 
> And what device is this?  The parent?  Current device?  What type of
> device is it?  And why is it needed?
> 
> Using "raw" devices is almost never something a driver should do, they
> are only passed into functions by the driver core, but then the driver
> should instantly turn them into the "real" structure.

We already discussed that a lot in previous versions (v7)... I am
trying my best to modify the code to address your concerns, but I am
not fully understanding what you mean about raw devices, and you didn't
answer some of my previous questions/interrogations in v7 about that.

So, in the new function that I
need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
a raw device to read a device tree property and to set
s->gpio.parent:

    count = device_property_count_u32(dev, ...
    ...
    s->gpio.parent = dev;

Do we agree on that?

Then, how do I pass this raw device to the 
device_property_count_u32() function and to the s->gpio.parent
assignment?

Should I modify sc16is7xx_setup_gpio_chip() like so:

    static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
    {
	struct device *dev = &s->p[0].port.dev;

        count = device_property_count_u32(dev, ...
        ...
        s->gpio.parent = dev;

?

If not, can you show me how you would like to do it to avoid me trying
to guess?

Thank you,
Hugo.

Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Greg KH]

On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> On Mon, 31 Jul 2023 17:55:42 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > In preparation for upcoming patch "fix regression with GPIO
> > > configuration". To facilitate review and make code more modular.
> > 
> > I would much rather the issue be fixed _before_ the code is refactored,
> > unless it is impossible to fix it without the refactor?
> 
> Hi Greg,
> normally I would agree, but the refactor in this case helps a lot to
> address some issues raised by you and Andy in V7 of this series.
> 
> Maybe I could merge it with the actual patch "fix regression with GPIO
> configuration"?

Sure.

> > > Cc: <stable@vger.kernel.org> # 6.1.x
> > 
> > What commit id does this fix?
> 
> It doesn't fix anything, but I tought that I needed this tag since
> this patch is a prerequisite for the next patch in the series, which
> would be applied to stable kernels. I will remove this tag (assuming
> the patch stays as it is, depending on your answer to the above
> question).
> 
>  
> > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > ---
> > >  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
> > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > index 32d43d00a583..5b0aeef9d534 100644
> > > --- a/drivers/tty/serial/sc16is7xx.c
> > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > @@ -332,6 +332,7 @@ struct sc16is7xx_one {
> > >  
> > >  struct sc16is7xx_port {
> > >  	const struct sc16is7xx_devtype	*devtype;
> > > +	struct device			*dev;
> > 
> > Why is this pointer needed?
> > 
> > Why is it grabbed and yet the reference count is never incremented?  Who
> > owns the reference count and when will it go away?
> > 
> > And what device is this?  The parent?  Current device?  What type of
> > device is it?  And why is it needed?
> > 
> > Using "raw" devices is almost never something a driver should do, they
> > are only passed into functions by the driver core, but then the driver
> > should instantly turn them into the "real" structure.
> 
> We already discussed that a lot in previous versions (v7)... I am
> trying my best to modify the code to address your concerns, but I am
> not fully understanding what you mean about raw devices, and you didn't
> answer some of my previous questions/interrogations in v7 about that.

I don't have time to answer all questions, sorry.

Please help review submitted patches to reduce my load and allow me to
answer other stuff :)

> So, in the new function that I
> need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> a raw device to read a device tree property and to set
> s->gpio.parent:
> 
>     count = device_property_count_u32(dev, ...
>     ...
>     s->gpio.parent = dev;
> 
> Do we agree on that?

Yes, but what type of parent is that?

> Then, how do I pass this raw device to the 
> device_property_count_u32() function and to the s->gpio.parent
> assignment?
> 
> Should I modify sc16is7xx_setup_gpio_chip() like so:
> 
>     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
>     {
> 	struct device *dev = &s->p[0].port.dev;
> 
>         count = device_property_count_u32(dev, ...
>         ...
>         s->gpio.parent = dev;

Again, what is the real type of that parent?  It's a port, right, so
pass in the port to this function and then do the "take the struct
device of the port" at that point in time.

thanks,

greg k-h

Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

On Fri, 4 Aug 2023 15:14:18 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > On Mon, 31 Jul 2023 17:55:42 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > In preparation for upcoming patch "fix regression with GPIO
> > > > configuration". To facilitate review and make code more modular.
> > > 
> > > I would much rather the issue be fixed _before_ the code is refactored,
> > > unless it is impossible to fix it without the refactor?
> > 
> > Hi Greg,
> > normally I would agree, but the refactor in this case helps a lot to
> > address some issues raised by you and Andy in V7 of this series.
> > 
> > Maybe I could merge it with the actual patch "fix regression with GPIO
> > configuration"?
> 
> Sure.

Hi Greg,
will do.

 
> > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > 
> > > What commit id does this fix?
> > 
> > It doesn't fix anything, but I tought that I needed this tag since
> > this patch is a prerequisite for the next patch in the series, which
> > would be applied to stable kernels. I will remove this tag (assuming
> > the patch stays as it is, depending on your answer to the above
> > question).
> > 
> >  
> > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > ---
> > > >  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
> > > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > index 32d43d00a583..5b0aeef9d534 100644
> > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > @@ -332,6 +332,7 @@ struct sc16is7xx_one {
> > > >  
> > > >  struct sc16is7xx_port {
> > > >  	const struct sc16is7xx_devtype	*devtype;
> > > > +	struct device			*dev;
> > > 
> > > Why is this pointer needed?
> > > 
> > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > owns the reference count and when will it go away?
> > > 
> > > And what device is this?  The parent?  Current device?  What type of
> > > device is it?  And why is it needed?
> > > 
> > > Using "raw" devices is almost never something a driver should do, they
> > > are only passed into functions by the driver core, but then the driver
> > > should instantly turn them into the "real" structure.
> > 
> > We already discussed that a lot in previous versions (v7)... I am
> > trying my best to modify the code to address your concerns, but I am
> > not fully understanding what you mean about raw devices, and you didn't
> > answer some of my previous questions/interrogations in v7 about that.
> 
> I don't have time to answer all questions, sorry.
> 
> Please help review submitted patches to reduce my load and allow me to
> answer other stuff :)

Ok.


> > So, in the new function that I
> > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > a raw device to read a device tree property and to set
> > s->gpio.parent:
> > 
> >     count = device_property_count_u32(dev, ...
> >     ...
> >     s->gpio.parent = dev;
> > 
> > Do we agree on that?
> 
> Yes, but what type of parent is that?

I am confused by your question. I do not understand why the type of
parent matters... And what do you call the parent: s, s->gpio or
s->gpio.parent?

For me, the way I understand it, the only question that matters is how I
can extract the raw device structure pointer from maybe "struct
sc16is7xx_port" or some other structure, and then use it in my
new function...

I should not have put "s->gpio.parent = dev" in the example, I think it
just complexifies things. Lets start over with a more simple example and
only:

    count = device_property_count_u32(dev, ...


> > Then, how do I pass this raw device to the 
> > device_property_count_u32() function and to the s->gpio.parent
> > assignment?
> > 
> > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > 
> >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> >     {
> > 	struct device *dev = &s->p[0].port.dev;
> > 
> >         count = device_property_count_u32(dev, ...
> >         ...
> >         s->gpio.parent = dev;
> 
> Again, what is the real type of that parent?  It's a port, right, so
> pass in the port to this function and then do the "take the struct
> device of the port" at that point in time.

With the simplified example, is the following ok:

static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
{
    struct device *dev = &s->p[0].port.dev;

    count = device_property_count_u32(dev, ...
    ...
}

If not, please indicate how you would do it with an actual example...

Thank you,
Hugo.

Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Greg KH]

On Fri, Aug 04, 2023 at 10:15:54AM -0400, Hugo Villeneuve wrote:
> On Fri, 4 Aug 2023 15:14:18 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > > On Mon, 31 Jul 2023 17:55:42 +0200
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > 
> > > > > In preparation for upcoming patch "fix regression with GPIO
> > > > > configuration". To facilitate review and make code more modular.
> > > > 
> > > > I would much rather the issue be fixed _before_ the code is refactored,
> > > > unless it is impossible to fix it without the refactor?
> > > 
> > > Hi Greg,
> > > normally I would agree, but the refactor in this case helps a lot to
> > > address some issues raised by you and Andy in V7 of this series.
> > > 
> > > Maybe I could merge it with the actual patch "fix regression with GPIO
> > > configuration"?
> > 
> > Sure.
> 
> Hi Greg,
> will do.
> 
>  
> > > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > > 
> > > > What commit id does this fix?
> > > 
> > > It doesn't fix anything, but I tought that I needed this tag since
> > > this patch is a prerequisite for the next patch in the series, which
> > > would be applied to stable kernels. I will remove this tag (assuming
> > > the patch stays as it is, depending on your answer to the above
> > > question).
> > > 
> > >  
> > > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > ---
> > > > >  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
> > > > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > > index 32d43d00a583..5b0aeef9d534 100644
> > > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > > @@ -332,6 +332,7 @@ struct sc16is7xx_one {
> > > > >  
> > > > >  struct sc16is7xx_port {
> > > > >  	const struct sc16is7xx_devtype	*devtype;
> > > > > +	struct device			*dev;
> > > > 
> > > > Why is this pointer needed?
> > > > 
> > > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > > owns the reference count and when will it go away?
> > > > 
> > > > And what device is this?  The parent?  Current device?  What type of
> > > > device is it?  And why is it needed?
> > > > 
> > > > Using "raw" devices is almost never something a driver should do, they
> > > > are only passed into functions by the driver core, but then the driver
> > > > should instantly turn them into the "real" structure.
> > > 
> > > We already discussed that a lot in previous versions (v7)... I am
> > > trying my best to modify the code to address your concerns, but I am
> > > not fully understanding what you mean about raw devices, and you didn't
> > > answer some of my previous questions/interrogations in v7 about that.
> > 
> > I don't have time to answer all questions, sorry.
> > 
> > Please help review submitted patches to reduce my load and allow me to
> > answer other stuff :)
> 
> Ok.
> 
> 
> > > So, in the new function that I
> > > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > > a raw device to read a device tree property and to set
> > > s->gpio.parent:
> > > 
> > >     count = device_property_count_u32(dev, ...
> > >     ...
> > >     s->gpio.parent = dev;
> > > 
> > > Do we agree on that?
> > 
> > Yes, but what type of parent is that?
> 
> I am confused by your question. I do not understand why the type of
> parent matters... And what do you call the parent: s, s->gpio or
> s->gpio.parent?
> 
> For me, the way I understand it, the only question that matters is how I
> can extract the raw device structure pointer from maybe "struct
> sc16is7xx_port" or some other structure, and then use it in my
> new function...
> 
> I should not have put "s->gpio.parent = dev" in the example, I think it
> just complexifies things. Lets start over with a more simple example and
> only:
> 
>     count = device_property_count_u32(dev, ...
> 
> 
> > > Then, how do I pass this raw device to the 
> > > device_property_count_u32() function and to the s->gpio.parent
> > > assignment?
> > > 
> > > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > > 
> > >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > >     {
> > > 	struct device *dev = &s->p[0].port.dev;
> > > 
> > >         count = device_property_count_u32(dev, ...
> > >         ...
> > >         s->gpio.parent = dev;
> > 
> > Again, what is the real type of that parent?  It's a port, right, so
> > pass in the port to this function and then do the "take the struct
> > device of the port" at that point in time.
> 
> With the simplified example, is the following ok:
> 
> static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> {
>     struct device *dev = &s->p[0].port.dev;
> 
>     count = device_property_count_u32(dev, ...
>     ...
> }
> 
> If not, please indicate how you would do it with an actual example...

At this point, after reviewing 500+ patches today, I really have no
idea, my brain is fried.  Do what you think is right here and submit a
new series and I'll be glad to review it.

thanks,

greg k-h

Re: [PATCH v9 04/10] serial: sc16is7xx: refactor GPIO controller registration

[Hugo Villeneuve]

On Fri, 4 Aug 2023 17:09:13 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Aug 04, 2023 at 10:15:54AM -0400, Hugo Villeneuve wrote:
> > On Fri, 4 Aug 2023 15:14:18 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Thu, Aug 03, 2023 at 12:14:49PM -0400, Hugo Villeneuve wrote:
> > > > On Mon, 31 Jul 2023 17:55:42 +0200
> > > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > 
> > > > > On Tue, Jul 25, 2023 at 10:23:36AM -0400, Hugo Villeneuve wrote:
> > > > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > > 
> > > > > > In preparation for upcoming patch "fix regression with GPIO
> > > > > > configuration". To facilitate review and make code more modular.
> > > > > 
> > > > > I would much rather the issue be fixed _before_ the code is refactored,
> > > > > unless it is impossible to fix it without the refactor?
> > > > 
> > > > Hi Greg,
> > > > normally I would agree, but the refactor in this case helps a lot to
> > > > address some issues raised by you and Andy in V7 of this series.
> > > > 
> > > > Maybe I could merge it with the actual patch "fix regression with GPIO
> > > > configuration"?
> > > 
> > > Sure.
> > 
> > Hi Greg,
> > will do.
> > 
> >  
> > > > > > Cc: <stable@vger.kernel.org> # 6.1.x
> > > > > 
> > > > > What commit id does this fix?
> > > > 
> > > > It doesn't fix anything, but I tought that I needed this tag since
> > > > this patch is a prerequisite for the next patch in the series, which
> > > > would be applied to stable kernels. I will remove this tag (assuming
> > > > the patch stays as it is, depending on your answer to the above
> > > > question).
> > > > 
> > > >  
> > > > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > > > ---
> > > > > >  drivers/tty/serial/sc16is7xx.c | 40 ++++++++++++++++++++--------------
> > > > > >  1 file changed, 24 insertions(+), 16 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > > > index 32d43d00a583..5b0aeef9d534 100644
> > > > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > > > @@ -332,6 +332,7 @@ struct sc16is7xx_one {
> > > > > >  
> > > > > >  struct sc16is7xx_port {
> > > > > >  	const struct sc16is7xx_devtype	*devtype;
> > > > > > +	struct device			*dev;
> > > > > 
> > > > > Why is this pointer needed?
> > > > > 
> > > > > Why is it grabbed and yet the reference count is never incremented?  Who
> > > > > owns the reference count and when will it go away?
> > > > > 
> > > > > And what device is this?  The parent?  Current device?  What type of
> > > > > device is it?  And why is it needed?
> > > > > 
> > > > > Using "raw" devices is almost never something a driver should do, they
> > > > > are only passed into functions by the driver core, but then the driver
> > > > > should instantly turn them into the "real" structure.
> > > > 
> > > > We already discussed that a lot in previous versions (v7)... I am
> > > > trying my best to modify the code to address your concerns, but I am
> > > > not fully understanding what you mean about raw devices, and you didn't
> > > > answer some of my previous questions/interrogations in v7 about that.
> > > 
> > > I don't have time to answer all questions, sorry.
> > > 
> > > Please help review submitted patches to reduce my load and allow me to
> > > answer other stuff :)
> > 
> > Ok.
> > 
> > 
> > > > So, in the new function that I
> > > > need to implement, sc16is7xx_setup_gpio_chip(), I absolutely need to use
> > > > a raw device to read a device tree property and to set
> > > > s->gpio.parent:
> > > > 
> > > >     count = device_property_count_u32(dev, ...
> > > >     ...
> > > >     s->gpio.parent = dev;
> > > > 
> > > > Do we agree on that?
> > > 
> > > Yes, but what type of parent is that?
> > 
> > I am confused by your question. I do not understand why the type of
> > parent matters... And what do you call the parent: s, s->gpio or
> > s->gpio.parent?
> > 
> > For me, the way I understand it, the only question that matters is how I
> > can extract the raw device structure pointer from maybe "struct
> > sc16is7xx_port" or some other structure, and then use it in my
> > new function...
> > 
> > I should not have put "s->gpio.parent = dev" in the example, I think it
> > just complexifies things. Lets start over with a more simple example and
> > only:
> > 
> >     count = device_property_count_u32(dev, ...
> > 
> > 
> > > > Then, how do I pass this raw device to the 
> > > > device_property_count_u32() function and to the s->gpio.parent
> > > > assignment?
> > > > 
> > > > Should I modify sc16is7xx_setup_gpio_chip() like so:
> > > > 
> > > >     static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > > >     {
> > > > 	struct device *dev = &s->p[0].port.dev;
> > > > 
> > > >         count = device_property_count_u32(dev, ...
> > > >         ...
> > > >         s->gpio.parent = dev;
> > > 
> > > Again, what is the real type of that parent?  It's a port, right, so
> > > pass in the port to this function and then do the "take the struct
> > > device of the port" at that point in time.
> > 
> > With the simplified example, is the following ok:
> > 
> > static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
> > {
> >     struct device *dev = &s->p[0].port.dev;
> > 
> >     count = device_property_count_u32(dev, ...
> >     ...
> > }
> > 
> > If not, please indicate how you would do it with an actual example...
> 
> At this point, after reviewing 500+ patches today, I really have no
> idea, my brain is fried.  Do what you think is right here and submit a
> new series and I'll be glad to review it.

Ok :)

Will do.

Hugo.

[PATCH v9 05/10] dt-bindings: sc16is7xx: Add property to change GPIO function

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

Some variants in this series of UART controllers have GPIO pins that
are shared between GPIO and modem control lines.

The pin mux mode (GPIO or modem control lines) can be set for each
ports (channels) supported by the variant.

This adds a property to the device tree to set the GPIO pin mux to
modem control lines on selected ports if needed.

Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 .../bindings/serial/nxp,sc16is7xx.txt         | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
index 0fa8e3e43bf8..1a7e4bff0456 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.txt
@@ -23,6 +23,9 @@ Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as
+				modem control lines.
 
 Example:
         sc16is750: sc16is750@51 {
@@ -35,6 +38,26 @@ Example:
                 #gpio-cells = <2>;
         };
 
+	sc16is752: sc16is752@53 {
+		compatible = "nxp,sc16is752";
+		reg = <0x53>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
+	sc16is752: sc16is752@54 {
+		compatible = "nxp,sc16is752";
+		reg = <0x54>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
+	};
+
 * spi as bus
 
 Required properties:
@@ -59,6 +82,9 @@ Optional properties:
     1 = active low.
 - irda-mode-ports: An array that lists the indices of the port that
 		   should operate in IrDA mode.
+- nxp,modem-control-line-ports: An array that lists the indices of the port that
+				should have shared GPIO lines configured as
+				modem control lines.
 
 Example:
 	sc16is750: sc16is750@0 {
@@ -70,3 +96,23 @@ Example:
 		gpio-controller;
 		#gpio-cells = <2>;
 	};
+
+	sc16is752: sc16is752@1 {
+		compatible = "nxp,sc16is752";
+		reg = <1>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <1>; /* Port 1 as modem control lines */
+		gpio-controller; /* Port 0 as GPIOs */
+		#gpio-cells = <2>;
+	};
+
+	sc16is752: sc16is752@2 {
+		compatible = "nxp,sc16is752";
+		reg = <2>;
+		clocks = <&clk20m>;
+		interrupt-parent = <&gpio3>;
+		interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
+		nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
+	};
-- 
2.30.2

[PATCH v9 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

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
"nxp,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 DT property.

Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
Cc: <stable@vger.kernel.org> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
Cc: <stable@vger.kernel.org> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
Cc: <stable@vger.kernel.org> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
Cc: <stable@vger.kernel.org> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
Cc: <stable@vger.kernel.org> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 104 +++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 5b0aeef9d534..bc0a288f258d 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)
@@ -337,7 +338,9 @@ struct sc16is7xx_port {
 	struct clk			*clk;
 #ifdef CONFIG_GPIOLIB
 	struct gpio_chip		gpio;
+	unsigned long			gpio_valid_mask;
 #endif
+	u8				mctrl_mask;
 	unsigned char			buf[SC16IS7XX_FIFO_SIZE];
 	struct kthread_worker		kworker;
 	struct task_struct		*kworker_task;
@@ -448,35 +451,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)
@@ -1351,14 +1349,43 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 	return 0;
 }
 
+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 sc16is7xx_port *s)
 {
 	if (!s->devtype->nr_gpio)
 		return 0;
 
+	switch (s->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 == 0)
+		return 0;
+
 	s->gpio.owner		 = THIS_MODULE;
 	s->gpio.parent		 = s->dev;
 	s->gpio.label		 = dev_name(s->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 +1398,47 @@ static int sc16is7xx_setup_gpio_chip(struct sc16is7xx_port *s)
 }
 #endif
 
+/*
+ * Configure ports designated to operate as modem control lines.
+ */
+static int sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
+{
+	int i;
+	int ret;
+	int count;
+	u32 mctrl_port[2];
+
+	count = device_property_count_u32(s->dev,
+					  "nxp,modem-control-line-ports");
+	if (count < 0 || count > ARRAY_SIZE(mctrl_port))
+		return 0;
+
+	ret = device_property_read_u32_array(s->dev,
+					     "nxp,modem-control-line-ports",
+					     mctrl_port, count);
+	if (ret)
+		return ret;
+
+	s->mctrl_mask = 0;
+
+	for (i = 0; i < count; i++) {
+		/* Use GPIO lines as modem control lines */
+		if (mctrl_port[i] == 0)
+			s->mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
+		else if (mctrl_port[i] == 1)
+			s->mctrl_mask |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
+	}
+
+	if (s->mctrl_mask)
+		regmap_update_bits(
+			s->regmap,
+			SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
+			SC16IS7XX_IOCONTROL_MODEM_A_BIT |
+			SC16IS7XX_IOCONTROL_MODEM_B_BIT, s->mctrl_mask);
+
+	return 0;
+}
+
 static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.flags = SER_RS485_ENABLED | SER_RS485_RTS_AFTER_SEND,
 	.delay_rts_before_send = 1,
@@ -1479,12 +1547,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);
@@ -1522,6 +1584,10 @@ static int sc16is7xx_probe(struct device *dev,
 				s->p[u].irda_mode = true;
 	}
 
+	ret = sc16is7xx_setup_mctrl_ports(s);
+	if (ret)
+		goto out_ports;
+
 #ifdef CONFIG_GPIOLIB
 	ret = sc16is7xx_setup_gpio_chip(s);
 	if (ret)
@@ -1548,7 +1614,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);
 #endif
 
@@ -1572,7 +1638,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
 
-- 
2.30.2

Re: [PATCH v9 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Tue, Jul 25, 2023 at 10:23:38AM -0400, 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.
> 
> 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
> "nxp,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 DT property.
> 
> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> Cc: <stable@vger.kernel.org> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
> Cc: <stable@vger.kernel.org> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
> Cc: <stable@vger.kernel.org> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
> Cc: <stable@vger.kernel.org> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
> Cc: <stable@vger.kernel.org> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init

Where are these git commit ids from?  I don't see them in Linus's tree,
how are they supposed to be picked up by the stable developers if they
are not valid ones?

confused,

greg k-h

Re: [PATCH v9 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Hugo Villeneuve]

On Mon, 31 Jul 2023 17:58:41 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:38AM -0400, 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.
> > 
> > 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
> > "nxp,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 DT property.
> > 
> > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > Cc: <stable@vger.kernel.org> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
> > Cc: <stable@vger.kernel.org> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
> > Cc: <stable@vger.kernel.org> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
> > Cc: <stable@vger.kernel.org> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
> > Cc: <stable@vger.kernel.org> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init
> 
> Where are these git commit ids from?  I don't see them in Linus's tree,
> how are they supposed to be picked up by the stable developers if they
> are not valid ones?
> 
> confused,
> 
> greg k-h

Hi Greg,
once again, I simply misinterpreted stable-kernel-rules.rst.

I wrongly assumed that, for example, this patch had, as a prerequisite,
all the patches before it in this series, and that is why I listed
them.

So I will remove them all, since this patch doesn't have any other
requisites other than the previous patches in this series.

Maybe it would be good to add some notes about that in
stable-kernel-rules.rst?

Thank you, Hugo.

Re: [PATCH v9 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Andy Shevchenko]

On Thu, Aug 3, 2023 at 5:18 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> On Mon, 31 Jul 2023 17:58:41 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Tue, Jul 25, 2023 at 10:23:38AM -0400, Hugo Villeneuve wrote:

...

> > > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > > Cc: <stable@vger.kernel.org> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
> > > Cc: <stable@vger.kernel.org> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
> > > Cc: <stable@vger.kernel.org> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
> > > Cc: <stable@vger.kernel.org> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
> > > Cc: <stable@vger.kernel.org> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init
> >
> > Where are these git commit ids from?  I don't see them in Linus's tree,
> > how are they supposed to be picked up by the stable developers if they
> > are not valid ones?
> >
> > confused,

...

> I wrongly assumed that, for example, this patch had, as a prerequisite,
> all the patches before it in this series, and that is why I listed
> them.

The problem, as I understand it, is not that you listed them (how else
will the backporter know that this patch requires something else?) but
the format (you used wrong SHA-1 sums).

...

> So I will remove them all, since this patch doesn't have any other
> requisites other than the previous patches in this series.
>
> Maybe it would be good to add some notes about that in
> stable-kernel-rules.rst?

This probably is a good idea. Briefly looking at it I see no examples
like yours there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 06/10] serial: sc16is7xx: fix regression with GPIO configuration

[Greg KH]

On Fri, Aug 04, 2023 at 12:04:29AM +0300, Andy Shevchenko wrote:
> On Thu, Aug 3, 2023 at 5:18 PM Hugo Villeneuve <hugo@hugovil.com> wrote:
> > On Mon, 31 Jul 2023 17:58:41 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Tue, Jul 25, 2023 at 10:23:38AM -0400, Hugo Villeneuve wrote:
> 
> ...
> 
> > > > Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
> > > > Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
> > > > Cc: <stable@vger.kernel.org> # 6.1.x: 95982fad dt-bindings: sc16is7xx: Add property to change GPIO function
> > > > Cc: <stable@vger.kernel.org> # 6.1.x: 1584d572 serial: sc16is7xx: refactor GPIO controller registration
> > > > Cc: <stable@vger.kernel.org> # 6.1.x: ac2caa5a serial: sc16is7xx: remove obsolete out_thread label
> > > > Cc: <stable@vger.kernel.org> # 6.1.x: d90961ad serial: sc16is7xx: mark IOCONTROL register as volatile
> > > > Cc: <stable@vger.kernel.org> # 6.1.x: 6dae3bad serial: sc16is7xx: fix broken port 0 uart init
> > >
> > > Where are these git commit ids from?  I don't see them in Linus's tree,
> > > how are they supposed to be picked up by the stable developers if they
> > > are not valid ones?
> > >
> > > confused,
> 
> ...
> 
> > I wrongly assumed that, for example, this patch had, as a prerequisite,
> > all the patches before it in this series, and that is why I listed
> > them.

That's fine, but if you have already marked those patches for stable
inclusion, no need to list them here too.

> The problem, as I understand it, is not that you listed them (how else
> will the backporter know that this patch requires something else?) but
> the format (you used wrong SHA-1 sums).

Exactly, those are invalid sha1 values.

> > So I will remove them all, since this patch doesn't have any other
> > requisites other than the previous patches in this series.
> >
> > Maybe it would be good to add some notes about that in
> > stable-kernel-rules.rst?
> 
> This probably is a good idea. Briefly looking at it I see no examples
> like yours there.

Because it's not a thing?  Just mark all of these patches in the series
as cc: stable@ and all will happen automatically for you.  Nothing
fancy or complex here, happens daily in other subsystems just fine :)

thanks,

greg k-h

[PATCH v9 07/10] serial: sc16is7xx: fix bug when first setting GPIO direction

[Hugo Villeneuve]

From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

When configuring a pin as an output pin with a value of logic 0, we
end up as having a value of logic 1 on the output pin. Setting a
logic 0 a second time (or more) after that will correctly output a
logic 0 on the output pin.

By default, all GPIO pins are configured as inputs. When we enter
sc16is7xx_gpio_direction_output() for the first time, we first set the
desired value in IOSTATE, and then we configure the pin as an output.
The datasheet states that writing to IOSTATE register will trigger a
transfer of the value to the I/O pin configured as output, so if the
pin is configured as an input, nothing will be transferred.

Therefore, set the direction first in IODIR, and then set the desired
value in IOSTATE.

This is what is done in NXP application note AN10587.

Fixes: dfeae619d781 ("serial: sc16is7xx")
Cc: <stable@vger.kernel.org>
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index bc0a288f258d..07ae889db296 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1342,9 +1342,18 @@ static int sc16is7xx_gpio_direction_output(struct gpio_chip *chip,
 		state |= BIT(offset);
 	else
 		state &= ~BIT(offset);
-	sc16is7xx_port_write(port, SC16IS7XX_IOSTATE_REG, state);
+
+	/*
+	 * If we write IOSTATE first, and then IODIR, the output value is not
+	 * transferred to the corresponding I/O pin.
+	 * The datasheet states that each register bit will be transferred to
+	 * the corresponding I/O pin programmed as output when writing to
+	 * IOSTATE. Therefore, configure direction first with IODIR, and then
+	 * set value after with IOSTATE.
+	 */
 	sc16is7xx_port_update(port, SC16IS7XX_IODIR_REG, BIT(offset),
 			      BIT(offset));
+	sc16is7xx_port_write(port, SC16IS7XX_IOSTATE_REG, state);
 
 	return 0;
 }
-- 
2.30.2