Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-06 23:04, Sudip Mukherjee wrote:
> On Monday 06 February 2017 02:45 PM, Jan Kiszka wrote:
>> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>>> From: Sudip Mukherjee 
>>>
>>> Add the serial driver for the Exar chips. And also register the
>>> platform device for the GPIO provided by the Exar chips.
>>>
>>
>> And another question: you left pci_fastcom335_setup and related things
>> untouched - did that code come later, or is it left in 8250_pci.c for a
>> reason?
> 
> That was discussed.
> Those are separate chips from different vendor and this patchset was
> specifically for Exar chips. So i suggested I will do it via separate
> patch.

If they are from different vendors, why are they addressing Exar
registers? Seems more like they are just rebranded and should indeed be
moved as well.

Jan

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-06 23:04, Sudip Mukherjee wrote:
> On Monday 06 February 2017 02:45 PM, Jan Kiszka wrote:
>> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>>> From: Sudip Mukherjee 
>>>
>>> Add the serial driver for the Exar chips. And also register the
>>> platform device for the GPIO provided by the Exar chips.
>>>
>>
>> And another question: you left pci_fastcom335_setup and related things
>> untouched - did that code come later, or is it left in 8250_pci.c for a
>> reason?
> 
> That was discussed.
> Those are separate chips from different vendor and this patchset was
> specifically for Exar chips. So i suggested I will do it via separate
> patch.

If they are from different vendors, why are they addressing Exar
registers? Seems more like they are just rebranded and should indeed be
moved as well.

Jan

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Monday 06 February 2017 01:49 PM, Jan Kiszka wrote:

On 2017-02-03 22:31, Sudip Mukherjee wrote:

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.


Well, Codethink has nothing to do with this patch. This was a voluntary
work started before I joined Codethink, but then I joined Codethink and
found very little time to finish this. So finally now its done.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html



Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)


well, I have been doing it this way from the very first day I started 
contributing.



Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Monday 06 February 2017 01:49 PM, Jan Kiszka wrote:

On 2017-02-03 22:31, Sudip Mukherjee wrote:

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.


Well, Codethink has nothing to do with this patch. This was a voluntary
work started before I joined Codethink, but then I joined Codethink and
found very little time to finish this. So finally now its done.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html



Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)


well, I have been doing it this way from the very first day I started 
contributing.



Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Monday 06 February 2017 02:06 PM, Greg Kroah-Hartman wrote:

On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:

On 2017-02-03 22:31, Sudip Mukherjee wrote:

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.


Well, Codethink has nothing to do with this patch. This was a voluntary
work started before I joined Codethink, but then I joined Codethink and
found very little time to finish this. So finally now its done.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html



Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)


There are numerous companies that do this, some create whole shell
orginizations in order to "hide" their kernel contributions for various
"interesting" reasons.

Fun stuff.  I suggest having your internal people talk to your lawyers,
they should know all about this (and if not, have those lawyers talk to
the LF lawyers...)

But that's not the issue here, we know Sudip :)


:)

Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Monday 06 February 2017 02:06 PM, Greg Kroah-Hartman wrote:

On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:

On 2017-02-03 22:31, Sudip Mukherjee wrote:

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.


Well, Codethink has nothing to do with this patch. This was a voluntary
work started before I joined Codethink, but then I joined Codethink and
found very little time to finish this. So finally now its done.

https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html



Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)


There are numerous companies that do this, some create whole shell
orginizations in order to "hide" their kernel contributions for various
"interesting" reasons.

Fun stuff.  I suggest having your internal people talk to your lawyers,
they should know all about this (and if not, have those lawyers talk to
the LF lawyers...)

But that's not the issue here, we know Sudip :)


:)

Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Monday 06 February 2017 02:45 PM, Jan Kiszka wrote:

On 2017-01-30 23:28, Sudip Mukherjee wrote:

From: Sudip Mukherjee 

Add the serial driver for the Exar chips. And also register the
platform device for the GPIO provided by the Exar chips.



And another question: you left pci_fastcom335_setup and related things
untouched - did that code come later, or is it left in 8250_pci.c for a
reason?


That was discussed.
Those are separate chips from different vendor and this patchset was 
specifically for Exar chips. So i suggested I will do it via separate patch.


Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Monday 06 February 2017 02:45 PM, Jan Kiszka wrote:

On 2017-01-30 23:28, Sudip Mukherjee wrote:

From: Sudip Mukherjee 

Add the serial driver for the Exar chips. And also register the
platform device for the GPIO provided by the Exar chips.



