diff mbox series

[RFC,3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198

Message ID 20201004162908.3216898-4-martin.blumenstingl@googlemail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers | expand

Commit Message

Martin Blumenstingl Oct. 4, 2020, 4:29 p.m. UTC
EJ168/EJ188/EJ198 are USB xHCI controllers. They also contain four GPIO
lines which are used on some systems to toggle an LED based on whether a
USB device is connected.

There is no public datasheet available for this hardware. All
information in this driver is taken from the
"F9K1115v2.03.97-GPL-10.2.85-20140313" GPL code dump of the Belkin
F9K1115v2. This board comes with an EJ168 USB xHCI controller and the
USB 3.0 LED is connected to one of the GPIOs. Inside the GPL source
archive the related code can be found in:
  linux/kernels/mips-linux-2.6.31/drivers/usb/host/etxhci-pci.c

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/gpio/Kconfig      |   9 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-ej1x8.c | 311 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/gpio/gpio-ej1x8.c

Comments

Linus Walleij Oct. 7, 2020, 9:29 a.m. UTC | #1
Hi Martin,

thanks for your patch!

As noted on the earlier patches I think this should be folded into the
existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
gets messy, as a separate bolt-on, something like
xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
You can use a Kconfig symbol for the GPIO portions or not.

On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:

> EJ168/EJ188/EJ198 are USB xHCI controllers. They also contain four GPIO
> lines which are used on some systems to toggle an LED based on whether a
> USB device is connected.
>
> There is no public datasheet available for this hardware. All
> information in this driver is taken from the
> "F9K1115v2.03.97-GPL-10.2.85-20140313" GPL code dump of the Belkin
> F9K1115v2. This board comes with an EJ168 USB xHCI controller and the
> USB 3.0 LED is connected to one of the GPIOs. Inside the GPL source
> archive the related code can be found in:
>   linux/kernels/mips-linux-2.6.31/drivers/usb/host/etxhci-pci.c
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
(...)

> +config GPIO_EJ1X8
> +       tristate "Etron Tech Inc. EJ168/EJ188/EJ198 GPIO driver"
> +       depends on OF_GPIO && PCI

It is fine to just select GPIOLIB if you want this to always be
compiled-in (if the USB maintainers agree).

> +       help
> +         Selecting this option will enable the GPIO pins present on
> +         the Etron Tech Inc. EJ168/EJ188/EJ198 USB xHCI controllers.
> +
> +         If unsure, say N.

(...)
> +#define EJ1X8_GPIO_INIT                                        0x44
> +#define EJ1X8_GPIO_WRITE                               0x68
> +#define EJ1X8_GPIO_READ                                        0x6c
> +
> +#define EJ1X8_GPIO_CTRL                                        0x18005020
> +#define EJ1X8_GPIO_CTRL_READ_ALL_MASK                  GENMASK(7, 0)
> +#define EJ1X8_GPIO_CTRL_WRITE_ALL_MASK                 GENMASK(23, 16)
> +#define EJ1X8_GPIO_CTRL_OUT_LOW                                0x0
> +#define EJ1X8_GPIO_CTRL_OUT_HIGH                       0x1
> +#define EJ1X8_GPIO_CTRL_IN                             0x2
> +#define EJ1X8_GPIO_CTRL_MASK                           0x3
> +
> +#define EJ1X8_GPIO_MODE                                        0x18005022
> +#define EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK            GENMASK(23, 16)
> +#define EJ1X8_GPIO_MODE_DISABLE                                0x0
> +#define EJ1X8_GPIO_MODE_ENABLE                         0x1
> +#define EJ1X8_GPIO_MODE_MASK                           0x3

Nice that you got all of this out of reverse-engineering!

> +static LIST_HEAD(ej1x8_gpios);

This should not be necessary. Tie the GPIO state into the PCI device
driver state, possibly using some #ifdefs.

> +static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask)
> +{
> +       return (gpio * fls(mask));
> +}
> +
> +static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask)
> +{
> +       return mask << ej1x8_gpio_shift(gpio, mask);
> +}

