Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311907 > unrolled thread

[PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Peter Hung <hpeter@gmail.com> - 2016-01-19 03:50 +0100
    [PATCH 1/3] serial: 8250_pci: Remove Fintek PCIE UART driver Peter Hung <hpeter@gmail.com> - 2016-01-19 03:50 +0100
    Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-01-19 05:00 +0100
      Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Peter Hung <hpeter@gmail.com> - 2016-01-19 09:50 +0100
        Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-01-19 10:40 +0100
        Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-19 13:40 +0100
          Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-01-19 14:30 +0100
            Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Peter Hung <hpeter@gmail.com> - 2016-01-20 04:00 +0100
              Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-01-20 07:30 +0100
                Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Peter Hung <hpeter@gmail.com> - 2016-01-20 09:30 +0100
                  Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-01-22 12:00 +0100
                    Re: [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-22 14:50 +0100

#1311907 — [PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file

Fintek F81504/508/512 is a multi-functional PCIE device. It contains
GPIO and serial port with high baudrate & RTS auto direction for RS485.

The serial ports support from 50bps to 1.5Mbps with Linux baudrate
define excluding 1.0Mbps due to not support 16MHz clock source.

IC function list:
	F81504: Max 2x8 GPIOs and max 4 serial ports
			port2/3 are multi-function
	F81508: Max 6x8 GPIOs and max 8 serial ports
			port2/3 are multi-function, port8/9/10/11 are gpio only
	F81512: Max 6x8 GPIOs and max 12 serial ports
			port2/3/8/9/10/11 are multi-function

We'll spilt from 8250_pci.c to new file 8250_fintek_pci.c and make it
as a kernel module with first & second patch, implements GPIOLIB with
third patch. 

Peter Hung (3):
  serial: 8250_pci: Remove Fintek PCIE UART driver
  8250_fintek_pci: Add Fintek PCIE UART driver
  8250_fintek_pci: Add GPIOLIB support

 drivers/tty/serial/8250/8250_fintek_pci.c | 767 ++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/8250_pci.c        | 201 --------
 drivers/tty/serial/8250/Kconfig           |   9 +
 drivers/tty/serial/8250/Makefile          |   1 +
 4 files changed, 777 insertions(+), 201 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_fintek_pci.c

-- 
1.9.1

[toc] | [next] | [standalone]

#1311912 — [PATCH 1/3] serial: 8250_pci: Remove Fintek PCIE UART driver

Remove Fintek F81504/508/512 PCIE-to-UART device driver from 8250_pci.c

Signed-off-by: Peter Hung <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_pci.c | 201 -------------------------------------
 1 file changed, 201 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 4097f3f..0eeb4a3 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -1527,156 +1527,6 @@ pci_brcm_trumanage_setup(struct serial_private *priv,
 	return ret;
 }
 
-/* RTS will control by MCR if this bit is 0 */
-#define FINTEK_RTS_CONTROL_BY_HW	BIT(4)
-/* only worked with FINTEK_RTS_CONTROL_BY_HW on */
-#define FINTEK_RTS_INVERT		BIT(5)
-
-/* We should do proper H/W transceiver setting before change to RS485 mode */
-static int pci_fintek_rs485_config(struct uart_port *port,
-			       struct serial_rs485 *rs485)
-{
-	u8 setting;
-	u8 *index = (u8 *) port->private_data;
-	struct pci_dev *pci_dev = container_of(port->dev, struct pci_dev,
-						dev);
-
-	pci_read_config_byte(pci_dev, 0x40 + 8 * *index + 7, &setting);
-
-	if (!rs485)
-		rs485 = &port->rs485;
-	else if (rs485->flags & SER_RS485_ENABLED)
-		memset(rs485->padding, 0, sizeof(rs485->padding));
-	else
-		memset(rs485, 0, sizeof(*rs485));
-
-	/* F81504/508/512 not support RTS delay before or after send */
-	rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND;
-
-	if (rs485->flags & SER_RS485_ENABLED) {
-		/* Enable RTS H/W control mode */
-		setting |= FINTEK_RTS_CONTROL_BY_HW;
-
-		if (rs485->flags & SER_RS485_RTS_ON_SEND) {
-			/* RTS driving high on TX */
-			setting &= ~FINTEK_RTS_INVERT;
-		} else {
-			/* RTS driving low on TX */
-			setting |= FINTEK_RTS_INVERT;
-		}
-
-		rs485->delay_rts_after_send = 0;
-		rs485->delay_rts_before_send = 0;
-	} else {
-		/* Disable RTS H/W control mode */
-		setting &= ~(FINTEK_RTS_CONTROL_BY_HW | FINTEK_RTS_INVERT);
-	}
-
-	pci_write_config_byte(pci_dev, 0x40 + 8 * *index + 7, setting);
-
-	if (rs485 != &port->rs485)
-		port->rs485 = *rs485;
-
-	return 0;
-}
-
-static int pci_fintek_setup(struct serial_private *priv,
-			    const struct pciserial_board *board,
-			    struct uart_8250_port *port, int idx)
-{
-	struct pci_dev *pdev = priv->dev;
-	u8 *data;
-	u8 config_base;
-	u16 iobase;
-
-	config_base = 0x40 + 0x08 * idx;
-
-	/* Get the io address from configuration space */
-	pci_read_config_word(pdev, config_base + 4, &iobase);
-
-	dev_dbg(&pdev->dev, "%s: idx=%d iobase=0x%x", __func__, idx, iobase);
-
-	port->port.iotype = UPIO_PORT;
-	port->port.iobase = iobase;
-	port->port.rs485_config = pci_fintek_rs485_config;
-
-	data = devm_kzalloc(&pdev->dev, sizeof(u8), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	/* preserve index in PCI configuration space */
-	*data = idx;
-	port->port.private_data = data;
-
-	return 0;
-}
-
-static int pci_fintek_init(struct pci_dev *dev)
-{
-	unsigned long iobase;
-	u32 max_port, i;
-	u32 bar_data[3];
-	u8 config_base;
-	struct serial_private *priv = pci_get_drvdata(dev);
-	struct uart_8250_port *port;
-
-	switch (dev->device) {
-	case 0x1104: /* 4 ports */
-	case 0x1108: /* 8 ports */
-		max_port = dev->device & 0xff;
-		break;
-	case 0x1112: /* 12 ports */
-		max_port = 12;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	/* Get the io address dispatch from the BIOS */
-	pci_read_config_dword(dev, 0x24, &bar_data[0]);
-	pci_read_config_dword(dev, 0x20, &bar_data[1]);
-	pci_read_config_dword(dev, 0x1c, &bar_data[2]);
-
-	for (i = 0; i < max_port; ++i) {
-		/* UART0 configuration offset start from 0x40 */
-		config_base = 0x40 + 0x08 * i;
-
-		/* Calculate Real IO Port */
-		iobase = (bar_data[i / 4] & 0xffffffe0) + (i % 4) * 8;
-
-		/* Enable UART I/O port */
-		pci_write_config_byte(dev, config_base + 0x00, 0x01);
-
-		/* Select 128-byte FIFO and 8x FIFO threshold */
-		pci_write_config_byte(dev, config_base + 0x01, 0x33);
-
-		/* LSB UART */
-		pci_write_config_byte(dev, config_base + 0x04,
-				(u8)(iobase & 0xff));
-
-		/* MSB UART */
-		pci_write_config_byte(dev, config_base + 0x05,
-				(u8)((iobase & 0xff00) >> 8));
-
-		pci_write_config_byte(dev, config_base + 0x06, dev->irq);
-
-		if (priv) {
-			/* re-apply RS232/485 mode when
-			 * pciserial_resume_ports()
-			 */
-			port = serial8250_get_port(priv->line[i]);
-			pci_fintek_rs485_config(&port->port, NULL);
-		} else {
-			/* First init without port data
-			 * force init to RS232 Mode
-			 */
-			pci_write_config_byte(dev, config_base + 0x07, 0x01);
-		}
-	}
-
-	return max_port;
-}
-
 static int skip_tx_en_setup(struct serial_private *priv,
 			const struct pciserial_board *board,
 			struct uart_8250_port *port, int idx)
@@ -2707,31 +2557,6 @@ static struct pci_serial_quirk pci_serial_quirks[] __refdata = {
 		.subdevice	= PCI_ANY_ID,
 		.setup		= pci_brcm_trumanage_setup,
 	},
-	{
-		.vendor		= 0x1c29,
-		.device		= 0x1104,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= pci_fintek_setup,
-		.init		= pci_fintek_init,
-	},
-	{
-		.vendor		= 0x1c29,
-		.device		= 0x1108,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= pci_fintek_setup,
-		.init		= pci_fintek_init,
-	},
-	{
-		.vendor		= 0x1c29,
-		.device		= 0x1112,
-		.subvendor	= PCI_ANY_ID,
-		.subdevice	= PCI_ANY_ID,
-		.setup		= pci_fintek_setup,
-		.init		= pci_fintek_init,
-	},
-
 	/*
 	 * Default "match everything" terminator entry
 	 */
@@ -2933,9 +2758,6 @@ enum pci_board_num_t {
 	pbn_omegapci,
 	pbn_NETMOS9900_2s_115200,
 	pbn_brcm_trumanage,
-	pbn_fintek_4,
-	pbn_fintek_8,
-	pbn_fintek_12,
 	pbn_wch384_4,
 	pbn_pericom_PI7C9X7951,
 	pbn_pericom_PI7C9X7952,
@@ -3738,24 +3560,6 @@ static struct pciserial_board pci_boards[] = {
 		.reg_shift	= 2,
 		.base_baud	= 115200,
 	},
-	[pbn_fintek_4] = {
-		.num_ports	= 4,
-		.uart_offset	= 8,
-		.base_baud	= 115200,
-		.first_offset	= 0x40,
-	},
-	[pbn_fintek_8] = {
-		.num_ports	= 8,
-		.uart_offset	= 8,
-		.base_baud	= 115200,
-		.first_offset	= 0x40,
-	},
-	[pbn_fintek_12] = {
-		.num_ports	= 12,
-		.uart_offset	= 8,
-		.base_baud	= 115200,
-		.first_offset	= 0x40,
-	},
 	[pbn_wch384_4] = {
 		.flags		= FL_BASE0,
 		.num_ports	= 4,
@@ -5581,11 +5385,6 @@ static struct pci_device_id serial_pci_tbl[] = {
 		0,
 		0, pbn_exar_XR17V358 },
 
-	/* Fintek PCI serial cards */
-	{ PCI_DEVICE(0x1c29, 0x1104), .driver_data = pbn_fintek_4 },
-	{ PCI_DEVICE(0x1c29, 0x1108), .driver_data = pbn_fintek_8 },
-	{ PCI_DEVICE(0x1c29, 0x1112), .driver_data = pbn_fintek_12 },
-
 	/*
 	 * These entries match devices with class COMMUNICATION_SERIAL,
 	 * COMMUNICATION_MODEM or COMMUNICATION_MULTISERIAL
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1311932

[[PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file] On 19/01/2016 (Tue 10:41) Peter Hung wrote:

> Fintek F81504/508/512 is a multi-functional PCIE device. It contains
> GPIO and serial port with high baudrate & RTS auto direction for RS485.
> 

Some general high level comments (meaning I've not looked at the code in
detail, but have looked at this series) and I wonder about some issues:

> The serial ports support from 50bps to 1.5Mbps with Linux baudrate
> define excluding 1.0Mbps due to not support 16MHz clock source.

How does this differ from what was achieved or possible with the old way
of things?  What was the limitation in the existing 8250 code sharing
that required Fintek code to fork and become independent? 

How much code was just copied 8250 boilerplate vs. being a new
implementation?  The diffstat shows approx 500 lines of new code.  What
does that add vs. just copying?

Don't get me wrong -- forking workarounds for buggy hardware into
smaller workaround files and/or Kconfigs can be a win for everyone else,
but we should be clear on why we do them.

> 
> IC function list:
> 	F81504: Max 2x8 GPIOs and max 4 serial ports
> 			port2/3 are multi-function
> 	F81508: Max 6x8 GPIOs and max 8 serial ports
> 			port2/3 are multi-function, port8/9/10/11 are gpio only
> 	F81512: Max 6x8 GPIOs and max 12 serial ports
> 			port2/3/8/9/10/11 are multi-function
> 
> We'll spilt from 8250_pci.c to new file 8250_fintek_pci.c and make it
> as a kernel module with first & second patch, implements GPIOLIB with
> third patch. 

> 
> Peter Hung (3):
>   serial: 8250_pci: Remove Fintek PCIE UART driver

If someone had 8250 (PCI) builtin before, and Fintek stops working,
they will most guaranteed bisect to this commit above where you remove
support.  That is less than ideal.  We try to avoid code deletions or
Kconfig addtions that will be obvious bisect magnets.

>   8250_fintek_pci: Add Fintek PCIE UART driver

This creates a new Kconfig var. which is default=m.  How does that work
if people were using these for built-in early console support in the
past?  Are these cards universal, or should it be default=m if (...)
based on a Kconfig where this hardware exists?

>   8250_fintek_pci: Add GPIOLIB support

What does this add?  The commit log is not at all clear.  Leaving me to
ask if it does belong in the core PCI support code at all?  I honestly
don't know, since I don't know the hardware details here.  The commit
long logs could go a long way to closing this knowledge gap if the 0/N
listed the shortcomings and the 3/3 here indicated what the GPIO magic
had managed to add.

Again, this may be obvious to others, but the long logs should try and
give a hint to people on the fringe who maybe don't have all the
specific Fintek hardware details when reading the logs.

P.
--

> 
>  drivers/tty/serial/8250/8250_fintek_pci.c | 767 ++++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/8250_pci.c        | 201 --------
>  drivers/tty/serial/8250/Kconfig           |   9 +
>  drivers/tty/serial/8250/Makefile          |   1 +
>  4 files changed, 777 insertions(+), 201 deletions(-)
>  create mode 100644 drivers/tty/serial/8250/8250_fintek_pci.c
> 
> -- 
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1312005

Hi Paul,

Paul Gortmaker 於 2016/1/19 上午 11:56 寫道:
>> The serial ports support from 50bps to 1.5Mbps with Linux baudrate
>> define excluding 1.0Mbps due to not support 16MHz clock source.
>
> How does this differ from what was achieved or possible with the old way
> of things?  What was the limitation in the existing 8250 code sharing
> that required Fintek code to fork and become independent?

The architecture of 8250_pci.c is good for PCIE device with 8250
compatible serial ports. We want to implement all functions of
F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
implement GPIOLIB in 8250_pci.c

Could I implement GPIOLIB within 8250_pci.c instead of a newer file?

> How much code was just copied 8250 boilerplate vs. being a new
> implementation?  The diffstat shows approx 500 lines of new code.  What
> does that add vs. just copying?

Due to this IC contains 8250-compatible ports, the most functions is
copy from fintek section of 8250_pci.c. The differences are highbaud
rate & GPIOLIB implementations.

>
> If someone had 8250 (PCI) builtin before, and Fintek stops working,
> they will most guaranteed bisect to this commit above where you remove
> support.  That is less than ideal.  We try to avoid code deletions or
> Kconfig addtions that will be obvious bisect magnets.

It can be prevented if implements GPIOLIB in 8250_pci.c.

>>    8250_fintek_pci: Add Fintek PCIE UART driver
>
> This creates a new Kconfig var. which is default=m.  How does that work
> if people were using these for built-in early console support in the
> past?  Are these cards universal, or should it be default=m if (...)
> based on a Kconfig where this hardware exists?

Thanks for point this out, for the early console I should make the
default mode to SERIAL_8250 if it need to split as a new file.

>>    8250_fintek_pci: Add GPIOLIB support
>
> What does this add?  The commit log is not at all clear.  Leaving me to
> ask if it does belong in the core PCI support code at all?  I honestly
> don't know, since I don't know the hardware details here.  The commit
> long logs could go a long way to closing this knowledge gap if the 0/N
> listed the shortcomings and the 3/3 here indicated what the GPIO magic
> had managed to add.

Sorry for the ambiguous logs. We'll implement GPIOLIB due to the
following circumstance.

Some H/W manufacturer use this IC and transform some port into GPIO
mode. The current 8250_pci.c not handle this so it maybe confuse
end-user.

> Again, this may be obvious to others, but the long logs should try and
> give a hint to people on the fringe who maybe don't have all the
> specific Fintek hardware details when reading the logs.
>

I'll try to make more sense with long long.
Thanks for your advices,
-- 
With Best Regards,
Peter Hung

[toc] | [prev] | [next] | [standalone]

#1312043

On Tue, 2016-01-19 at 16:45 +0800, Peter Hung wrote:
> Hi Paul,
> 
> Paul Gortmaker 於 2016/1/19 上午 11:56 寫道:
> > > The serial ports support from 50bps to 1.5Mbps with Linux
> > > baudrate
> > > define excluding 1.0Mbps due to not support 16MHz clock source.
> > 
> > How does this differ from what was achieved or possible with the
> > old way
> > of things?  What was the limitation in the existing 8250 code
> > sharing
> > that required Fintek code to fork and become independent?
> 
> The architecture of 8250_pci.c is good for PCIE device with 8250
> compatible serial ports. We want to implement all functions of
> F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> implement GPIOLIB in 8250_pci.c
> 
> Could I implement GPIOLIB within 8250_pci.c instead of a newer file?

Hm… So, can we stick with separate driver, or you're gonna shake for
each reviewer's comment?

> 
> > How much code was just copied 8250 boilerplate vs. being a new
> > implementation?  The diffstat shows approx 500 lines of new
> > code.  What
> > does that add vs. just copying?
> 
> Due to this IC contains 8250-compatible ports, the most functions is
> copy from fintek section of 8250_pci.c. The differences are highbaud
> rate & GPIOLIB implementations.

I agree with Paul, I think what you have done is to:

1) split out existing code to separate driver (no your changes, but
minimum necessary to this split) — one patch!
2) clean up it (at least I see the old PM code which should be
refactored)
3) enhance functionality accordingly to what you need.

> 
> > 
> > If someone had 8250 (PCI) builtin before, and Fintek stops working,
> > they will most guaranteed bisect to this commit above where you
> > remove
> > support.  That is less than ideal.  We try to avoid code deletions
> > or
> > Kconfig addtions that will be obvious bisect magnets.
> 
> It can be prevented if implements GPIOLIB in 8250_pci.c.

Yeah, see item 1) above.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1312180

> The architecture of 8250_pci.c is good for PCIE device with 8250
> compatible serial ports. We want to implement all functions of
> F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> implement GPIOLIB in 8250_pci.c

Your device is multi-function. Create an MFD driver for it. Make the
8250 driver bind to the MFD, and provide your own baud rate methods
within the standard 8250 layer

Implement the GPIO lines in a GPIO driver that also binds to the MFD and
lives in drivers/gpio

All the needed pieces already exist to implement it cleanly this way
without duplicating a ton of code.

Alan

[toc] | [prev] | [next] | [standalone]

#1312222

On Tue, 2016-01-19 at 12:33 +0000, One Thousand Gnomes wrote:
> > The architecture of 8250_pci.c is good for PCIE device with 8250
> > compatible serial ports. We want to implement all functions of
> > F81504/508/512, but it'll make 8250_pci.c bloated and complex if we
> > implement GPIOLIB in 8250_pci.c
> 
> Your device is multi-function. Create an MFD driver for it. Make the
> 8250 driver bind to the MFD, and provide your own baud rate methods
> within the standard 8250 layer

Ouch, somehow I missed this one!

Peter, Alan's suggestion is really worth to try.

> 
> Implement the GPIO lines in a GPIO driver that also binds to the MFD
> and
> lives in drivers/gpio
> 
> All the needed pieces already exist to implement it cleanly this way
> without duplicating a ton of code.
> 
> Alan

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1312852

Hi Andy, Alan

Andy Shevchenko 於 2016/1/19 下午 09:21 寫道:
>> Your device is multi-function. Create an MFD driver for it. Make the
>> 8250 driver bind to the MFD, and provide your own baud rate methods
>> within the standard 8250 layer
>
> Ouch, somehow I missed this one!
>
> Peter, Alan's suggestion is really worth to try.
>

Thanks for point this. It seems good to probe on MFD driver, them MFD
register platform devices to invoke platform driver to initialize
sub-parts. I'll try to survey first.

But I had a new question, If I really do it with MFD subsystem, it'll
split into 3 parts, MFD probe(driver/mfd) / GPIO (driver/gpio) / UART
(drivers/tty/serial/8250). It'll cross more than 2 subsystems and 
maintainers How should I do to organize the patches?

For examples, I should remove the probe function in 8250_pci.c and
move it to new MFD file. It should organize it in the same patch as Paul
said, but this patch will need 2 subsystem maintainer to do with the
same patch, it seems weird.

Andy had cc "[PATCH v5] serial: 8250: add gpio support to exar" to me,
could I use the same way to do GPIOLIB? First add a platform driver
for F81504 gpio and add platform device into 8250_pci.c? It seems to
be good and simple to implement.

-- 
With Best Regards,
Peter Hung

[toc] | [prev] | [next] | [standalone]

#1312914

On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
> Hi Andy, Alan
> 
> Andy Shevchenko 於 2016/1/19 下午 09:21 寫道:
> >>Your device is multi-function. Create an MFD driver for it. Make the
> >>8250 driver bind to the MFD, and provide your own baud rate methods
> >>within the standard 8250 layer
> >
> >Ouch, somehow I missed this one!
> >
> >Peter, Alan's suggestion is really worth to try.
> >
> 
> Thanks for point this. It seems good to probe on MFD driver, them MFD
> register platform devices to invoke platform driver to initialize
> sub-parts. I'll try to survey first.
> 
> But I had a new question, If I really do it with MFD subsystem, it'll
> split into 3 parts, MFD probe(driver/mfd) / GPIO (driver/gpio) / UART
> (drivers/tty/serial/8250). It'll cross more than 2 subsystems and
> maintainers How should I do to organize the patches?
> 
> For examples, I should remove the probe function in 8250_pci.c and
> move it to new MFD file. It should organize it in the same patch as Paul
> said, but this patch will need 2 subsystem maintainer to do with the
> same patch, it seems weird.
> 
> Andy had cc "[PATCH v5] serial: 8250: add gpio support to exar" to me,
> could I use the same way to do GPIOLIB? First add a platform driver
> for F81504 gpio and add platform device into 8250_pci.c? It seems to
> be good and simple to implement.

+ Rob

Your hardware and my hardware both are almost same, so I guess the
discussion and the decision will apply to both of us.
And to have it as MFD, we can have a look at sm501.c it has serial and
gpio both.
But my personal opinion, if we move out the serial port related code
into a new driver (a new Kconfig symbol) userspace of many system will
break if this new symbol is not enabled by the distributions. But in the
way I have done the new symbol needs to be enabled only if the user
wants to use the GPIO capability. If that is not enabled GPIO cannot be
used but it will never break the serial port related code for them.
I think we should give a thought to that before splitting out the codes
from 8250_pci.

regards
sudip

[toc] | [prev] | [next] | [standalone]

#1312990

Hi Sudip,

Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
> On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:

> But my personal opinion, if we move out the serial port related code
> into a new driver (a new Kconfig symbol) userspace of many system will
> break if this new symbol is not enabled by the distributions. But in the
> way I have done the new symbol needs to be enabled only if the user
> wants to use the GPIO capability. If that is not enabled GPIO cannot be
> used but it will never break the serial port related code for them.
> I think we should give a thought to that before splitting out the codes
> from 8250_pci.

I agree with your opinion. I'm trying to implement GPIO with 2 ways,
One is like yours, add platform_device with in 8250_pci.c and implement
GPIOLIB platform driver with in 'driver/gpio", and the other is trying
split out from 8250_pci.c to MFD.

In my personal opinion, the first method is less impact with compatible
old system.
-- 
With Best Regards,
Peter Hung

[toc] | [prev] | [next] | [standalone]

#1314889

On Wed, Jan 20, 2016 at 04:24:36PM +0800, Peter Hung wrote:
> Hi Sudip,
> 
> Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
> >On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
> 
> >But my personal opinion, if we move out the serial port related code
> >into a new driver (a new Kconfig symbol) userspace of many system will
> >break if this new symbol is not enabled by the distributions. But in the
> >way I have done the new symbol needs to be enabled only if the user
> >wants to use the GPIO capability. If that is not enabled GPIO cannot be
> >used but it will never break the serial port related code for them.
> >I think we should give a thought to that before splitting out the codes
> >from 8250_pci.
> 
> I agree with your opinion. I'm trying to implement GPIO with 2 ways,
> One is like yours, add platform_device with in 8250_pci.c and implement
> GPIOLIB platform driver with in 'driver/gpio", and the other is trying
> split out from 8250_pci.c to MFD.
> 
> In my personal opinion, the first method is less impact with compatible
> old system.

Looks like no one else is in support of our opinion. Fair enough, I will
split out the related code from 8250_pci and create the MFD driver this
weekend for my hardware.

regards
sudip

[toc] | [prev] | [next] | [standalone]

#1314991

On Fri, Jan 22, 2016 at 12:53 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Wed, Jan 20, 2016 at 04:24:36PM +0800, Peter Hung wrote:
>> Hi Sudip,
>>
>> Sudip Mukherjee 於 2016/1/20 下午 02:22 寫道:
>> >On Wed, Jan 20, 2016 at 10:59:28AM +0800, Peter Hung wrote:
>>
>> >But my personal opinion, if we move out the serial port related code
>> >into a new driver (a new Kconfig symbol) userspace of many system will
>> >break if this new symbol is not enabled by the distributions. But in the
>> >way I have done the new symbol needs to be enabled only if the user
>> >wants to use the GPIO capability. If that is not enabled GPIO cannot be
>> >used but it will never break the serial port related code for them.
>> >I think we should give a thought to that before splitting out the codes
>> >from 8250_pci.
>>
>> I agree with your opinion. I'm trying to implement GPIO with 2 ways,
>> One is like yours, add platform_device with in 8250_pci.c and implement
>> GPIOLIB platform driver with in 'driver/gpio", and the other is trying
>> split out from 8250_pci.c to MFD.
>>
>> In my personal opinion, the first method is less impact with compatible
>> old system.
>
> Looks like no one else is in support of our opinion. Fair enough, I will
> split out the related code from 8250_pci and create the MFD driver this
> weekend for my hardware.

Yeah, MFD looks preferable.

Btw, don't forget to backlist your devices in 8250_pci since they
quite possible provide a PCI class which is used by 8250_pci driver
for default enumeration.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web