linux-kernel - Re: [PATCH v7 2/3] serial: exar: split out the exar code from 8250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 08 Jan 2017 11:11:29 +0000
From:   Sudip Mukherjee <sudipm.mukherjee@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v7 2/3] serial: exar: split out the exar code from 8250_pci

On Sunday 08 January 2017 01:02 AM, Andy Shevchenko wrote:
> On Sun, Jan 8, 2017 at 1:57 AM, Sudip Mukherjee
> <sudipm.mukherjee@...il.com> wrote:
>> Add the serial driver for the exar chips. And also register the
>> platform device for the exar gpio.
>
> Did you ignore some comments?
>
> IIRC I recommended to use proper vendor name like Exar (or how is it spelled?).

oops, sorry. I missed that.

>
>> Headers, if arranged in alphabetical order, will give a build warning.
>
> I think I know how to make it better.
>
>> And thanks for revewing that v6. I think those were the worst patch I
>> have ever posted.
>
> You may do even more better. See below.
>
>> +#undef DEBUG
>
>> +#include <asm/byteorder.h>
>
> (1)
>
>> +#include <linux/pci.h>
>
> Squeez this to the rest
>
>> +#include <linux/8250_pci.h>
>
> (2)
>
>> +#include <linux/bitops.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/serial_core.h>
>> +#include <linux/serial_reg.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +#include <linux/tty.h>
>
> You perhaps need something like this here:
>
> + empty line
> (1) +#include <asm/byteorder.h>
>
>> +
>
> (2) +#include <linux/8250_pci.h>
>
>> +#include "8250.h"

not sure if I have understood this header thing properly. But let me 
play with it and see,

>
>> +
>> +#define PCI_NUM_BAR_RESOURCES  6
<snip>
>> +static struct pci_device_id exar_pci_tbl[] = {
>> +       {       PCI_DEVICE_SUB(PCI_VENDOR_ID_EXAR,
>> +               PCI_DEVICE_ID_EXAR_XR17C152,
>> +               PCI_SUBVENDOR_ID_CONNECT_TECH,
>> +               PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_2_232), 0, 0,
>> +               (kernel_ulong_t)&pbn_b0_2_1843200_200 },
>
> You ignored my comment regarding to make a macro(s).

I used PCI_DEVICE_SUB() and PCI_VDEVICE(), but yes, custom macro might 
be better here. I was trying to have one custom macro, but with two 
different macros it should be ok.

>
> Moreover, some of data, like number of ports, can be easily calculated
> from device ID.

yes, but since the baudrate is different i will need to have different 
board id for it.
Like: 'PCI_SUBDEVICE_ID_CONNECT_TECH_PCI_UART_4_232' has a device id 
'PCI_DEVICE_ID_EXAR_XR17C154' is having a baudrate of 1843200 but the 
other devices with the same deviceid will have a baudrate of 921600.

unless, in the probe I compare the subvendor with 
PCI_SUBVENDOR_ID_CONNECT_TECH and modify the baud. Let me try.

Thanks for reviewing.

Regards
Sudip