This looks a bit like regmap but trying to use regmap for this
would probably be overengineering.

Looking at the code I get annoyed that it uses the config space to
manipulate the GPIOs, else you could have used GPIO_GENERIC
but now you can't, how typical.

Other than that the code looks nice, but fold it into the USB
host driver somehow unless there is a compelling argument
as to why not.

Yours,
Linus Walleij
Martin Blumenstingl Oct. 7, 2020, 7:44 p.m. UTC | #2
Hi Linus,

On Wed, Oct 7, 2020 at 11:29 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Martin,
>
> thanks for your patch!
thank you for reviewing the whole series!

> As noted on the earlier patches I think this should be folded into the
> existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
> gets messy, as a separate bolt-on, something like
> xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
> You can use a Kconfig symbol for the GPIO portions or not.
OK, I will do that if there are no objections from other developers
I am intending to place the relevant code in xhci-pci-etron.c, similar
to what we already have with xhci-pci-renesas.c

[...]
> This should not be necessary. Tie the GPIO state into the PCI device
> driver state, possibly using some #ifdefs.
>
> > +static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask)
> > +{
> > +       return (gpio * fls(mask));
> > +}
> > +
> > +static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask)
> > +{
> > +       return mask << ej1x8_gpio_shift(gpio, mask);
> > +}
>
> This looks a bit like regmap but trying to use regmap for this
> would probably be overengineering.
the problem is also the "INIT" register which needs to be set before
writing the registers

> Looking at the code I get annoyed that it uses the config space to
> manipulate the GPIOs, else you could have used GPIO_GENERIC
> but now you can't, how typical.
I think this won't work in practice because of the EJ1X8_GPIO_CTRL for
which we have to read from bits [7:0] but write to bits [23:16]
due to this (and the INIT register as mentioned above) I did not
consider GPIO_GENERIC any further

> Other than that the code looks nice, but fold it into the USB
> host driver somehow unless there is a compelling argument
> as to why not.
will do so, thanks!


Best regards,
Martin
Martin Blumenstingl Dec. 21, 2020, 3:28 p.m. UTC | #3
Hi Linus,

On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
[...]
> > As noted on the earlier patches I think this should be folded into the
> > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
> > gets messy, as a separate bolt-on, something like
> > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
> > You can use a Kconfig symbol for the GPIO portions or not.
> OK, I will do that if there are no objections from other developers
> I am intending to place the relevant code in xhci-pci-etron.c, similar
> to what we already have with xhci-pci-renesas.c
I tried this and unfortunately there's a catch.
the nice thing about having a separate GPIO driver means that the
xhci-pci driver doesn't need to know about it.

I implemented xhci-pci-etron.c and gave it a Kconfig option.
xhci-pci is then calling into xhci-pci-etron (through some
etron_xhci_pci_probe function).
unfortunately this means that xhci-pci now depends on xhci-pci-etron.
for xhci-pci-renesas this is fine (I think) because that part of the
code is needed to get the xHCI controller going
but for xhci-pci-etron this is a different story: the GPIO controller
is entirely optional and only used on few devices

my goal is (at some point in the future) to have the GPIO driver in OpenWrt.
I am not sure if they would accept a patch where xhci-pci would then
pull in the dependencies for that Etron controller, even though most
boards don't need it.

Please let me know if you have any idea on how to solve this.


Best regards,
Martin
Martin Blumenstingl Dec. 31, 2020, 12:15 a.m. UTC | #4
Hi Linus,