And another question: you left pci_fastcom335_setup and related things
untouched - did that code come later, or is it left in 8250_pci.c for a
reason?


That was discussed.
Those are separate chips from different vendor and this patchset was 
specifically for Exar chips. So i suggested I will do it via separate patch.


Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-06 20:37, Jan Kiszka wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee 
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>>
>> Reviewed-by: Andy Shevchenko 
>> Signed-off-by: Sudip Mukherjee 
> 
> ...
> 
>> +static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> + int idx, unsigned int offset,
>> + struct uart_8250_port *port)
>> +{
>> +const struct exar8250_board *board = priv->board;
>> +unsigned int bar = 0;
>> +
>> +port->port.iotype = UPIO_MEM;
>> +port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
>> +port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;
> 
> This always gives you 0 for membase because you missed to call
> pcim_iomap for bar 0.
> 
> Sorry to pick on this piece-wise, but I just ran into this bug now while
> porting patches over.
> 
>> +port->port.regshift = board->reg_shift;
>> +
>> +return 0;
>> +}
>> +
>> +static int
>> +pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +   struct uart_8250_port *port, int idx)
>> +{
>> +unsigned int offset = idx * 0x200;
>> +unsigned int baud = 1843200;
>> +
>> +port->port.uartclk = baud * 16;
>> +return default_setup(priv, pcidev, idx, offset, port);
>> +}
>> +
>> +static int
>> +pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +   struct uart_8250_port *port, int idx)
>> +{
>> +unsigned int offset = idx * 0x200;
>> +unsigned int baud = 921600;
>> +
>> +port->port.uartclk = baud * 16;
>> +return default_setup(priv, pcidev, idx, offset, port);
>> +}
>> +
>> +static void setup_gpio(u8 __iomem *p)
>> +{
>> +writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
>> +}
>> +
>> +static void *
>> +xr17v35x_register_gpio(struct pci_dev *pcidev)
>> +{
>> +struct platform_device *pdev;
>> +
>> +pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
>> +if (!pdev)
>> +return NULL;
>> +
>> +platform_set_drvdata(pdev, pcidev);
>> +if (platform_device_add(pdev) < 0) {
>> +platform_device_put(pdev);
>> +return NULL;
>> +}
>> +
>> +return pdev;
>> +}
>> +
>> +static int
>> +pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +   struct uart_8250_port *port, int idx)
>> +{
>> +const struct exar8250_board *board = priv->board;
>> +unsigned int offset = idx * 0x400;
>> +unsigned int baud = 7812500;
>> +u8 __iomem *p;
>> +int ret;
>> +
>> +port->port.uartclk = baud * 16;
>> +/*
>> + * Setup the uart clock for the devices on expansion slot to
>> + * half the clock speed of the main chip (which is 125MHz)
>> + */
>> +if (board->has_slave && idx >= 8)
>> +port->port.uartclk /= 2;
>> +
>> +p = pci_ioremap_bar(pcidev, 0);
> 
> If we move the default_setup before this, we can use pcim_iomap_table()
> and avoid this temporary mapping completely.

Actually, we should use port->port.membase here: The original code that
came from 8250_pci was broken already by always programming port 0,
irrespective of idx. Using membase, we will pick the right address for
the target port.

Jan

> 
>> +if (!p)
>> +return -ENOMEM;
>> +
>> +/* Setup Multipurpose Input/Output pins. */
>> +if (idx == 0)
>> +setup_gpio(p);
>> +
>> +writeb(0x00, p + UART_EXAR_8XMODE);
>> +writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
>> +writeb(128, p + UART_EXAR_TXTRG);
>> +writeb(128, p + UART_EXAR_RXTRG);
>> +iounmap(p);
>> +
>> +ret = default_setup(priv, pcidev, idx, offset, port);
>> +if (ret)
>> +return ret;
>> +
>> +if (idx == 0)
>> +port->port.private_data =
>> +xr17v35x_register_gpio(pcidev);
>> +
>> +return 0;
>> +}
> 
> I suppose I should still send patches on top, right?
> 
> Jan
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-06 20:37, Jan Kiszka wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee 
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>>
>> Reviewed-by: Andy Shevchenko 
>> Signed-off-by: Sudip Mukherjee 
> 
> ...
> 
>> +static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> + int idx, unsigned int offset,
>> + struct uart_8250_port *port)
>> +{
>> +const struct exar8250_board *board = priv->board;
>> +unsigned int bar = 0;
>> +
>> +port->port.iotype = UPIO_MEM;
>> +port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
>> +port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;
> 
> This always gives you 0 for membase because you missed to call
> pcim_iomap for bar 0.
> 
> Sorry to pick on this piece-wise, but I just ran into this bug now while
> porting patches over.
> 
>> +port->port.regshift = board->reg_shift;
>> +
>> +return 0;
>> +}
>> +
>> +static int
>> +pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +   struct uart_8250_port *port, int idx)
>> +{
>> +unsigned int offset = idx * 0x200;
>> +unsigned int baud = 1843200;
>> +
>> +port->port.uartclk = baud * 16;
>> +return default_setup(priv, pcidev, idx, offset, port);
>> +}
>> +
>> +static int
>> +pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +   struct uart_8250_port *port, int idx)
>> +{
>> +unsigned int offset = idx * 0x200;
>> +unsigned int baud = 921600;
>> +
>> +port->port.uartclk = baud * 16;
>> +return default_setup(priv, pcidev, idx, offset, port);
>> +}
>> +
>> +static void setup_gpio(u8 __iomem *p)
>> +{
>> +writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
>> +writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
>> +writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
>> +}
>> +
>> +static void *
>> +xr17v35x_register_gpio(struct pci_dev *pcidev)
>> +{
>> +struct platform_device *pdev;
>> +
>> +pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
>> +if (!pdev)
>> +return NULL;
>> +
>> +platform_set_drvdata(pdev, pcidev);
>> +if (platform_device_add(pdev) < 0) {
>> +platform_device_put(pdev);
>> +return NULL;
>> +}
>> +
>> +return pdev;
>> +}
>> +
>> +static int
>> +pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
>> +   struct uart_8250_port *port, int idx)
>> +{
>> +const struct exar8250_board *board = priv->board;
>> +unsigned int offset = idx * 0x400;
>> +unsigned int baud = 7812500;
>> +u8 __iomem *p;
>> +int ret;
>> +
>> +port->port.uartclk = baud * 16;
>> +/*
>> + * Setup the uart clock for the devices on expansion slot to
>> + * half the clock speed of the main chip (which is 125MHz)
>> + */
>> +if (board->has_slave && idx >= 8)
>> +port->port.uartclk /= 2;
>> +
>> +p = pci_ioremap_bar(pcidev, 0);
> 
> If we move the default_setup before this, we can use pcim_iomap_table()
> and avoid this temporary mapping completely.

Actually, we should use port->port.membase here: The original code that
came from 8250_pci was broken already by always programming port 0,
irrespective of idx. Using membase, we will pick the right address for
the target port.

Jan

> 
>> +if (!p)
>> +return -ENOMEM;
>> +
>> +/* Setup Multipurpose Input/Output pins. */
>> +if (idx == 0)
>> +setup_gpio(p);
>> +
>> +writeb(0x00, p + UART_EXAR_8XMODE);
>> +writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
>> +writeb(128, p + UART_EXAR_TXTRG);
>> +writeb(128, p + UART_EXAR_RXTRG);
>> +iounmap(p);
>> +
>> +ret = default_setup(priv, pcidev, idx, offset, port);
>> +if (ret)
>> +return ret;
>> +
>> +if (idx == 0)
>> +port->port.private_data =
>> +xr17v35x_register_gpio(pcidev);
>> +
>> +return 0;
>> +}
> 
> I suppose I should still send patches on top, right?
> 
> Jan
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.
> 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Sudip Mukherjee 

