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

[Greg KH <gregkh@linuxfoundation.org>]

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