On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Linus,
>
> On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> [...]
> > > As noted on the earlier patches I think this should be folded into the
> > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
> > > gets messy, as a separate bolt-on, something like
> > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
> > > You can use a Kconfig symbol for the GPIO portions or not.
> > OK, I will do that if there are no objections from other developers
> > I am intending to place the relevant code in xhci-pci-etron.c, similar
> > to what we already have with xhci-pci-renesas.c
> I tried this and unfortunately there's a catch.
> the nice thing about having a separate GPIO driver means that the
> xhci-pci driver doesn't need to know about it.
>
> I implemented xhci-pci-etron.c and gave it a Kconfig option.
> xhci-pci is then calling into xhci-pci-etron (through some
> etron_xhci_pci_probe function).
> unfortunately this means that xhci-pci now depends on xhci-pci-etron.
> for xhci-pci-renesas this is fine (I think) because that part of the
> code is needed to get the xHCI controller going
> but for xhci-pci-etron this is a different story: the GPIO controller
> is entirely optional and only used on few devices
>
> my goal is (at some point in the future) to have the GPIO driver in OpenWrt.
> I am not sure if they would accept a patch where xhci-pci would then
> pull in the dependencies for that Etron controller, even though most
> boards don't need it.
>
> Please let me know if you have any idea on how to solve this.
next week I have some free time to work on this
I am still interested in any ideas that you have about this


Best regards,
Martin
Linus Walleij Jan. 5, 2021, 10:23 p.m. UTC | #5
On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> [...]
> > > As noted on the earlier patches I think this should be folded into the
> > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
> > > gets messy, as a separate bolt-on, something like
> > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
> > > You can use a Kconfig symbol for the GPIO portions or not.
> > OK, I will do that if there are no objections from other developers
> > I am intending to place the relevant code in xhci-pci-etron.c, similar
> > to what we already have with xhci-pci-renesas.c
>
> I tried this and unfortunately there's a catch.
> the nice thing about having a separate GPIO driver means that the
> xhci-pci driver doesn't need to know about it.

Since PCI devices have device-wide power management and things
like that I think that is a really dangerous idea.

What if the GPIO driver starts poking around in this PCI device
when the main driver is also probed and has put the device
into sleep state?

This type of set-up needs to be discussed with
the PCI maintainer to make sure it is safe.

> I implemented xhci-pci-etron.c and gave it a Kconfig option.
> xhci-pci is then calling into xhci-pci-etron (through some
> etron_xhci_pci_probe function).

This sounds about right.

> unfortunately this means that xhci-pci now depends on xhci-pci-etron.
> for xhci-pci-renesas this is fine (I think) because that part of the
> code is needed to get the xHCI controller going
> but for xhci-pci-etron this is a different story: the GPIO controller
> is entirely optional and only used on few devices

I might be naive but should it not be the other way around?
That xhci-pci-etron is dependent on xhci-pci? I imagine
it would be an optional add-on.

> my goal is (at some point in the future) to have the GPIO driver in OpenWrt.
> I am not sure if they would accept a patch where xhci-pci would then
> pull in the dependencies for that Etron controller, even though most
> boards don't need it.

Make sure the etron part is an additional module that can be
loaded after xhci-pci.

OpenWrt support optional modules to be compiled per-system.

Yours,
Linus Walleij
Martin Blumenstingl Jan. 6, 2021, 3:17 p.m. UTC | #6
Hi Linus,

On Tue, Jan 5, 2021 at 11:23 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> > On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com> wrote:
> > [...]
> > > > As noted on the earlier patches I think this should be folded into the
> > > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
> > > > gets messy, as a separate bolt-on, something like
> > > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
> > > > You can use a Kconfig symbol for the GPIO portions or not.
> > > OK, I will do that if there are no objections from other developers
> > > I am intending to place the relevant code in xhci-pci-etron.c, similar
> > > to what we already have with xhci-pci-renesas.c
> >
> > I tried this and unfortunately there's a catch.
> > the nice thing about having a separate GPIO driver means that the
> > xhci-pci driver doesn't need to know about it.
>
> Since PCI devices have device-wide power management and things
> like that I think that is a really dangerous idea.
>
> What if the GPIO driver starts poking around in this PCI device
> when the main driver is also probed and has put the device
> into sleep state?
that is asking for trouble, indeed.