...

> +static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +  int idx, unsigned int offset,
> +  struct uart_8250_port *port)
> +{
> + const struct exar8250_board *board = priv->board;
> + unsigned int bar = 0;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> + port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;

This always gives you 0 for membase because you missed to call
pcim_iomap for bar 0.

Sorry to pick on this piece-wise, but I just ran into this bug now while
porting patches over.

> + port->port.regshift = board->reg_shift;
> +
> + return 0;
> +}
> +
> +static int
> +pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +struct uart_8250_port *port, int idx)
> +{
> + unsigned int offset = idx * 0x200;
> + unsigned int baud = 1843200;
> +
> + port->port.uartclk = baud * 16;
> + return default_setup(priv, pcidev, idx, offset, port);
> +}
> +
> +static int
> +pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +struct uart_8250_port *port, int idx)
> +{
> + unsigned int offset = idx * 0x200;
> + unsigned int baud = 921600;
> +
> + port->port.uartclk = baud * 16;
> + return default_setup(priv, pcidev, idx, offset, port);
> +}
> +
> +static void setup_gpio(u8 __iomem *p)
> +{
> + writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
> + writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
> + writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
> +}
> +
> +static void *
> +xr17v35x_register_gpio(struct pci_dev *pcidev)
> +{
> + struct platform_device *pdev;
> +
> + pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
> + if (!pdev)
> + return NULL;
> +
> + platform_set_drvdata(pdev, pcidev);
> + if (platform_device_add(pdev) < 0) {
> + platform_device_put(pdev);
> + return NULL;
> + }
> +
> + return pdev;
> +}
> +
> +static int
> +pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +struct uart_8250_port *port, int idx)
> +{
> + const struct exar8250_board *board = priv->board;
> + unsigned int offset = idx * 0x400;
> + unsigned int baud = 7812500;
> + u8 __iomem *p;
> + int ret;
> +
> + port->port.uartclk = baud * 16;
> + /*
> +  * Setup the uart clock for the devices on expansion slot to
> +  * half the clock speed of the main chip (which is 125MHz)
> +  */
> + if (board->has_slave && idx >= 8)
> + port->port.uartclk /= 2;
> +
> + p = pci_ioremap_bar(pcidev, 0);

If we move the default_setup before this, we can use pcim_iomap_table()
and avoid this temporary mapping completely.

> + if (!p)
> + return -ENOMEM;
> +
> + /* Setup Multipurpose Input/Output pins. */
> + if (idx == 0)
> + setup_gpio(p);
> +
> + writeb(0x00, p + UART_EXAR_8XMODE);
> + writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
> + writeb(128, p + UART_EXAR_TXTRG);
> + writeb(128, p + UART_EXAR_RXTRG);
> + iounmap(p);
> +
> + ret = default_setup(priv, pcidev, idx, offset, port);
> + if (ret)
> + return ret;
> +
> + if (idx == 0)
> + port->port.private_data =
> + xr17v35x_register_gpio(pcidev);
> +
> + return 0;
> +}

