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

From: Greg KH <gregkh@linuxfoundation.org>
To: Matthew Howell <mrhowel@g.clemson.edu>
Cc: linux-serial@vger.kernel.org, jeff.baldwin@sealevel.com,
	ryan.wenglarz@sealevel.com, matthew.howell@sealevel.com
Subject: Re: [PATCH v2] serial: exar: Fix GPIO configuration for Sealevel cards based on XR17V35X
Date: Sat, 11 Jul 2020 08:40:14 +0200	[thread overview]
Message-ID: <20200711064014.GA2786431@kroah.com> (raw)
In-Reply-To: <156b27a1-82c5-090e-0ae8-86944b849d6d@g.clemson.edu>

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