[...]
> > I implemented xhci-pci-etron.c and gave it a Kconfig option.
> > xhci-pci is then calling into xhci-pci-etron (through some
> > etron_xhci_pci_probe function).
>
> This sounds about right.
>
> > unfortunately this means that xhci-pci now depends on xhci-pci-etron.
> > for xhci-pci-renesas this is fine (I think) because that part of the
> > code is needed to get the xHCI controller going
> > but for xhci-pci-etron this is a different story: the GPIO controller
> > is entirely optional and only used on few devices
>
> I might be naive but should it not be the other way around?
> That xhci-pci-etron is dependent on xhci-pci? I imagine
> it would be an optional add-on.
the only way to achieve this that I can think of is to basically have
xhci-pci-etron implement it's own pci_driver and then call
xhci_pci_probe, xhci_pci_remove, etc.
but then it depends on the driver load order if the GPIO controller is exposed

what structure did you have in mind to achieve this?

> > my goal is (at some point in the future) to have the GPIO driver in OpenWrt.
> > I am not sure if they would accept a patch where xhci-pci would then
> > pull in the dependencies for that Etron controller, even though most
> > boards don't need it.
>
> Make sure the etron part is an additional module that can be
> loaded after xhci-pci.
my approach from above unfortunately would not achieve this
so if you have an idea how to achieve this (or have any other driver
in mind that I can use as reference, even if not related to
GPIO/USB/PCI then please let me know)

> OpenWrt support optional modules to be compiled per-system.
that I already found out. That's why I think that I need to get the
driver part "right" and then get the OpenWrt part done in just a few
lines of their build-system


Best regards,
Martin
Linus Walleij Jan. 7, 2021, 10:36 a.m. UTC | #7
On Wed, Jan 6, 2021 at 4:17 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:

> > > unfortunately this means that xhci-pci now depends on xhci-pci-etron.
> > > for xhci-pci-renesas this is fine (I think) because that part of the
> > > code is needed to get the xHCI controller going
> > > but for xhci-pci-etron this is a different story: the GPIO controller
> > > is entirely optional and only used on few devices
> >
> > I might be naive but should it not be the other way around?
> > That xhci-pci-etron is dependent on xhci-pci? I imagine
> > it would be an optional add-on.
>
> the only way to achieve this that I can think of is to basically have
> xhci-pci-etron implement it's own pci_driver and then call
> xhci_pci_probe, xhci_pci_remove, etc.
> but then it depends on the driver load order if the GPIO controller is exposed
>
> what structure did you have in mind to achieve this?

Something that is compiled and called conditionally with
stubs in the local .h file.


Kconfig:

config FOO
   tristate "Main matter"

config FOO_ADD_ON
    tristate "Optional on"
    depends on FOO


Makefile:

obj-$(CONFIG_FOO) += foo.o
obj-$(CONFIG_FOO_ADD_ON) += foo-add-on.o


foo.h:

struct foo {
...
};

#if IS_ENABLED(CONFIG_FOO_ADD_ON)
int foo_add_on_init(struct foo *);
#else /* No CONFIG_FOO_ADD_ON */
static int foo_add_on_init(struct foo *)
{
    return 0;
}
#endif


foo.c:

#include "foo.h"

ret = foo_add_on_init(foo);
(...)


foo-add-on.c:

int foo_add_on_init(struct foo *)
{
(...)
}
EXPORT_SYMBOL_GPL(foo_add_on_init);

> > Make sure the etron part is an additional module that can be
> > loaded after xhci-pci.
>
> my approach from above unfortunately would not achieve this
> so if you have an idea how to achieve this (or have any other driver
> in mind that I can use as reference, even if not related to
> GPIO/USB/PCI then please let me know)

See per above. I don't see any problem with this, it will be an additional
module that does not feature a probe() call and device driver bind.

I think it is also possible to link both files into the same object if
the optional add on is enabled, so it is part of the main module
when modprobing. The foo.h stubs are still needed, then the
binary will just be smaller if the add-on is not enabled. There are
solutions like this in the kernel, I just don't remember one right
now so grep around.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8030fd91a3cc..88820b04ffa5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -215,6 +215,15 @@  config GPIO_EIC_SPRD
 	help
 	  Say yes here to support Spreadtrum EIC device.
 