I suppose I should still send patches on top, right?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.
> 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Sudip Mukherjee 

...

> +static int default_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +  int idx, unsigned int offset,
> +  struct uart_8250_port *port)
> +{
> + const struct exar8250_board *board = priv->board;
> + unsigned int bar = 0;
> +
> + port->port.iotype = UPIO_MEM;
> + port->port.mapbase = pci_resource_start(pcidev, bar) + offset;
> + port->port.membase = pcim_iomap_table(pcidev)[bar] + offset;

This always gives you 0 for membase because you missed to call
pcim_iomap for bar 0.

Sorry to pick on this piece-wise, but I just ran into this bug now while
porting patches over.

> + port->port.regshift = board->reg_shift;
> +
> + return 0;
> +}
> +
> +static int
> +pci_connect_tech_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +struct uart_8250_port *port, int idx)
> +{
> + unsigned int offset = idx * 0x200;
> + unsigned int baud = 1843200;
> +
> + port->port.uartclk = baud * 16;
> + return default_setup(priv, pcidev, idx, offset, port);
> +}
> +
> +static int
> +pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +struct uart_8250_port *port, int idx)
> +{
> + unsigned int offset = idx * 0x200;
> + unsigned int baud = 921600;
> +
> + port->port.uartclk = baud * 16;
> + return default_setup(priv, pcidev, idx, offset, port);
> +}
> +
> +static void setup_gpio(u8 __iomem *p)
> +{
> + writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
> + writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOSEL_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
> + writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
> + writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOSEL_15_8);
> + writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
> +}
> +
> +static void *
> +xr17v35x_register_gpio(struct pci_dev *pcidev)
> +{
> + struct platform_device *pdev;
> +
> + pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
> + if (!pdev)
> + return NULL;
> +
> + platform_set_drvdata(pdev, pcidev);
> + if (platform_device_add(pdev) < 0) {
> + platform_device_put(pdev);
> + return NULL;
> + }
> +
> + return pdev;
> +}
> +
> +static int
> +pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> +struct uart_8250_port *port, int idx)
> +{
> + const struct exar8250_board *board = priv->board;
> + unsigned int offset = idx * 0x400;
> + unsigned int baud = 7812500;
> + u8 __iomem *p;
> + int ret;
> +
> + port->port.uartclk = baud * 16;
> + /*
> +  * Setup the uart clock for the devices on expansion slot to
> +  * half the clock speed of the main chip (which is 125MHz)
> +  */
> + if (board->has_slave && idx >= 8)
> + port->port.uartclk /= 2;
> +
> + p = pci_ioremap_bar(pcidev, 0);

If we move the default_setup before this, we can use pcim_iomap_table()
and avoid this temporary mapping completely.

> + if (!p)
> + return -ENOMEM;
> +
> + /* Setup Multipurpose Input/Output pins. */
> + if (idx == 0)
> + setup_gpio(p);
> +
> + writeb(0x00, p + UART_EXAR_8XMODE);
> + writeb(UART_FCTR_EXAR_TRGD, p + UART_EXAR_FCTR);
> + writeb(128, p + UART_EXAR_TXTRG);
> + writeb(128, p + UART_EXAR_RXTRG);
> + iounmap(p);
> +
> + ret = default_setup(priv, pcidev, idx, offset, port);
> + if (ret)
> + return ret;
> +
> + if (idx == 0)
> + port->port.private_data =
> + xr17v35x_register_gpio(pcidev);
> +
> + return 0;
> +}

