From: Matthew Howell <mrhowel@g.clemson.edu>
To: Greg KH <gregkh@linuxfoundation.org>
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: Mon, 13 Jul 2020 12:26:54 -0400 [thread overview]
Message-ID: <c2c183d1-4a90-199a-23ad-2a736e431b27@g.clemson.edu> (raw)
In-Reply-To: <20200711064014.GA2786431@kroah.com>
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
next prev parent reply other threads:[~2020-07-13 16:26 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-07-21 19:06 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c2c183d1-4a90-199a-23ad-2a736e431b27@g.clemson.edu \
--to=mrhowel@g.clemson.edu \
--cc=gregkh@linuxfoundation.org \
--cc=jeff.baldwin@sealevel.com \
--cc=linux-serial@vger.kernel.org \
--cc=matthew.howell@sealevel.com \
--cc=ryan.wenglarz@sealevel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).