+config GPIO_EJ1X8
+	tristate "Etron Tech Inc. EJ168/EJ188/EJ198 GPIO driver"
+	depends on OF_GPIO && PCI
+	help
+	  Selecting this option will enable the GPIO pins present on
+	  the Etron Tech Inc. EJ168/EJ188/EJ198 USB xHCI controllers.
+
+	  If unsure, say N.
+
 config GPIO_EM
 	tristate "Emma Mobile GPIO"
 	depends on (ARCH_EMEV2 || COMPILE_TEST) && OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4f9abff4f2dc..6d5e345b1f2d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -55,6 +55,7 @@  obj-$(CONFIG_GPIO_DAVINCI)		+= gpio-davinci.o
 obj-$(CONFIG_GPIO_DLN2)			+= gpio-dln2.o
 obj-$(CONFIG_GPIO_DWAPB)		+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EIC_SPRD)		+= gpio-eic-sprd.o
+obj-$(CONFIG_GPIO_EJ1X8)		+= gpio-ej1x8.o
 obj-$(CONFIG_GPIO_EM)			+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)		+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_EXAR)			+= gpio-exar.o
diff --git a/drivers/gpio/gpio-ej1x8.c b/drivers/gpio/gpio-ej1x8.c
new file mode 100644
index 000000000000..c673e62c34f8
--- /dev/null
+++ b/drivers/gpio/gpio-ej1x8.c
@@ -0,0 +1,311 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2020 Martin Blumenstingl <martin.blumenstingl@googlemail.com> */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+#include <linux/property.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define EJ1X8_GPIO_INIT					0x44
+#define EJ1X8_GPIO_WRITE				0x68
+#define EJ1X8_GPIO_READ					0x6c
+
+#define EJ1X8_GPIO_CTRL					0x18005020
+#define EJ1X8_GPIO_CTRL_READ_ALL_MASK			GENMASK(7, 0)
+#define EJ1X8_GPIO_CTRL_WRITE_ALL_MASK			GENMASK(23, 16)
+#define EJ1X8_GPIO_CTRL_OUT_LOW				0x0
+#define EJ1X8_GPIO_CTRL_OUT_HIGH			0x1
+#define EJ1X8_GPIO_CTRL_IN				0x2
+#define EJ1X8_GPIO_CTRL_MASK				0x3
+
+#define EJ1X8_GPIO_MODE					0x18005022
+#define EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK		GENMASK(23, 16)
+#define EJ1X8_GPIO_MODE_DISABLE				0x0
+#define EJ1X8_GPIO_MODE_ENABLE				0x1
+#define EJ1X8_GPIO_MODE_MASK				0x3
+
+static LIST_HEAD(ej1x8_gpios);
+
+struct ej1x8_gpio {
+	spinlock_t			lock;
+	struct pci_dev			*pci_dev;
+	struct gpio_chip		chip;
+	struct list_head		list_head;
+};
+
+static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask)
+{
+	return (gpio * fls(mask));
+}
+
+static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask)
+{
+	return mask << ej1x8_gpio_shift(gpio, mask);
+}
+
+static int ej1x8_gpio_read(struct gpio_chip *gc, u32 reg, u32 *value)
+{
+	struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc);
+	int err;
+
+	err = pci_write_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_WRITE, reg);
+	if (err) {
+		dev_err(gc->parent, "Failed to select 0x%08x register\n", reg);
+		return err;
+	}
+
+	usleep_range(1000, 10000);
+
+	err = pci_read_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_READ, value);
+	if (err) {
+		dev_err(gc->parent, "Failed to read 0x%08x register\n", reg);
+		return err;
+	}
+
+	return 0;
+}
+
+static int ej1x8_gpio_write(struct gpio_chip *gc, u32 reg, u32 value)
+{
+	struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc);
+	int err;
+
+	err = pci_write_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_WRITE,
+				     reg | value | BIT(24));
+	if (err) {
+		dev_err(gc->parent, "Failed to write 0x%08x register\n", reg);
+		return err;
+	}
+
+	usleep_range(1000, 10000);
+
+	return 0;
+}
+
+static int ej1x8_gpio_config(struct gpio_chip *gc, unsigned int gpio, u8 mode,
+			     u8 ctrl)
+{
+	struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc);
+	u8 all_gpio_ctrl, all_gpio_mode;
+	u32 temp;
+	int err;
+
+	spin_lock(&ej1x8->lock);
+
+	err = pci_read_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_INIT, &temp);
+	if (err) {
+		dev_err(gc->parent, "Failed to read INIT register\n");
+		return err;
+	}
+
+	err = pci_write_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_INIT,
+				     temp | 0x1);
+	if (err) {
+		dev_err(gc->parent, "Failed to write INIT register\n");
+		return err;
+	}
+
+	err = ej1x8_gpio_read(gc, EJ1X8_GPIO_CTRL, &temp);
+	if (err)
+		goto err_unlock;
+
+	all_gpio_ctrl = FIELD_GET(EJ1X8_GPIO_CTRL_READ_ALL_MASK, temp);
+	all_gpio_ctrl &= ~ej1x8_gpio_mask(gpio, EJ1X8_GPIO_CTRL_MASK);
+	all_gpio_ctrl |= ctrl << ej1x8_gpio_shift(gpio, EJ1X8_GPIO_CTRL_MASK);
+
+	err = ej1x8_gpio_read(gc, EJ1X8_GPIO_MODE, &temp);
+	if (err)
+		goto err_unlock;
+
+	all_gpio_mode = FIELD_GET(EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK, temp);
+	all_gpio_mode &= ~ej1x8_gpio_mask(gpio, EJ1X8_GPIO_MODE_MASK);
+	all_gpio_mode |= mode << ej1x8_gpio_shift(gpio, EJ1X8_GPIO_MODE_MASK);
+
+	err = ej1x8_gpio_write(gc, EJ1X8_GPIO_CTRL,
+			       FIELD_PREP(EJ1X8_GPIO_CTRL_WRITE_ALL_MASK,
+					  all_gpio_ctrl));
+	if (err)
+		goto err_unlock;
+
+	err = ej1x8_gpio_write(gc, EJ1X8_GPIO_MODE,
+			       FIELD_PREP(EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK,
+					  all_gpio_mode));
+	if (err)
+		goto err_unlock;
+
+	spin_unlock(&ej1x8->lock);
+
+	return 0;
+
+err_unlock:
+	spin_unlock(&ej1x8->lock);
+	return err;
+}
+
+static int ej1x8_gpio_get_mode(struct gpio_chip *gc, unsigned int gpio, u8 *mode)
+{
+	struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc);
+	u32 temp, all_gpio_mode;
+	int err;
+
+	spin_lock(&ej1x8->lock);
+	err = ej1x8_gpio_read(gc, EJ1X8_GPIO_MODE, &temp);
+	spin_unlock(&ej1x8->lock);
+
+	if (err)
+		return err;
+
+	all_gpio_mode = FIELD_GET(EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK, temp);
+	*mode = all_gpio_mode >> ej1x8_gpio_shift(gpio, EJ1X8_GPIO_MODE_MASK);
+	*mode &= EJ1X8_GPIO_MODE_MASK;
+
+	return 0;
+}
+
+static void ej1x8_gpio_free(struct gpio_chip *gc, unsigned int gpio)
+{
+	ej1x8_gpio_config(gc, gpio, EJ1X8_GPIO_MODE_DISABLE, EJ1X8_GPIO_CTRL_IN);
+}
+
+static int ej1x8_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+	u8 mode;
+	int err;
+
+	err = ej1x8_gpio_get_mode(gc, gpio, &mode);
+	if (err)
+		return err;
+
+	switch (mode) {
+	case EJ1X8_GPIO_CTRL_IN:
+		return GPIO_LINE_DIRECTION_IN;
+
+	case EJ1X8_GPIO_CTRL_OUT_HIGH:
+	case EJ1X8_GPIO_CTRL_OUT_LOW:
+		return GPIO_LINE_DIRECTION_OUT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ej1x8_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio,
+				       int value)
+{
+	u8 gpio_ctrl;
+
+	if (value)
+		gpio_ctrl = EJ1X8_GPIO_CTRL_OUT_HIGH;
+	else
+		gpio_ctrl = EJ1X8_GPIO_CTRL_OUT_LOW;
+
+	return ej1x8_gpio_config(gc, gpio, EJ1X8_GPIO_MODE_ENABLE, gpio_ctrl);
+}
+
+static int ej1x8_gpio_get_value(struct gpio_chip *gc, unsigned int gpio)
+{
+	u8 mode;
+	int err;
+
+	err = ej1x8_gpio_get_mode(gc, gpio, &mode);
+	if (err)
+		return err;
+
+	switch (mode) {
+	case EJ1X8_GPIO_CTRL_OUT_HIGH:
+		return 1;
+
+	case EJ1X8_GPIO_CTRL_OUT_LOW:
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static void ej1x8_gpio_set_value(struct gpio_chip *gc, unsigned int gpio, int value)
+{
+	ej1x8_gpio_direction_output(gc, gpio, value);
+}
+
+static const struct pci_device_id ej1x8_gpio_pci_ids[] __initconst = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_ETRON, PCI_DEVICE_ID_ETRON_EJ168) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_ETRON, PCI_DEVICE_ID_ETRON_EJ188) },
+	{ /* sentinel */ }
+};
+
+static int __init ej1x8_gpio_init(void)
+{
+	struct pci_dev *pci_dev = NULL;
+	struct ej1x8_gpio *ej1x8;
+	int err;
+
+	for_each_pci_dev(pci_dev) {
+		if (!pci_match_id(ej1x8_gpio_pci_ids, pci_dev))
+			continue;
+
+		if (!device_property_read_bool(&pci_dev->dev, "gpio-controller"))
+			continue;
+
+		ej1x8 = kzalloc(sizeof(*ej1x8), GFP_KERNEL);
+		if (!ej1x8)
+			continue;
+
+		spin_lock_init(&ej1x8->lock);
+		ej1x8->pci_dev = pci_dev_get(pci_dev);
+
+		/* TODO: input mode is supported by the hardware but not the driver */
+		ej1x8->chip.label = dev_name(&pci_dev->dev);
+		ej1x8->chip.owner = THIS_MODULE;
+		ej1x8->chip.parent = &pci_dev->dev;
+		ej1x8->chip.of_node = pci_dev->dev.of_node;
+		ej1x8->chip.free = ej1x8_gpio_free;
+		ej1x8->chip.get_direction = ej1x8_gpio_get_direction;
+		ej1x8->chip.direction_output = ej1x8_gpio_direction_output;
+		ej1x8->chip.get = ej1x8_gpio_get_value;
+		ej1x8->chip.set = ej1x8_gpio_set_value;
+		ej1x8->chip.base = -1;
+		ej1x8->chip.ngpio = 4;
+
+		err = gpiochip_add_data(&ej1x8->chip, ej1x8);
+		if (err) {
+			dev_warn(&pci_dev->dev,
+				 "Failed to register GPIO device: %d\n", err);
+			pci_dev_put(ej1x8->pci_dev);
+			kfree(ej1x8);
+			continue;
+		}
+
+		list_add(&ej1x8->list_head, &ej1x8_gpios);
+	}
+
+	return 0;
+}
+
+static void __exit ej1x8_gpio_exit(void)
+{
+	struct ej1x8_gpio *ej1x8, *tmp;
+
+	list_for_each_entry_safe(ej1x8, tmp, &ej1x8_gpios, list_head) {
+		gpiochip_remove(&ej1x8->chip);
+		pci_dev_put(ej1x8->pci_dev);
+		list_del(&ej1x8->list_head);
+		kfree(ej1x8);
+	}
+}
+
+module_init(ej1x8_gpio_init);
+module_exit(ej1x8_gpio_exit);
+
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_DESCRIPTION("Etron Technology Inc. EJ168/EJ188/EJ198 GPIO driver");
+MODULE_LICENSE("GPL v2");