I suppose I should still send patches on top, right?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.
> 

And another question: you left pci_fastcom335_setup and related things
untouched - did that code come later, or is it left in 8250_pci.c for a
reason?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.
> 

And another question: you left pci_fastcom335_setup and related things
untouched - did that code come later, or is it left in 8250_pci.c for a
reason?

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-06 15:06, Greg Kroah-Hartman wrote:
> On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
>> On 2017-02-03 22:31, Sudip Mukherjee wrote:
>>> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
 BTW, are you personally the copyright holder or your employer Codethink?
 Depends on your contractual situation, but the former is less common.
>>>
>>> Well, Codethink has nothing to do with this patch. This was a voluntary
>>> work started before I joined Codethink, but then I joined Codethink and
>>> found very little time to finish this. So finally now its done.
>>>
>>> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
>>>
>>
>> Hmm, why using your corporate email address then? This suggests a
>> different copyright situation.
>>
>> Funnily, I just received this question internally: How can you tell
>> apart if someone sends a personal contribution via his/her employer
>> account from someone contributing on behalf of a company, thus with that
>> company holding the rights? I argued that no one would do the former to
>> prevent wrong accounting, but you just proved a counterexample. :)
> 
> There are numerous companies that do this, some create whole shell
> orginizations in order to "hide" their kernel contributions for various
> "interesting" reasons.

I was not talking about companies but individuals: If they use their
company address for something written in their spare time (and their
contract allow to keep ownership of that), they needless suggest their
company holds the copyright that way around. If they stick with a
private address, it remains more clearly in their hand.

What you mentioned is a different story and can indeed be interesting
for the companies when they realize they would like for prove their code
ownership to some legal authority for whatever reason.

Anyway, off-topic now.

Jan

> 
> Fun stuff.  I suggest having your internal people talk to your lawyers,
> they should know all about this (and if not, have those lawyers talk to
> the LF lawyers...)
> 
> But that's not the issue here, we know Sudip :)
> 
> thanks,
> 
> greg k-h
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-06 15:06, Greg Kroah-Hartman wrote:
> On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
>> On 2017-02-03 22:31, Sudip Mukherjee wrote:
>>> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
 BTW, are you personally the copyright holder or your employer Codethink?
 Depends on your contractual situation, but the former is less common.
>>>
>>> Well, Codethink has nothing to do with this patch. This was a voluntary
>>> work started before I joined Codethink, but then I joined Codethink and
>>> found very little time to finish this. So finally now its done.
>>>
>>> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
>>>
>>
>> Hmm, why using your corporate email address then? This suggests a
>> different copyright situation.
>>
>> Funnily, I just received this question internally: How can you tell
>> apart if someone sends a personal contribution via his/her employer
>> account from someone contributing on behalf of a company, thus with that
>> company holding the rights? I argued that no one would do the former to
>> prevent wrong accounting, but you just proved a counterexample. :)
> 
> There are numerous companies that do this, some create whole shell
> orginizations in order to "hide" their kernel contributions for various
> "interesting" reasons.

I was not talking about companies but individuals: If they use their
company address for something written in their spare time (and their
contract allow to keep ownership of that), they needless suggest their
company holds the copyright that way around. If they stick with a
private address, it remains more clearly in their hand.

What you mentioned is a different story and can indeed be interesting
for the companies when they realize they would like for prove their code
ownership to some legal authority for whatever reason.

Anyway, off-topic now.

Jan

> 
> Fun stuff.  I suggest having your internal people talk to your lawyers,
> they should know all about this (and if not, have those lawyers talk to
> the LF lawyers...)
> 
> But that's not the issue here, we know Sudip :)
> 
> thanks,
> 
> greg k-h
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Greg Kroah-Hartman]

