Re: [PATCH v3] serial: 8250: add gpio support to exar

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

On Mon, Dec 21, 2015 at 08:28:51PM +0200, Andy Shevchenko wrote:
> On Mon, Dec 21, 2015 at 5:19 PM, Sudip Mukherjee
> <sudipm.mukherjee@gmail.com> wrote:
> 
> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > index 6412f14..ec3d287 100644
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -378,3 +378,11 @@ config SERIAL_8250_MID
> >           Selecting this option will enable handling of the extra features
> >           present on the UART found on Intel Medfield SOC and various other
> >           Intel platforms.
> > +
> > +config SERIAL_8250_EXAR_GPIO
> > +       bool "Support for GPIO pins on XR17V352/354/358"
> > +       depends on SERIAL_8250_PCI && GPIOLIB
> > +       select GPIO_SYSFS
> > +       help
> > +         Selecting this option will enable handling of GPIO pins present
> > +         on Exar XR17V352/354/358 chips.
> > diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> > index e177f86..1d0fce0 100644
> > --- a/drivers/tty/serial/8250/Makefile
> > +++ b/drivers/tty/serial/8250/Makefile
> > @@ -28,5 +28,8 @@ obj-$(CONFIG_SERIAL_8250_MT6577)      += 8250_mtk.o
> >  obj-$(CONFIG_SERIAL_8250_UNIPHIER)     += 8250_uniphier.o
> >  obj-$(CONFIG_SERIAL_8250_INGENIC)      += 8250_ingenic.o
> >  obj-$(CONFIG_SERIAL_8250_MID)          += 8250_mid.o
> > +obj-$(CONFIG_SERIAL_8250_EXAR_GPIO)    += exar_gpio.o
> > +exar_gpio-y                            := 8250_pci.o 8250_gpio.o
> > +
> >
> >  CFLAGS_8250_ingenic.o += -I$(srctree)/scripts/dtc/libfdt
> >
> >
> > But I am getting:
> > ERROR: "xr17v35x_gpio_exit" [drivers/tty/serial/8250/8250_pci.ko] undefined!
> > ERROR: "xr17v35x_gpio_init" [drivers/tty/serial/8250/8250_pci.ko] undefined!
> >
> > I will need little hint for the Makefile, please. Its clear that i have
> > messed up.
> 
> Yes, read my previous mail.
> 
> There are at least two approaches:
>  - use 8250_pci, etc as a library (see example: 8250_mid.c)
>  - force 8250_pci to use external libraries in some cases (seems your approach)
> 
> In the latter you have to extend 8250_pci and rename it. I have no
> idea if Greg or anyone else gives a green light for that.

I am sure Greg will give a big red light for this. :)

> 
> Something like this (forget syntax of ifs in Makefile though
> obj-(…8250_PCI) += 8250_pci.o
> obj-8250_pci = 8250_pci_orig.o
> ifneq (…EXAR_GPIO,)
> obj-8250_pci += exar_gpio.o
> endif
> 
> But what I can recommend is to split exar parts out of 8250_pci first
> to a separate driver, do your part as a part of that new driver.

Ok, same approach what 8250_mid.c has done. But in the initial design
discussions Greg was against the idea of having it as a separate driver.

> Of course better to gather maintainer's opinion first.

Greg, can you please give some idea here about the best way to approach...
I personally think, having it as a module with the minor changes that
Alan and Andy has suggested is the best approach. The only downside is
that the module gets loaded even if the device is not there. But for
arguments sake, the module will be enabled in the config only if someone
is interested to build it. Everyone has the option to disable it if
required.

regards
sudip