[PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X

[PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X

[Matthew Howell]

From: Matthew Howell <mrhowel@g.clemson.edu>

Sealevel XR17V35X based devices are inoperable on kernel versions
4.11 and above due to a change in the GPIO preconfiguration introduced in commit
7dea8165f1d. This patch fixes this by preconfiguring the GPIO on Sealevel
cards to the value (0x00) used prior to commit 7dea8165f1d

Fixes: 7dea8165f1d ("serial: exar: Preconfigure xr17v35x MPIOs as output")
Signed-off-by: Matthew Howell <mrhowel@g.clemson.edu>
---

This is a revised patch submission based on comments received on
the previous submission.
See https://www.spinics.net/lists/linux-serial/msg39348.html

I am using a different email address to address the email footer issue,
and I have attempted to fix the formatting issues.

Summary/justification of the patch is below.

With GPIOs preconfigured as per commit 7dea8165f1d all ports on Sealevel
XR17V35X based devices become stuck in high impedance mode, regardless of
dip-switch or software configuration. This causes the device to become
effectively unusable. This patch (in various forms) has been distributed
to our customers and no issues related to it have been reported.

Let me know if any changes need to be made.

--- linux/drivers/tty/serial/8250/8250_exar.c.orig    2020-07-09 11:05:03.920060577 -0400
+++ linux/drivers/tty/serial/8250/8250_exar.c    2020-07-09 11:05:25.275891627 -0400
@@ -326,7 +326,7 @@ static void setup_gpio(struct pci_dev *p
      * devices will export them as GPIOs, so we pre-configure them safely
      * as inputs.
      */
-    u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
+    u8 dir = (pcidev->vendor == PCI_VENDOR_ID_EXAR && pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL) ? 0xff : 0x00;

     writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
     writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);

Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X

[Greg KH]

On Fri, Jul 10, 2020 at 04:33:00PM -0400, Matthew Howell wrote:
> 
> From: Matthew Howell <mrhowel@g.clemson.edu>
> 
> Sealevel XR17V35X based devices are inoperable on kernel versions
> 4.11 and above due to a change in the GPIO preconfiguration introduced in commit
> 7dea8165f1d. This patch fixes this by preconfiguring the GPIO on Sealevel
> cards to the value (0x00) used prior to commit 7dea8165f1d
> 
> Fixes: 7dea8165f1d ("serial: exar: Preconfigure xr17v35x MPIOs as output")
> Signed-off-by: Matthew Howell <mrhowel@g.clemson.edu>
> ---
> 
> This is a revised patch submission based on comments received on
> the previous submission.
> See https://www.spinics.net/lists/linux-serial/msg39348.html
> 
> I am using a different email address to address the email footer issue,
> and I have attempted to fix the formatting issues.

The footer issues are fixed, but you should probably change the from:
and signed-off-by to your company address, right?

> 
> Summary/justification of the patch is below.
> 
> With GPIOs preconfigured as per commit 7dea8165f1d all ports on Sealevel
> XR17V35X based devices become stuck in high impedance mode, regardless of
> dip-switch or software configuration. This causes the device to become
> effectively unusable. This patch (in various forms) has been distributed
> to our customers and no issues related to it have been reported.

Why not put that paragraph in the changelog as well?

> 
> Let me know if any changes need to be made.
> 
> --- linux/drivers/tty/serial/8250/8250_exar.c.orig    2020-07-09 11:05:03.920060577 -0400
> +++ linux/drivers/tty/serial/8250/8250_exar.c    2020-07-09 11:05:25.275891627 -0400
> @@ -326,7 +326,7 @@ static void setup_gpio(struct pci_dev *p
>       * devices will export them as GPIOs, so we pre-configure them safely
>       * as inputs.
>       */
> -    u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
> +    u8 dir = (pcidev->vendor == PCI_VENDOR_ID_EXAR && pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL) ? 0xff : 0x00;

That's a horrible line to try to read now, right?

Why not turn it into a real if statement so we can make more sense of it
over time:

	u8 dir = 0x00;

	if ((pcidev->vendor == PCI_VENDOR_ID_EXAR) &&
	    (pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL))
		dir = 0xff;

Looks better, right?

thanks,

greg k-h

Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X

[Matthew Howell]

On 7/11/20 2:40 AM, Greg KH wrote:
> On Fri, Jul 10, 2020 at 04:33:00PM -0400, Matthew Howell wrote:
>>
>> From: Matthew Howell <mrhowel@g.clemson.edu>
>>
>> Sealevel XR17V35X based devices are inoperable on kernel versions
>> 4.11 and above due to a change in the GPIO preconfiguration introduced in commit
>> 7dea8165f1d. This patch fixes this by preconfiguring the GPIO on Sealevel
>> cards to the value (0x00) used prior to commit 7dea8165f1d
>>
>> Fixes: 7dea8165f1d ("serial: exar: Preconfigure xr17v35x MPIOs as output")
>> Signed-off-by: Matthew Howell <mrhowel@g.clemson.edu>
>> ---
>>
>> This is a revised patch submission based on comments received on
>> the previous submission.
>> See https://www.spinics.net/lists/linux-serial/msg39348.html
>>
>> I am using a different email address to address the email footer issue,
>> and I have attempted to fix the formatting issues.
>
> The footer issues are fixed, but you should probably change the from:
> and signed-off-by to your company address, right?
>

That would be optimal, yes. However, I don't have direct control over
the footer as it is enforced by our email server / group policy. Let
me know if the company email is *required* to be in the from: field
for this patch to be accepted though I will see if there is any way I
can get an exemption in this case.

>>
>> Summary/justification of the patch is below.
>>
>> With GPIOs preconfigured as per commit 7dea8165f1d all ports on Sealevel
>> XR17V35X based devices become stuck in high impedance mode, regardless of
>> dip-switch or software configuration. This causes the device to become
>> effectively unusable. This patch (in various forms) has been distributed
>> to our customers and no issues related to it have been reported.
>
> Why not put that paragraph in the changelog as well?

It is my understanding that the message above signed-off-by is
included as the commit message and should be as short as possible,
while additional information and justification is provided below the
sign-off-by line. Is that not the case? If it is preferable to be
above signed-off-by line I can move it to there.

>
>>
>> Let me know if any changes need to be made.
>>
>> --- linux/drivers/tty/serial/8250/8250_exar.c.orig    2020-07-09 11:05:03.920060577 -0400
>> +++ linux/drivers/tty/serial/8250/8250_exar.c    2020-07-09 11:05:25.275891627 -0400
>> @@ -326,7 +326,7 @@ static void setup_gpio(struct pci_dev *p
>>       * devices will export them as GPIOs, so we pre-configure them safely
>>       * as inputs.
>>       */
>> -    u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
>> +    u8 dir = (pcidev->vendor == PCI_VENDOR_ID_EXAR && pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL) ? 0xff : 0x00;
>
> That's a horrible line to try to read now, right?
>
> Why not turn it into a real if statement so we can make more sense of it
> over time:
>
>     u8 dir = 0x00;
>
>     if ((pcidev->vendor == PCI_VENDOR_ID_EXAR) &&
>         (pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL))
>         dir = 0xff;
>
> Looks better, right?
>
> thanks,
>
> greg k-h

Thanks for that feedback. It must have been unclear since the value of
dir in your if statement has the wrong value. Revised patch diff with
added comments is below.

--- linux/drivers/tty/serial/8250/8250_exar.c.orig    2020-07-09 11:05:03.920060577 -0400
+++ linux/drivers/tty/serial/8250/8250_exar.c    2020-07-13 11:54:44.386718167 -0400
@@ -326,7 +326,20 @@ static void setup_gpio(struct pci_dev *p
      * devices will export them as GPIOs, so we pre-configure them safely
      * as inputs.
      */
-    u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
+
+    u8 dir = 0x00;
+
+    if  ((pcidev->vendor == PCI_VENDOR_ID_EXAR) &&
+        (pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL))
+    {
+       // Configure GPIO as inputs for Commtech adapters
+       dir = 0xff;
+    }
+    else
+    {
+       // Configure GPIO as outputs for SeaLevel adapters
+       dir = 0x00;
+    }
 
     writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
     writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);

--

Thanks,
Matthew Howell

Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X

[Greg KH]

On Mon, Jul 13, 2020 at 12:26:54PM -0400, Matthew Howell wrote:
> 
> On 7/11/20 2:40 AM, Greg KH wrote:
> > On Fri, Jul 10, 2020 at 04:33:00PM -0400, Matthew Howell wrote:
> >>
> >> From: Matthew Howell <mrhowel@g.clemson.edu>
> >>
> >> Sealevel XR17V35X based devices are inoperable on kernel versions
> >> 4.11 and above due to a change in the GPIO preconfiguration introduced in commit
> >> 7dea8165f1d. This patch fixes this by preconfiguring the GPIO on Sealevel
> >> cards to the value (0x00) used prior to commit 7dea8165f1d
> >>
> >> Fixes: 7dea8165f1d ("serial: exar: Preconfigure xr17v35x MPIOs as output")
> >> Signed-off-by: Matthew Howell <mrhowel@g.clemson.edu>
> >> ---
> >>
> >> This is a revised patch submission based on comments received on
> >> the previous submission.
> >> See https://www.spinics.net/lists/linux-serial/msg39348.html
> >>
> >> I am using a different email address to address the email footer issue,
> >> and I have attempted to fix the formatting issues.
> >
> > The footer issues are fixed, but you should probably change the from:
> > and signed-off-by to your company address, right?
> >
> 
> That would be optimal, yes. However, I don't have direct control over
> the footer as it is enforced by our email server / group policy. Let
> me know if the company email is *required* to be in the from: field
> for this patch to be accepted though I will see if there is any way I
> can get an exemption in this case.

You can use the From: line for your company address and keep the
signed-off-by line from that same address as well and send from your
university account, that's fine and happens more than most companies
know :)

> >> Summary/justification of the patch is below.
> >>
> >> With GPIOs preconfigured as per commit 7dea8165f1d all ports on Sealevel
> >> XR17V35X based devices become stuck in high impedance mode, regardless of
> >> dip-switch or software configuration. This causes the device to become
> >> effectively unusable. This patch (in various forms) has been distributed
> >> to our customers and no issues related to it have been reported.
> >
> > Why not put that paragraph in the changelog as well?
> 
> It is my understanding that the message above signed-off-by is
> included as the commit message and should be as short as possible,
> while additional information and justification is provided below the
> sign-off-by line. Is that not the case? If it is preferable to be
> above signed-off-by line I can move it to there.

Please move it, it helps.

> 
> >
> >>
> >> Let me know if any changes need to be made.
> >>
> >> --- linux/drivers/tty/serial/8250/8250_exar.c.orig    2020-07-09 11:05:03.920060577 -0400
> >> +++ linux/drivers/tty/serial/8250/8250_exar.c    2020-07-09 11:05:25.275891627 -0400
> >> @@ -326,7 +326,7 @@ static void setup_gpio(struct pci_dev *p
> >>       * devices will export them as GPIOs, so we pre-configure them safely
> >>       * as inputs.
> >>       */
> >> -    u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
> >> +    u8 dir = (pcidev->vendor == PCI_VENDOR_ID_EXAR && pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL) ? 0xff : 0x00;
> >
> > That's a horrible line to try to read now, right?
> >
> > Why not turn it into a real if statement so we can make more sense of it
> > over time:
> >
> >     u8 dir = 0x00;
> >
> >     if ((pcidev->vendor == PCI_VENDOR_ID_EXAR) &&
> >         (pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL))
> >         dir = 0xff;
> >
> > Looks better, right?
> >
> > thanks,
> >
> > greg k-h
> 
> Thanks for that feedback. It must have been unclear since the value of
> dir in your if statement has the wrong value. Revised patch diff with
> added comments is below.

Ah, see, I misread your patch, all the more reason to do this "right".

> 
> --- linux/drivers/tty/serial/8250/8250_exar.c.orig    2020-07-09 11:05:03.920060577 -0400
> +++ linux/drivers/tty/serial/8250/8250_exar.c    2020-07-13 11:54:44.386718167 -0400
> @@ -326,7 +326,20 @@ static void setup_gpio(struct pci_dev *p
>       * devices will export them as GPIOs, so we pre-configure them safely
>       * as inputs.
>       */
> -    u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
> +
> +    u8 dir = 0x00;
> +
> +    if  ((pcidev->vendor == PCI_VENDOR_ID_EXAR) &&
> +        (pcidev->subsystem_vendor != PCI_VENDOR_ID_SEALEVEL))
> +    {
> +       // Configure GPIO as inputs for Commtech adapters
> +       dir = 0xff;
> +    }
> +    else
> +    {
> +       // Configure GPIO as outputs for SeaLevel adapters
> +       dir = 0x00;
> +    }

Coding style issues aside, looks good!

Please fix that up and resend, thanks.

greg k-h