On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
> On 2017-02-03 22:31, Sudip Mukherjee wrote:
> > On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
> >> BTW, are you personally the copyright holder or your employer Codethink?
> >> Depends on your contractual situation, but the former is less common.
> > 
> > Well, Codethink has nothing to do with this patch. This was a voluntary
> > work started before I joined Codethink, but then I joined Codethink and
> > found very little time to finish this. So finally now its done.
> > 
> > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
> > 
> 
> Hmm, why using your corporate email address then? This suggests a
> different copyright situation.
> 
> Funnily, I just received this question internally: How can you tell
> apart if someone sends a personal contribution via his/her employer
> account from someone contributing on behalf of a company, thus with that
> company holding the rights? I argued that no one would do the former to
> prevent wrong accounting, but you just proved a counterexample. :)

There are numerous companies that do this, some create whole shell
orginizations in order to "hide" their kernel contributions for various
"interesting" reasons.

Fun stuff.  I suggest having your internal people talk to your lawyers,
they should know all about this (and if not, have those lawyers talk to
the LF lawyers...)

But that's not the issue here, we know Sudip :)

thanks,

greg k-h

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Greg Kroah-Hartman]

On Mon, Feb 06, 2017 at 02:49:07PM +0100, Jan Kiszka wrote:
> On 2017-02-03 22:31, Sudip Mukherjee wrote:
> > On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
> >> BTW, are you personally the copyright holder or your employer Codethink?
> >> Depends on your contractual situation, but the former is less common.
> > 
> > Well, Codethink has nothing to do with this patch. This was a voluntary
> > work started before I joined Codethink, but then I joined Codethink and
> > found very little time to finish this. So finally now its done.
> > 
> > https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
> > 
> 
> Hmm, why using your corporate email address then? This suggests a
> different copyright situation.
> 
> Funnily, I just received this question internally: How can you tell
> apart if someone sends a personal contribution via his/her employer
> account from someone contributing on behalf of a company, thus with that
> company holding the rights? I argued that no one would do the former to
> prevent wrong accounting, but you just proved a counterexample. :)

There are numerous companies that do this, some create whole shell
orginizations in order to "hide" their kernel contributions for various
"interesting" reasons.

Fun stuff.  I suggest having your internal people talk to your lawyers,
they should know all about this (and if not, have those lawyers talk to
the LF lawyers...)

But that's not the issue here, we know Sudip :)

thanks,

greg k-h

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-03 22:31, Sudip Mukherjee wrote:
> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>> BTW, are you personally the copyright holder or your employer Codethink?
>> Depends on your contractual situation, but the former is less common.
> 
> Well, Codethink has nothing to do with this patch. This was a voluntary
> work started before I joined Codethink, but then I joined Codethink and
> found very little time to finish this. So finally now its done.
> 
> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
> 

Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)

Regards,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-02-03 22:31, Sudip Mukherjee wrote:
> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>> BTW, are you personally the copyright holder or your employer Codethink?
>> Depends on your contractual situation, but the former is less common.
> 
> Well, Codethink has nothing to do with this patch. This was a voluntary
> work started before I joined Codethink, but then I joined Codethink and
> found very little time to finish this. So finally now its done.
> 
> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html
> 

Hmm, why using your corporate email address then? This suggests a
different copyright situation.

Funnily, I just received this question internally: How can you tell
apart if someone sends a personal contribution via his/her employer
account from someone contributing on behalf of a company, thus with that
company holding the rights? I argued that no one would do the former to
prevent wrong accounting, but you just proved a counterexample. :)

Regards,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Andy Shevchenko]

On Fri, Feb 3, 2017 at 11:31 PM, Sudip Mukherjee
 wrote:
> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>> On 2017-01-30 23:28, Sudip Mukherjee wrote:


