Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X
@ 2020-07-10 20:33 Matthew Howell
  2020-07-11  6:40 ` Greg KH
  0 siblings, 1 reply; 4+ messages in thread
From: Matthew Howell @ 2020-07-10 20:33 UTC (permalink / raw)
  To: gregkh; +Cc: linux-serial, jeff.baldwin, ryan.wenglarz, 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);


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X
  2020-07-10 20:33 [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X Matthew Howell
@ 2020-07-11  6:40 ` Greg KH
  2020-07-13 16:26   ` Matthew Howell
  0 siblings, 1 reply; 4+ messages in thread
From: Greg KH @ 2020-07-11  6:40 UTC (permalink / raw)
  To: Matthew Howell; +Cc: linux-serial, jeff.baldwin, ryan.wenglarz, matthew.howell

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X
  2020-07-11  6:40 ` Greg KH
@ 2020-07-13 16:26   ` Matthew Howell
  2020-07-21 19:06     ` Greg KH
  0 siblings, 1 reply; 4+ messages in thread
From: Matthew Howell @ 2020-07-13 16:26 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-serial, jeff.baldwin, ryan.wenglarz, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X
  2020-07-13 16:26   ` Matthew Howell
@ 2020-07-21 19:06     ` Greg KH
  0 siblings, 0 replies; 4+ messages in thread
From: Greg KH @ 2020-07-21 19:06 UTC (permalink / raw)
  To: Matthew Howell; +Cc: linux-serial, jeff.baldwin, ryan.wenglarz, matthew.howell

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-10 20:33 [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X Matthew Howell
2020-07-11  6:40 ` Greg KH
2020-07-13 16:26   ` Matthew Howell
2020-07-21 19:06     ` Greg KH

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git