>>> @@ -0,0 +1,396 @@
>>> +/*
>>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>>> + *
>>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>>> + *
>>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>>
>>
>> It's legally cleaner to carry over the copyright notice from the
>> original file, unless you rewrote everything (unlikely on first glance).
>> You may still add yours to the list for the significant contributions.
>
> Should i send a separate patch to modify those? Andy?

Please, do.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Andy Shevchenko]

On Fri, Feb 3, 2017 at 11:31 PM, Sudip Mukherjee
 wrote:
> On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:
>> On 2017-01-30 23:28, Sudip Mukherjee wrote:


>>> @@ -0,0 +1,396 @@
>>> +/*
>>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>>> + *
>>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>>> + *
>>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>>
>>
>> It's legally cleaner to carry over the copyright notice from the
>> original file, unless you rewrote everything (unlikely on first glance).
>> You may still add yours to the list for the significant contributions.
>
> Should i send a separate patch to modify those? Andy?

Please, do.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:

On 2017-01-30 23:28, Sudip Mukherjee wrote:

From: Sudip Mukherjee 

Add the serial driver for the Exar chips. And also register the
platform device for the GPIO provided by the Exar chips.


"Also" means you are doing two things in one patch - was this already
discussed and accepted in previous review rounds? If so, ignore my
comment, but I would have asked for two patches, one that just
translates the existing code and another that adds this new feature.



Like Andy replied, this is already in tty-next.



Reviewed-by: Andy Shevchenko 
Signed-off-by: Sudip Mukherjee 
---

Andy,
I have added the if (!board) check, but I am not sure how board can be
NULL here. If probe executes that will mean there was a match of the
device id and so in that case board can not be NULL.

  drivers/tty/serial/8250/8250_exar.c | 396 
  drivers/tty/serial/8250/Kconfig |   4 +
  drivers/tty/serial/8250/Makefile|   1 +
  3 files changed, 401 insertions(+)
  create mode 100644 drivers/tty/serial/8250/8250_exar.c

diff --git a/drivers/tty/serial/8250/8250_exar.c 
b/drivers/tty/serial/8250/8250_exar.c
new file mode 100644
index 000..ba1f359
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -0,0 +1,396 @@
+/*
+ *  Probe module for 8250/16550-type Exar chips PCI serial ports.
+ *
+ *  Based on drivers/tty/serial/8250/8250_pci.c,
+ *
+ *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.


It's legally cleaner to carry over the copyright notice from the
original file, unless you rewrote everything (unlikely on first glance).
You may still add yours to the list for the significant contributions.


Should i send a separate patch to modify those? Andy?



BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.


Well, Codethink has nothing to do with this patch. This was a voluntary 
work started before I joined Codethink, but then I joined Codethink and 
found very little time to finish this. So finally now its done.


https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html


Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Sudip Mukherjee]

On Friday 03 February 2017 02:02 PM, Jan Kiszka wrote:

On 2017-01-30 23:28, Sudip Mukherjee wrote:

From: Sudip Mukherjee 

Add the serial driver for the Exar chips. And also register the
platform device for the GPIO provided by the Exar chips.


"Also" means you are doing two things in one patch - was this already
discussed and accepted in previous review rounds? If so, ignore my
comment, but I would have asked for two patches, one that just
translates the existing code and another that adds this new feature.



Like Andy replied, this is already in tty-next.



Reviewed-by: Andy Shevchenko 
Signed-off-by: Sudip Mukherjee 
---

Andy,
I have added the if (!board) check, but I am not sure how board can be
NULL here. If probe executes that will mean there was a match of the
device id and so in that case board can not be NULL.

  drivers/tty/serial/8250/8250_exar.c | 396 
  drivers/tty/serial/8250/Kconfig |   4 +
  drivers/tty/serial/8250/Makefile|   1 +
  3 files changed, 401 insertions(+)
  create mode 100644 drivers/tty/serial/8250/8250_exar.c

diff --git a/drivers/tty/serial/8250/8250_exar.c 
b/drivers/tty/serial/8250/8250_exar.c
new file mode 100644
index 000..ba1f359
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -0,0 +1,396 @@
+/*
+ *  Probe module for 8250/16550-type Exar chips PCI serial ports.
+ *
+ *  Based on drivers/tty/serial/8250/8250_pci.c,
+ *
+ *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.


It's legally cleaner to carry over the copyright notice from the
original file, unless you rewrote everything (unlikely on first glance).
You may still add yours to the list for the significant contributions.


Should i send a separate patch to modify those? Andy?



BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.


Well, Codethink has nothing to do with this patch. This was a voluntary 
work started before I joined Codethink, but then I joined Codethink and 
found very little time to finish this. So finally now its done.


https://lists.kernelnewbies.org/pipermail/kernelnewbies/2015-November/015372.html


Regards
Sudip

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Andy Shevchenko]

On Fri, Feb 3, 2017 at 4:02 PM, Jan Kiszka  wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee 
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>
> "Also" means you are doing two things in one patch - was this already
> discussed and accepted in previous review rounds? If so, ignore my
> comment, but I would have asked for two patches, one that just
> translates the existing code and another that adds this new feature.

Since it's already in Greg's tty-next, no point to fix anymore this
particular part.
However, you are right that few lines of code might be split to a
separate change.

>> +/*
>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>> + *
>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>> + *
>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>
> It's legally cleaner to carry over the copyright notice from the
> original file, unless you rewrote everything (unlikely on first glance).
> You may still add yours to the list for the significant contributions.
>
> BTW, are you personally the copyright holder or your employer Codethink?
> Depends on your contractual situation, but the former is less common.

This is good comment and I think it needs to be addressed (as a
separate change due to above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Andy Shevchenko]

On Fri, Feb 3, 2017 at 4:02 PM, Jan Kiszka  wrote:
> On 2017-01-30 23:28, Sudip Mukherjee wrote:
>> From: Sudip Mukherjee 
>>
>> Add the serial driver for the Exar chips. And also register the
>> platform device for the GPIO provided by the Exar chips.
>
> "Also" means you are doing two things in one patch - was this already
> discussed and accepted in previous review rounds? If so, ignore my
> comment, but I would have asked for two patches, one that just
> translates the existing code and another that adds this new feature.

Since it's already in Greg's tty-next, no point to fix anymore this
particular part.
However, you are right that few lines of code might be split to a
separate change.

>> +/*
>> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
>> + *
>> + *  Based on drivers/tty/serial/8250/8250_pci.c,
>> + *
>> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.
>
> It's legally cleaner to carry over the copyright notice from the
> original file, unless you rewrote everything (unlikely on first glance).
> You may still add yours to the list for the significant contributions.
>
> BTW, are you personally the copyright holder or your employer Codethink?
> Depends on your contractual situation, but the former is less common.

This is good comment and I think it needs to be addressed (as a
separate change due to above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.

"Also" means you are doing two things in one patch - was this already
discussed and accepted in previous review rounds? If so, ignore my
comment, but I would have asked for two patches, one that just
translates the existing code and another that adds this new feature.

> 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Sudip Mukherjee 
> ---
> 
> Andy,
> I have added the if (!board) check, but I am not sure how board can be
> NULL here. If probe executes that will mean there was a match of the
> device id and so in that case board can not be NULL.
> 
>  drivers/tty/serial/8250/8250_exar.c | 396 
> 
>  drivers/tty/serial/8250/Kconfig |   4 +
>  drivers/tty/serial/8250/Makefile|   1 +
>  3 files changed, 401 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_exar.c
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c 
> b/drivers/tty/serial/8250/8250_exar.c
> new file mode 100644
> index 000..ba1f359
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -0,0 +1,396 @@
> +/*
> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
> + *
> + *  Based on drivers/tty/serial/8250/8250_pci.c,
> + *
> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.

It's legally cleaner to carry over the copyright notice from the
original file, unless you rewrote everything (unlikely on first glance).
You may still add yours to the list for the significant contributions.

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v13 1/2] serial: exar: split out the exar code from 8250_pci

[Jan Kiszka]

On 2017-01-30 23:28, Sudip Mukherjee wrote:
> From: Sudip Mukherjee 
> 
> Add the serial driver for the Exar chips. And also register the
> platform device for the GPIO provided by the Exar chips.

"Also" means you are doing two things in one patch - was this already
discussed and accepted in previous review rounds? If so, ignore my
comment, but I would have asked for two patches, one that just
translates the existing code and another that adds this new feature.

> 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Sudip Mukherjee 
> ---
> 
> Andy,
> I have added the if (!board) check, but I am not sure how board can be
> NULL here. If probe executes that will mean there was a match of the
> device id and so in that case board can not be NULL.
> 
>  drivers/tty/serial/8250/8250_exar.c | 396 
> 
>  drivers/tty/serial/8250/Kconfig |   4 +
>  drivers/tty/serial/8250/Makefile|   1 +
>  3 files changed, 401 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_exar.c
> 
> diff --git a/drivers/tty/serial/8250/8250_exar.c 
> b/drivers/tty/serial/8250/8250_exar.c
> new file mode 100644
> index 000..ba1f359
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -0,0 +1,396 @@
> +/*
> + *  Probe module for 8250/16550-type Exar chips PCI serial ports.
> + *
> + *  Based on drivers/tty/serial/8250/8250_pci.c,
> + *
> + *  Copyright (C) 2017 Sudip Mukherjee, All Rights Reserved.

It's legally cleaner to carry over the copyright notice from the
original file, unless you rewrote everything (unlikely on first glance).
You may still add yours to the list for the significant contributions.

BTW, are you personally the copyright holder or your employer Codethink?
Depends on your contractual situation, but the former is less common.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux