[PATCH 1/5] i8042: intel-8042 DT documentation

[PATCH 1/5] i8042: intel-8042 DT documentation

[Roman Volkov]

Documentation for 'intel,8042' DT compatible node.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 .../devicetree/bindings/input/intel-8042.txt       | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt

diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt b/Documentation/devicetree/bindings/input/intel-8042.txt
new file mode 100644
index 0000000..2aea7ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/intel-8042.txt
@@ -0,0 +1,29 @@
+* Intel 8042 Keyboard Controller
+
+Required properties:
+- compatible: should be "intel,8042"
+- regs: memory for keyboard controller
+- interrupts: two interrupts should be specified (keyboard and aux)
+- command-reg: offset in memory for command register
+- status-reg: offset in memory for status register
+- data-reg: offset in memory for data register
+
+Optional properties:
+- init-reset: Controller should be reset on init and cleanup
+
+Optional Linux-specific properties:
+- linux,kbd_phys_desc: defaults to i8042/serio0
+- linux,aux_phys_desc: defaults to i8042/serio1
+- linux,mux_phys_desc: defaults to i8042/serio%d
+
+
+Example:
+	keyboard@d8008800 {
+		compatible = "intel,8042";
+		reg = <0xd8008800 0x100>;
+		interrupts = <23 4>;
+		command-reg = <0x04>;
+		status-reg = <0x04>;
+		data-reg = <0x00>;
+		mux-ports = <2>;
+	};
-- 
2.2.2

[PATCH 2/5] i8042: Kernel configuration handling for DT support

[Roman Volkov]

i8042_dt.h should be included when CONFIG_ARCH_MIGHT_HAVE_PC_SERIO and
CONFIG_USE_OF are selected. It should be not necessary to create
additional options in the kernel config.

Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..d2c1761 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -28,6 +28,9 @@
 #include "i8042-x86ia64io.h"
 #elif defined(CONFIG_UNICORE32)
 #include "i8042-unicore32io.h"
+#elif defined(CONFIG_ARCH_MIGHT_HAVE_PC_SERIO) && defined(CONFIG_USE_OF)
+#define SERIO_I8042_DT
+#include "i8042-dt.h"
 #else
 #include "i8042-io.h"
 #endif
-- 
2.2.2

[PATCH 3/5] i8042: Add OF match table

[Roman Volkov]

The OF device table allows the platform_driver_probe() function to
automatically match device and parse the DT node.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 924e4bf..c53323e 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1460,12 +1460,22 @@ static int i8042_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef SERIO_I8042_DT
+static struct of_device_id i8042_dt_ids[] = {
+	{ .compatible = "intel,8042" },
+	{ /* Sentinel */ },
+};
+#endif
+
 static struct platform_driver i8042_driver = {
 	.driver		= {
 		.name	= "i8042",
 #ifdef CONFIG_PM
 		.pm	= &i8042_pm_ops,
 #endif
+#ifdef SERIO_I8042_DT
+		.of_match_table = i8042_dt_ids,
+#endif
 	},
 	.remove		= i8042_remove,
 	.shutdown	= i8042_shutdown,
-- 
2.2.2

[PATCH 4/5] i8042: Prepare i8042 driver for DT support

[Roman Volkov]

Use platform_device_probe() instead of platform_create_bundle() when
compiled with DT support, since the latter function is not suitable for
handling the OF device tree.

The order of initialization is changed, since i8042_platform_init() for DT
requires initialized platform_device structure. To avoid searching of the
compatible node twice, the platform_device structure pointer must be passed
to the i8042_platform_init() function right after initialization by
platform_device_probe().

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---

Yes, many of these ifdefs look ugly. Suggestions on how to avoid this are
welcome (except using of_find_compatible_node() for searching the node twice
before calling the probe function).

The problem is that platform_device_add() causes parent drivers to be probed
while the i8042 is not ready yet (register addresses point to the sky).

I would suggest to do something like deferred probing of the parent drivers,
but this would be a part of another investigation and patchset.

Regards,
Roman.

 drivers/input/serio/i8042.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index c53323e..2978b0e 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1407,16 +1407,32 @@ static int __init i8042_probe(struct platform_device *dev)
 	int error;
 
 	i8042_platform_device = dev;
+#ifdef SERIO_I8042_DT
+	error = i8042_platform_init(dev);
+	if (error)
+		return error;
 
+	error = i8042_controller_check();
+	if (error)
+		goto out_platform_exit;
+#endif
 	if (i8042_reset) {
 		error = i8042_controller_selftest();
 		if (error)
+#ifdef SERIO_I8042_DT
+			goto out_platform_exit;
+#else
 			return error;
+#endif
 	}
 
 	error = i8042_controller_init();
 	if (error)
+#ifdef SERIO_I8042_DT
+		goto out_platform_exit;
+#else
 		return error;
+#endif
 
 #ifdef CONFIG_X86
 	if (i8042_dritek)
@@ -1446,7 +1462,10 @@ static int __init i8042_probe(struct platform_device *dev)
 	i8042_free_irqs();
 	i8042_controller_reset(false);
 	i8042_platform_device = NULL;
-
+#ifdef SERIO_I8042_DT
+ out_platform_exit:
+	i8042_platform_exit();
+#endif
 	return error;
 }
 
@@ -1483,11 +1502,18 @@ static struct platform_driver i8042_driver = {
 
 static int __init i8042_init(void)
 {
+#ifndef SERIO_I8042_DT
 	struct platform_device *pdev;
+#endif
 	int err;
 
 	dbg_init();
 
+#ifdef SERIO_I8042_DT
+	err = platform_driver_probe(&i8042_driver, i8042_probe);
+	if (err)
+		return err;
+#else
 	err = i8042_platform_init();
 	if (err)
 		return err;
@@ -1501,14 +1527,15 @@ static int __init i8042_init(void)
 		err = PTR_ERR(pdev);
 		goto err_platform_exit;
 	}
-
+#endif
 	panic_blink = i8042_panic_blink;
 
 	return 0;
-
+#ifndef SERIO_I8042_DT
  err_platform_exit:
 	i8042_platform_exit();
 	return err;
+#endif
 }
 
 static void __exit i8042_exit(void)
-- 
2.2.2

[PATCH 5/5] i8042: Add i8042_dt.h glue for DT support

[Roman Volkov]

This header file designed to be similar to other glue layers found
for i8042. The difference is that interrupt numbers, device address,
and other information should be retrieved from the device tree.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042-dt.h | 112 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100644 drivers/input/serio/i8042-dt.h

diff --git a/drivers/input/serio/i8042-dt.h b/drivers/input/serio/i8042-dt.h
new file mode 100644
index 0000000..0d1a344
--- /dev/null
+++ b/drivers/input/serio/i8042-dt.h
@@ -0,0 +1,112 @@
+#ifndef _I8042_DT_H
+#define _I8042_DT_H
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+static void __iomem *i8042_base;
+static unsigned int i8042_command_reg;
+static unsigned int i8042_status_reg;
+static unsigned int i8042_data_reg;
+#define I8042_COMMAND_REG i8042_command_reg
+#define I8042_STATUS_REG i8042_status_reg
+#define I8042_DATA_REG i8042_data_reg
+
+/*
+ * Names.
+ */
+
+static const char *i8042_kbd_phys_desc;
+static const char *i8042_aux_phys_desc;
+static const char *i8042_mux_phys_desc;
+#define I8042_KBD_PHYS_DESC i8042_kbd_phys_desc
+#define I8042_AUX_PHYS_DESC i8042_aux_phys_desc
+#define I8042_MUX_PHYS_DESC i8042_mux_phys_desc
+
+/*
+ * IRQs.
+ */
+static int i8042_kbd_irq;
+static int i8042_aux_irq;
+#define I8042_KBD_IRQ i8042_kbd_irq
+#define I8042_AUX_IRQ i8042_aux_irq
+
+static inline int i8042_read_data(void)
+{
+	return readb(i8042_base + i8042_data_reg);
+}
+
+static inline int i8042_read_status(void)
+{
+	return readb(i8042_base + i8042_status_reg);
+}
+
+static inline void i8042_write_data(int val)
+{
+	writeb(val, i8042_base + i8042_data_reg);
+}
+
+static inline void i8042_write_command(int val)
+{
+	writeb(val, i8042_base + i8042_command_reg);
+}
+
+static inline int i8042_platform_init(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int status;
+
+	i8042_base = of_iomap(np, 0);
+	if (!i8042_base)
+		return -ENOMEM;
+
+	status = of_property_read_u32(np, "command-reg", &i8042_command_reg);
+	if (status)
+		return status;
+
+	status = of_property_read_u32(np, "status-reg", &i8042_status_reg);
+	if (status)
+		return status;
+
+	status = of_property_read_u32(np, "data-reg", &i8042_data_reg);
+	if (status)
+		return status;
+
+	i8042_kbd_irq = irq_of_parse_and_map(np, 0);
+	i8042_aux_irq = irq_of_parse_and_map(np, 1);
+
+	status = of_property_read_string(np, "linux,kbd_phys_desc",
+						&i8042_kbd_phys_desc);
+	if (status)
+		i8042_kbd_phys_desc = "i8042/serio0";
+
+	status = of_property_read_string(np, "linux,aux_phys_desc",
+						&i8042_aux_phys_desc);
+	if (status)
+		i8042_aux_phys_desc = "i8042/serio1";
+
+	status = of_property_read_string(np, "linux,mux_phys_desc",
+						&i8042_mux_phys_desc);
+	if (status)
+		i8042_mux_phys_desc = "i8042/serio%d";
+
+	if (of_get_property(np, "init-reset", NULL))
+		i8042_reset = true;
+
+	return 0;
+}
+
+static inline void i8042_platform_exit(void)
+{
+	if (i8042_base)
+		iounmap(i8042_base);
+}
+
+#endif
-- 
2.2.2

Re: [PATCH 1/5] i8042: intel-8042 DT documentation

[Roman Volkov]

В Tue,  3 Feb 2015 00:48:46 +0300
Roman Volkov <v1ron@v1ros.org> пишет:

> Documentation for 'intel,8042' DT compatible node.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> ---
>  .../devicetree/bindings/input/intel-8042.txt       | 29
> ++++++++++++++++++++++ 1 file changed, 29 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/input/intel-8042.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt
> b/Documentation/devicetree/bindings/input/intel-8042.txt new file
> mode 100644 index 0000000..2aea7ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/intel-8042.txt
> @@ -0,0 +1,29 @@
> +* Intel 8042 Keyboard Controller
> +
> +Required properties:
> +- compatible: should be "intel,8042"
> +- regs: memory for keyboard controller
> +- interrupts: two interrupts should be specified (keyboard and aux)
> +- command-reg: offset in memory for command register
> +- status-reg: offset in memory for status register
> +- data-reg: offset in memory for data register
> +
> +Optional properties:
> +- init-reset: Controller should be reset on init and cleanup
> +
> +Optional Linux-specific properties:
> +- linux,kbd_phys_desc: defaults to i8042/serio0
> +- linux,aux_phys_desc: defaults to i8042/serio1
> +- linux,mux_phys_desc: defaults to i8042/serio%d
> +
> +
> +Example:
> +	keyboard@d8008800 {
> +		compatible = "intel,8042";
> +		reg = <0xd8008800 0x100>;
> +		interrupts = <23 4>;
> +		command-reg = <0x04>;
> +		status-reg = <0x04>;
> +		data-reg = <0x00>;
> +		mux-ports = <2>;
> +	};

Hi,

This patch set is to enable the Open Firmware Device Tree support for
the i8042 controller. Yes, some ARM SoC boards are still using i8042. As
an example, it is the vt8500 architecture.

I've tested this on my wm8505 machine in both configurations: as a
module and built-in. Also a modification of this driver is available
at https://github.com/linux-wmt/linux-vtwm. This should not affect x86
and similar architectures without the DT enabled in the config.

Regards,
Roman.

Re: [PATCH 1/5] i8042: intel-8042 DT documentation

[Mark Rutland]

On Mon, Feb 02, 2015 at 09:48:46PM +0000, Roman Volkov wrote:
> Documentation for 'intel,8042' DT compatible node.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> ---
>  .../devicetree/bindings/input/intel-8042.txt       | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt b/Documentation/devicetree/bindings/input/intel-8042.txt
> new file mode 100644
> index 0000000..2aea7ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/intel-8042.txt
> @@ -0,0 +1,29 @@
> +* Intel 8042 Keyboard Controller
> +
> +Required properties:
> +- compatible: should be "intel,8042"
> +- regs: memory for keyboard controller
> +- interrupts: two interrupts should be specified (keyboard and aux)

Is it possible only one of these is wired up?

It might be worth using interrupt-names.

> +- command-reg: offset in memory for command register
> +- status-reg: offset in memory for status register
> +- data-reg: offset in memory for data register
> +
> +Optional properties:
> +- init-reset: Controller should be reset on init and cleanup

Why is this necessary? Can't we just always reset it?

> +
> +Optional Linux-specific properties:
> +- linux,kbd_phys_desc: defaults to i8042/serio0
> +- linux,aux_phys_desc: defaults to i8042/serio1
> +- linux,mux_phys_desc: defaults to i8042/serio%d

As a general note, s/_/-/ in property names please.

That said, I don't follow why we should have these at all. I don't
understand what the description is intended to mean.

In general we want to avoid Linux-specific properties. If a DTB needs to
know about the inernals of an OS it's likely to be fragile and broken
over time.

> +
> +
> +Example:
> +	keyboard@d8008800 {
> +		compatible = "intel,8042";
> +		reg = <0xd8008800 0x100>;
> +		interrupts = <23 4>;

If this is intended to be two interrupts, please bracket them
individually, e.g.

	interrupts = <23>, <4>;

> +		command-reg = <0x04>;
> +		status-reg = <0x04>;

Same address?

> +		data-reg = <0x00>;
> +		mux-ports = <2>;

This wasn't documented above.

Thanks,
Mark.

> +	};
> -- 
> 2.2.2
> 
> 

Re: [PATCH 5/5] i8042: Add i8042_dt.h glue for DT support

[Mark Rutland]

On Mon, Feb 02, 2015 at 09:48:50PM +0000, Roman Volkov wrote:
> This header file designed to be similar to other glue layers found
> for i8042. The difference is that interrupt numbers, device address,
> and other information should be retrieved from the device tree.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> ---
>  drivers/input/serio/i8042-dt.h | 112 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100644 drivers/input/serio/i8042-dt.h
> 
> diff --git a/drivers/input/serio/i8042-dt.h b/drivers/input/serio/i8042-dt.h
> new file mode 100644
> index 0000000..0d1a344
> --- /dev/null
> +++ b/drivers/input/serio/i8042-dt.h
> @@ -0,0 +1,112 @@
> +#ifndef _I8042_DT_H
> +#define _I8042_DT_H
> +
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +static void __iomem *i8042_base;
> +static unsigned int i8042_command_reg;
> +static unsigned int i8042_status_reg;
> +static unsigned int i8042_data_reg;
> +#define I8042_COMMAND_REG i8042_command_reg
> +#define I8042_STATUS_REG i8042_status_reg
> +#define I8042_DATA_REG i8042_data_reg
> +
> +/*
> + * Names.
> + */
> +
> +static const char *i8042_kbd_phys_desc;
> +static const char *i8042_aux_phys_desc;
> +static const char *i8042_mux_phys_desc;
> +#define I8042_KBD_PHYS_DESC i8042_kbd_phys_desc
> +#define I8042_AUX_PHYS_DESC i8042_aux_phys_desc
> +#define I8042_MUX_PHYS_DESC i8042_mux_phys_desc
> +
> +/*
> + * IRQs.
> + */
> +static int i8042_kbd_irq;
> +static int i8042_aux_irq;
> +#define I8042_KBD_IRQ i8042_kbd_irq
> +#define I8042_AUX_IRQ i8042_aux_irq

That's a lot of static values. Surely nothing physically prevents the
use of multiple i8042 chips?

> +
> +static inline int i8042_read_data(void)
> +{
> +	return readb(i8042_base + i8042_data_reg);
> +}
> +
> +static inline int i8042_read_status(void)
> +{
> +	return readb(i8042_base + i8042_status_reg);
> +}
> +
> +static inline void i8042_write_data(int val)
> +{
> +	writeb(val, i8042_base + i8042_data_reg);
> +}
> +
> +static inline void i8042_write_command(int val)
> +{
> +	writeb(val, i8042_base + i8042_command_reg);
> +}
> +
> +static inline int i8042_platform_init(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int status;
> +
> +	i8042_base = of_iomap(np, 0);
> +	if (!i8042_base)
> +		return -ENOMEM;
> +
> +	status = of_property_read_u32(np, "command-reg", &i8042_command_reg);
> +	if (status)
> +		return status;
> +
> +	status = of_property_read_u32(np, "status-reg", &i8042_status_reg);
> +	if (status)
> +		return status;
> +
> +	status = of_property_read_u32(np, "data-reg", &i8042_data_reg);
> +	if (status)
> +		return status;

You should probably validate that these are within the range provided in
the reg property.

You also need to clean up if you fail. It looks like here and below we
leak the i8042_base mapping if we decide to fail.

> +
> +	i8042_kbd_irq = irq_of_parse_and_map(np, 0);
> +	i8042_aux_irq = irq_of_parse_and_map(np, 1);

You can use platform_get_irq(pdev, N) here, the IRQ will already have
been parsed by the core.

> +	status = of_property_read_string(np, "linux,kbd_phys_desc",
> +						&i8042_kbd_phys_desc);
> +	if (status)
> +		i8042_kbd_phys_desc = "i8042/serio0";
> +
> +	status = of_property_read_string(np, "linux,aux_phys_desc",
> +						&i8042_aux_phys_desc);
> +	if (status)
> +		i8042_aux_phys_desc = "i8042/serio1";
> +
> +	status = of_property_read_string(np, "linux,mux_phys_desc",
> +						&i8042_mux_phys_desc);
> +	if (status)
> +		i8042_mux_phys_desc = "i8042/serio%d";

User-provided values as format strings? Not a good idea.

What exactly are these used for?

> +
> +	if (of_get_property(np, "init-reset", NULL))
> +		i8042_reset = true;

Use of_property_read_bool.

Thanks,
Mark.

Re: [PATCH 5/5] i8042: Add i8042_dt.h glue for DT support

[Roman Volkov]

В Tue, 3 Feb 2015 11:52:50 +0000
Mark Rutland <mark.rutland@arm.com> пишет:

> On Mon, Feb 02, 2015 at 09:48:50PM +0000, Roman Volkov wrote:
> > This header file designed to be similar to other glue layers found
> > for i8042. The difference is that interrupt numbers, device address,
> > and other information should be retrieved from the device tree.
> > 
> > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> > ---
> >  drivers/input/serio/i8042-dt.h | 112
> > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112
> > insertions(+) create mode 100644 drivers/input/serio/i8042-dt.h
> > 
> > diff --git a/drivers/input/serio/i8042-dt.h
> > b/drivers/input/serio/i8042-dt.h new file mode 100644
> > index 0000000..0d1a344
> > --- /dev/null
> > +++ b/drivers/input/serio/i8042-dt.h
> > @@ -0,0 +1,112 @@
> > +#ifndef _I8042_DT_H
> > +#define _I8042_DT_H
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +
> > +/*
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms of the GNU General Public License version 2 as
> > published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +static void __iomem *i8042_base;
> > +static unsigned int i8042_command_reg;
> > +static unsigned int i8042_status_reg;
> > +static unsigned int i8042_data_reg;
> > +#define I8042_COMMAND_REG i8042_command_reg
> > +#define I8042_STATUS_REG i8042_status_reg
> > +#define I8042_DATA_REG i8042_data_reg
> > +
> > +/*
> > + * Names.
> > + */
> > +
> > +static const char *i8042_kbd_phys_desc;
> > +static const char *i8042_aux_phys_desc;
> > +static const char *i8042_mux_phys_desc;
> > +#define I8042_KBD_PHYS_DESC i8042_kbd_phys_desc
> > +#define I8042_AUX_PHYS_DESC i8042_aux_phys_desc
> > +#define I8042_MUX_PHYS_DESC i8042_mux_phys_desc
> > +
> > +/*
> > + * IRQs.
> > + */
> > +static int i8042_kbd_irq;
> > +static int i8042_aux_irq;
> > +#define I8042_KBD_IRQ i8042_kbd_irq
> > +#define I8042_AUX_IRQ i8042_aux_irq
> 
> That's a lot of static values. Surely nothing physically prevents the
> use of multiple i8042 chips?
> 
> > +
> > +static inline int i8042_read_data(void)
> > +{
> > +	return readb(i8042_base + i8042_data_reg);
> > +}
> > +
> > +static inline int i8042_read_status(void)
> > +{
> > +	return readb(i8042_base + i8042_status_reg);
> > +}
> > +
> > +static inline void i8042_write_data(int val)
> > +{
> > +	writeb(val, i8042_base + i8042_data_reg);
> > +}
> > +
> > +static inline void i8042_write_command(int val)
> > +{
> > +	writeb(val, i8042_base + i8042_command_reg);
> > +}
> > +
> > +static inline int i8042_platform_init(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	int status;
> > +
> > +	i8042_base = of_iomap(np, 0);
> > +	if (!i8042_base)
> > +		return -ENOMEM;
> > +
> > +	status = of_property_read_u32(np, "command-reg",
> > &i8042_command_reg);
> > +	if (status)
> > +		return status;
> > +
> > +	status = of_property_read_u32(np, "status-reg",
> > &i8042_status_reg);
> > +	if (status)
> > +		return status;
> > +
> > +	status = of_property_read_u32(np, "data-reg",
> > &i8042_data_reg);
> > +	if (status)
> > +		return status;
> 
> You should probably validate that these are within the range provided
> in the reg property.
> 
> You also need to clean up if you fail. It looks like here and below we
> leak the i8042_base mapping if we decide to fail.
> 
> > +
> > +	i8042_kbd_irq = irq_of_parse_and_map(np, 0);
> > +	i8042_aux_irq = irq_of_parse_and_map(np, 1);
> 
> You can use platform_get_irq(pdev, N) here, the IRQ will already have
> been parsed by the core.
> 
> > +	status = of_property_read_string(np, "linux,kbd_phys_desc",
> > +
> > &i8042_kbd_phys_desc);
> > +	if (status)
> > +		i8042_kbd_phys_desc = "i8042/serio0";
> > +
> > +	status = of_property_read_string(np, "linux,aux_phys_desc",
> > +
> > &i8042_aux_phys_desc);
> > +	if (status)
> > +		i8042_aux_phys_desc = "i8042/serio1";
> > +
> > +	status = of_property_read_string(np, "linux,mux_phys_desc",
> > +
> > &i8042_mux_phys_desc);
> > +	if (status)
> > +		i8042_mux_phys_desc = "i8042/serio%d";
> 
> User-provided values as format strings? Not a good idea.
> 
> What exactly are these used for?
> 
> > +
> > +	if (of_get_property(np, "init-reset", NULL))
> > +		i8042_reset = true;
> 
> Use of_property_read_bool.
> 
> Thanks,
> Mark.

Mark, thanks for the good review. The current i8042 driver likely does
not support multiple controllers. Maybe Dmitry will comment something
regarding this and overall idea.

I will check the case when DTS contains multiple i8042-compatible
nodes. Expected behavior is that only the first node will be parsed and
next nodes are silently ignored.

Regards,
Roman.

Regards,
Roman.

Re: [PATCH 5/5] i8042: Add i8042_dt.h glue for DT support

[Dmitry Torokhov]

On Tue, Feb 03, 2015 at 10:14:32PM +0300, Roman Volkov wrote:
> В Tue, 3 Feb 2015 11:52:50 +0000
> Mark Rutland <mark.rutland@arm.com> пишет:
> 
> > On Mon, Feb 02, 2015 at 09:48:50PM +0000, Roman Volkov wrote:
> > > This header file designed to be similar to other glue layers found
> > > for i8042. The difference is that interrupt numbers, device address,
> > > and other information should be retrieved from the device tree.
> > > 
> > > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > > Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> > > ---
> > >  drivers/input/serio/i8042-dt.h | 112
> > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112
> > > insertions(+) create mode 100644 drivers/input/serio/i8042-dt.h
> > > 
> > > diff --git a/drivers/input/serio/i8042-dt.h
> > > b/drivers/input/serio/i8042-dt.h new file mode 100644
> > > index 0000000..0d1a344
> > > --- /dev/null
> > > +++ b/drivers/input/serio/i8042-dt.h
> > > @@ -0,0 +1,112 @@
> > > +#ifndef _I8042_DT_H
> > > +#define _I8042_DT_H
> > > +
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_irq.h>
> > > +
> > > +/*
> > > + * This program is free software; you can redistribute it and/or
> > > modify it
> > > + * under the terms of the GNU General Public License version 2 as
> > > published by
> > > + * the Free Software Foundation.
> > > + */
> > > +
> > > +static void __iomem *i8042_base;
> > > +static unsigned int i8042_command_reg;
> > > +static unsigned int i8042_status_reg;
> > > +static unsigned int i8042_data_reg;
> > > +#define I8042_COMMAND_REG i8042_command_reg
> > > +#define I8042_STATUS_REG i8042_status_reg
> > > +#define I8042_DATA_REG i8042_data_reg
> > > +
> > > +/*
> > > + * Names.
> > > + */
> > > +
> > > +static const char *i8042_kbd_phys_desc;
> > > +static const char *i8042_aux_phys_desc;
> > > +static const char *i8042_mux_phys_desc;
> > > +#define I8042_KBD_PHYS_DESC i8042_kbd_phys_desc
> > > +#define I8042_AUX_PHYS_DESC i8042_aux_phys_desc
> > > +#define I8042_MUX_PHYS_DESC i8042_mux_phys_desc
> > > +
> > > +/*
> > > + * IRQs.
> > > + */
> > > +static int i8042_kbd_irq;
> > > +static int i8042_aux_irq;
> > > +#define I8042_KBD_IRQ i8042_kbd_irq
> > > +#define I8042_AUX_IRQ i8042_aux_irq
> > 
> > That's a lot of static values. Surely nothing physically prevents the
> > use of multiple i8042 chips?

i8042 is currently is a singleton and I do not really see it changing.
That said I would like at some point to decouple setting up of the
platform device from the driver itself and maybe getting rig of all
these statics.

Thanks.

-- 
Dmitry

Re: [PATCH 1/5] i8042: intel-8042 DT documentation

[Dmitry Torokhov]

On Tue, Feb 03, 2015 at 11:38:16AM +0000, Mark Rutland wrote:
> On Mon, Feb 02, 2015 at 09:48:46PM +0000, Roman Volkov wrote:
> > Documentation for 'intel,8042' DT compatible node.
> > 
> > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> > ---
> >  .../devicetree/bindings/input/intel-8042.txt       | 29 ++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt b/Documentation/devicetree/bindings/input/intel-8042.txt
> > new file mode 100644
> > index 0000000..2aea7ec
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/intel-8042.txt
> > @@ -0,0 +1,29 @@
> > +* Intel 8042 Keyboard Controller
> > +
> > +Required properties:
> > +- compatible: should be "intel,8042"
> > +- regs: memory for keyboard controller
> > +- interrupts: two interrupts should be specified (keyboard and aux)
> 
> Is it possible only one of these is wired up?

Yes, and we should support this case. The core of i8042 does.


> 
> It might be worth using interrupt-names.
> 
> > +- command-reg: offset in memory for command register
> > +- status-reg: offset in memory for status register
> > +- data-reg: offset in memory for data register
> > +
> > +Optional properties:
> > +- init-reset: Controller should be reset on init and cleanup
> 
> Why is this necessary? Can't we just always reset it?

We do not reset by default on x86 because BIOS takes care of this for us
and quite often firmware that emulates i8042 gets confused if we try to
reset it too. Non non-x86 we reset by default. I think we should do the
same for OF case  (reset) and not use this property.

> 
> > +
> > +Optional Linux-specific properties:
> > +- linux,kbd_phys_desc: defaults to i8042/serio0
> > +- linux,aux_phys_desc: defaults to i8042/serio1
> > +- linux,mux_phys_desc: defaults to i8042/serio%d
> 
> As a general note, s/_/-/ in property names please.
> 
> That said, I don't follow why we should have these at all. I don't
> understand what the description is intended to mean.
> 
> In general we want to avoid Linux-specific properties. If a DTB needs to
> know about the inernals of an OS it's likely to be fragile and broken
> over time.

Right, the desc were carried over from older days to keep dmesg
familiar. With OF it is new platforms so just settle on a generic
description and use it instead of allowing to specify through DT.

> 
> > +
> > +
> > +Example:
> > +	keyboard@d8008800 {
> > +		compatible = "intel,8042";
> > +		reg = <0xd8008800 0x100>;
> > +		interrupts = <23 4>;
> 
> If this is intended to be two interrupts, please bracket them
> individually, e.g.
> 
> 	interrupts = <23>, <4>;
> 
> > +		command-reg = <0x04>;
> > +		status-reg = <0x04>;
> 
> Same address?
> 
> > +		data-reg = <0x00>;
> > +		mux-ports = <2>;
> 
> This wasn't documented above.

I think active MUX is purely x86 concept, I have never heard of it being
used anywhere else.

Thanks.

-- 
Dmitry

Re: [PATCH 4/5] i8042: Prepare i8042 driver for DT support

[Dmitry Torokhov]

On Tue, Feb 03, 2015 at 12:48:49AM +0300, Roman Volkov wrote:
> Use platform_device_probe() instead of platform_create_bundle() when
> compiled with DT support, since the latter function is not suitable for
> handling the OF device tree.
> 
> The order of initialization is changed, since i8042_platform_init() for DT
> requires initialized platform_device structure. To avoid searching of the
> compatible node twice, the platform_device structure pointer must be passed
> to the i8042_platform_init() function right after initialization by
> platform_device_probe().
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> ---
> 
> Yes, many of these ifdefs look ugly. Suggestions on how to avoid this are
> welcome (except using of_find_compatible_node() for searching the node twice
> before calling the probe function).

I guess we need to split the dirver into part that create platform
device and the standard driver part. Then your OF code can supply most
of the needed data via resources/platform data. Yes, it is a larger
change, but the current splat of ifdefs makes my eyes water.

Thanks.

-- 
Dmitry

Re: [PATCH 4/5] i8042: Prepare i8042 driver for DT support

[Roman Volkov]

В Tue, 3 Feb 2015 11:38:35 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> пишет:

> On Tue, Feb 03, 2015 at 12:48:49AM +0300, Roman Volkov wrote:
> > Use platform_device_probe() instead of platform_create_bundle() when
> > compiled with DT support, since the latter function is not suitable
> > for handling the OF device tree.
> > 
> > The order of initialization is changed, since i8042_platform_init()
> > for DT requires initialized platform_device structure. To avoid
> > searching of the compatible node twice, the platform_device
> > structure pointer must be passed to the i8042_platform_init()
> > function right after initialization by platform_device_probe().
> > 
> > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> > ---
> > 
> > Yes, many of these ifdefs look ugly. Suggestions on how to avoid
> > this are welcome (except using of_find_compatible_node() for
> > searching the node twice before calling the probe function).
> 
> I guess we need to split the dirver into part that create platform
> device and the standard driver part. Then your OF code can supply most
> of the needed data via resources/platform data. Yes, it is a larger
> change, but the current splat of ifdefs makes my eyes water.
> 
> Thanks.
> 

Dmitry, could you describe this in details? Currently there is
platform_create_bundle() function in the driver that is not acceptable
to be used with OF (it creates additional device). This function must be
replaced with platform_driver_probe() and it is all that common
between various architectures and can be the "standard" part of the
driver.

The difference is in device creation between OF case and, for example,
x86 case. Here is a chicken-and-egg problem that needs to be solved, if
we try to make the code consistent between various platforms. The
problem is that i8042_platform_init() expected to be called first, but
at this point DT version requires platform_device structure pointer.

Regards,
Roman.

Re: [PATCH 1/5] i8042: intel-8042 DT documentation

[Roman Volkov]

В Tue, 3 Feb 2015 11:32:02 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> пишет:

> On Tue, Feb 03, 2015 at 11:38:16AM +0000, Mark Rutland wrote:
> > On Mon, Feb 02, 2015 at 09:48:46PM +0000, Roman Volkov wrote:
> > > Documentation for 'intel,8042' DT compatible node.
> > > 
> > > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > > Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> > > ---
> > >  .../devicetree/bindings/input/intel-8042.txt       | 29
> > > ++++++++++++++++++++++ 1 file changed, 29 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/input/intel-8042.txt
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/input/intel-8042.txt
> > > b/Documentation/devicetree/bindings/input/intel-8042.txt new file
> > > mode 100644 index 0000000..2aea7ec --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/intel-8042.txt
> > > @@ -0,0 +1,29 @@
> > > +* Intel 8042 Keyboard Controller
> > > +
> > > +Required properties:
> > > +- compatible: should be "intel,8042"
> > > +- regs: memory for keyboard controller
> > > +- interrupts: two interrupts should be specified (keyboard and
> > > aux)
> > 
> > Is it possible only one of these is wired up?
> 
> Yes, and we should support this case. The core of i8042 does.
> 

Do we need to just read these IRQ numbers and leave them negative if
absent? Will it be acceptable? This would look like:

i8042_kbd_irq = platform_get_irq_byname(pdev, "kbd");

Testing shows it prints "Invalid argument" error -22 when an IRQ is
absent and we are not using nokbd/noaux module options.

> 
> > 
> > It might be worth using interrupt-names.
> > 
> > > +- command-reg: offset in memory for command register
> > > +- status-reg: offset in memory for status register
> > > +- data-reg: offset in memory for data register
> > > +
> > > +Optional properties:
> > > +- init-reset: Controller should be reset on init and cleanup
> > 
> > Why is this necessary? Can't we just always reset it?
> 
> We do not reset by default on x86 because BIOS takes care of this for
> us and quite often firmware that emulates i8042 gets confused if we
> try to reset it too. Non non-x86 we reset by default. I think we
> should do the same for OF case  (reset) and not use this property.
> 
> > 
> > > +
> > > +Optional Linux-specific properties:
> > > +- linux,kbd_phys_desc: defaults to i8042/serio0
> > > +- linux,aux_phys_desc: defaults to i8042/serio1
> > > +- linux,mux_phys_desc: defaults to i8042/serio%d
> > 
> > As a general note, s/_/-/ in property names please.
> > 
> > That said, I don't follow why we should have these at all. I don't
> > understand what the description is intended to mean.
> > 
> > In general we want to avoid Linux-specific properties. If a DTB
> > needs to know about the inernals of an OS it's likely to be fragile
> > and broken over time.
> 
> Right, the desc were carried over from older days to keep dmesg
> familiar. With OF it is new platforms so just settle on a generic
> description and use it instead of allowing to specify through DT.
> 
> > 
> > > +
> > > +
> > > +Example:
> > > +	keyboard@d8008800 {
> > > +		compatible = "intel,8042";
> > > +		reg = <0xd8008800 0x100>;
> > > +		interrupts = <23 4>;
> > 
> > If this is intended to be two interrupts, please bracket them
> > individually, e.g.
> > 
> > 	interrupts = <23>, <4>;
> > 
> > > +		command-reg = <0x04>;
> > > +		status-reg = <0x04>;
> > 
> > Same address?
> > 
> > > +		data-reg = <0x00>;
> > > +		mux-ports = <2>;
> > 
> > This wasn't documented above.
> 
> I think active MUX is purely x86 concept, I have never heard of it
> being used anywhere else.
> 
> Thanks.
> 

[PATCH v2 0/5] Device Tree support for i8042 driver

[Roman Volkov]

Yes, some embedded devices still use the i8042 controller. This patch set
enables the i8042 driver to get necessary information from Device Tree instead
of using specific headers with hardcoded addresses for each specific machine.
For example, vt8500 architecture has i8042.

v2:
Changes in the documentation.
Errors fixed in the initialization funtion.
Redundant parameters removed from Device Tree bindings (init-reset, etc.).

Roman Volkov (5):
  i8042: intel-8042 DT documentation
  i8042: Kernel configuration handling for DT support
  i8042: Add OF match table
  i8042: Prepare i8042 driver for DT support
  i8042: Add i8042_dt.h glue for DT support

 .../devicetree/bindings/input/intel-8042.txt       |  26 ++++++
 drivers/input/serio/i8042-dt.h                     | 104 +++++++++++++++++++++
 drivers/input/serio/i8042.c                        |  43 ++++++++-
 drivers/input/serio/i8042.h                        |   3 +
 4 files changed, 173 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt
 create mode 100644 drivers/input/serio/i8042-dt.h

--
Friday the 13-th
2.3.0

[PATCH v2 1/5] i8042: intel-8042 DT documentation

[Roman Volkov]

Documentation for 'intel,8042' DT compatible node.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 .../devicetree/bindings/input/intel-8042.txt       | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt

diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt b/Documentation/devicetree/bindings/input/intel-8042.txt
new file mode 100644
index 0000000..ab8a3e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/intel-8042.txt
@@ -0,0 +1,26 @@
+Intel 8042 Keyboard Controller
+
+Required properties:
+- compatible: should be "intel,8042"
+- regs: memory for keyboard controller
+- interrupts: usually, two interrupts should be specified (keyboard and aux).
+	However, only one interrupt is also allowed in case of absence of the
+	physical port in the controller. The i8042 driver must be loaded with
+	nokbd/noaux option in this case.
+- interrupt-names: interrupt names corresponding to numbers in the list.
+	"kbd" is the keyboard interrupt and "aux" is the auxiliary (mouse)
+	interrupt.
+- command-reg: offset in memory for command register
+- status-reg: offset in memory for status register
+- data-reg: offset in memory for data register
+
+Example:
+	i8042@d8008800 {
+		compatible = "intel,8042";
+		regs = <0xd8008800 0x100>;
+		interrupts = <23>, <4>;
+		interrupt-names = "kbd", "aux";
+		command-reg = <0x04>;
+		status-reg = <0x04>;
+		data-reg = <0x00>;
+	};
-- 
2.3.0

[PATCH v2 2/5] i8042: Kernel configuration handling for DT support

[Roman Volkov]

i8042_dt.h should be included when CONFIG_ARCH_MIGHT_HAVE_PC_SERIO and
CONFIG_USE_OF are selected. It should be not necessary to create
additional options in the kernel config.

Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..d2c1761 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -28,6 +28,9 @@
 #include "i8042-x86ia64io.h"
 #elif defined(CONFIG_UNICORE32)
 #include "i8042-unicore32io.h"
+#elif defined(CONFIG_ARCH_MIGHT_HAVE_PC_SERIO) && defined(CONFIG_USE_OF)
+#define SERIO_I8042_DT
+#include "i8042-dt.h"
 #else
 #include "i8042-io.h"
 #endif
-- 
2.3.0

[PATCH v2 3/5] i8042: Add OF match table

[Roman Volkov]

The OF device table allows the platform_driver_probe() function to
automatically match device and parse the DT node.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 986a71c..2f09062 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1474,12 +1474,22 @@ static int i8042_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef SERIO_I8042_DT
+static struct of_device_id i8042_dt_ids[] = {
+	{ .compatible = "intel,8042" },
+	{ /* Sentinel */ },
+};
+#endif
+
 static struct platform_driver i8042_driver = {
 	.driver		= {
 		.name	= "i8042",
 #ifdef CONFIG_PM
 		.pm	= &i8042_pm_ops,
 #endif
+#ifdef SERIO_I8042_DT
+		.of_match_table = i8042_dt_ids,
+#endif
 	},
 	.remove		= i8042_remove,
 	.shutdown	= i8042_shutdown,
-- 
2.3.0

[PATCH v2 4/5] i8042: Prepare i8042 driver for DT support

[Roman Volkov]

Use platform_device_probe() instead of platform_create_bundle() when
compiled with DT support, since the latter function is not suitable for
handling the OF device tree.

The order of initialization is changed, since i8042_platform_init() for DT
requires initialized platform_device structure. To avoid searching of the
compatible node twice, the platform_device structure pointer must be passed
to the i8042_platform_init() function right after initialization by
platform_device_probe().

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
This is remaining weak place in the patch set. No ideas yet on
how to reduce the using of ifdefs to make code cleaner, need more discussion.

 drivers/input/serio/i8042.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 2f09062..86a47ec 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1421,16 +1421,32 @@ static int __init i8042_probe(struct platform_device *dev)
 	int error;
 
 	i8042_platform_device = dev;
+#ifdef SERIO_I8042_DT
+	error = i8042_platform_init(dev);
+	if (error)
+		return error;
 
+	error = i8042_controller_check();
+	if (error)
+		goto out_platform_exit;
+#endif
 	if (i8042_reset) {
 		error = i8042_controller_selftest();
 		if (error)
+#ifdef SERIO_I8042_DT
+			goto out_platform_exit;
+#else
 			return error;
+#endif
 	}
 
 	error = i8042_controller_init();
 	if (error)
+#ifdef SERIO_I8042_DT
+		goto out_platform_exit;
+#else
 		return error;
+#endif
 
 #ifdef CONFIG_X86
 	if (i8042_dritek)
@@ -1460,7 +1476,10 @@ static int __init i8042_probe(struct platform_device *dev)
 	i8042_free_irqs();
 	i8042_controller_reset(false);
 	i8042_platform_device = NULL;
-
+#ifdef SERIO_I8042_DT
+ out_platform_exit:
+	i8042_platform_exit();
+#endif
 	return error;
 }
 
@@ -1497,11 +1516,18 @@ static struct platform_driver i8042_driver = {
 
 static int __init i8042_init(void)
 {
+#ifndef SERIO_I8042_DT
 	struct platform_device *pdev;
+#endif
 	int err;
 
 	dbg_init();
 
+#ifdef SERIO_I8042_DT
+	err = platform_driver_probe(&i8042_driver, i8042_probe);
+	if (err)
+		return err;
+#else
 	err = i8042_platform_init();
 	if (err)
 		return err;
@@ -1515,14 +1541,15 @@ static int __init i8042_init(void)
 		err = PTR_ERR(pdev);
 		goto err_platform_exit;
 	}
-
+#endif
 	panic_blink = i8042_panic_blink;
 
 	return 0;
-
+#ifndef SERIO_I8042_DT
  err_platform_exit:
 	i8042_platform_exit();
 	return err;
+#endif
 }
 
 static void __exit i8042_exit(void)
-- 
2.3.0

[PATCH v2 5/5] i8042: Add i8042_dt.h glue for DT support

[Roman Volkov]

This header file designed to be similar to other glue layers found
for i8042. The difference is that interrupt numbers, device address,
and other information should be retrieved from the device tree.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042-dt.h | 104 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 drivers/input/serio/i8042-dt.h

diff --git a/drivers/input/serio/i8042-dt.h b/drivers/input/serio/i8042-dt.h
new file mode 100644
index 0000000..c0b319a
--- /dev/null
+++ b/drivers/input/serio/i8042-dt.h
@@ -0,0 +1,104 @@
+#ifndef _I8042_DT_H
+#define _I8042_DT_H
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+static void __iomem *i8042_base;
+static unsigned int i8042_command_reg;
+static unsigned int i8042_status_reg;
+static unsigned int i8042_data_reg;
+#define I8042_COMMAND_REG i8042_command_reg
+#define I8042_STATUS_REG i8042_status_reg
+#define I8042_DATA_REG i8042_data_reg
+
+/*
+ * Names.
+ */
+
+#define I8042_KBD_PHYS_DESC "i8042/serio0"
+#define I8042_AUX_PHYS_DESC "i8042/serio1"
+#define I8042_MUX_PHYS_DESC "i8042/serio%d"
+
+/*
+ * IRQs.
+ */
+static int i8042_kbd_irq;
+static int i8042_aux_irq;
+#define I8042_KBD_IRQ i8042_kbd_irq
+#define I8042_AUX_IRQ i8042_aux_irq
+
+static inline int i8042_read_data(void)
+{
+	return readb(i8042_base + i8042_data_reg);
+}
+
+static inline int i8042_read_status(void)
+{
+	return readb(i8042_base + i8042_status_reg);
+}
+
+static inline void i8042_write_data(int val)
+{
+	writeb(val, i8042_base + i8042_data_reg);
+}
+
+static inline void i8042_write_command(int val)
+{
+	writeb(val, i8042_base + i8042_command_reg);
+}
+
+static inline int i8042_platform_init(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const __be32 *regbase_p;
+	u64 regsize;
+	int status;
+
+	regbase_p = of_get_address(np, 0, &regsize, NULL);
+	if (!regbase_p)
+		return -EINVAL;
+
+	status = of_property_read_u32(np, "command-reg", &i8042_command_reg);
+	if (status)
+		return status;
+
+	status = of_property_read_u32(np, "status-reg", &i8042_status_reg);
+	if (status)
+		return status;
+
+	status = of_property_read_u32(np, "data-reg", &i8042_data_reg);
+	if (status)
+		return status;
+
+	if ((i8042_command_reg >= regsize) || (i8042_status_reg >= regsize) ||
+			(i8042_data_reg >= regsize))
+		return -EINVAL;
+
+	i8042_kbd_irq = platform_get_irq_byname(pdev, "kbd");
+	i8042_aux_irq = platform_get_irq_byname(pdev, "aux");
+
+	i8042_base = ioremap((unsigned long)of_translate_address(np, regbase_p),
+			(unsigned long)regsize);
+	if (!i8042_base)
+		return -ENOMEM;
+
+	i8042_reset = true;
+
+	return 0;
+}
+
+static inline void i8042_platform_exit(void)
+{
+	if (i8042_base)
+		iounmap(i8042_base);
+}
+
+#endif
-- 
2.3.0

[PATCH v3 0/5] Device Tree support for i8042 driver

[Roman Volkov]

Yes, some embedded devices still use the i8042 controller. This patch set
enables the i8042 driver to get necessary information from Device Tree
instead of using specific headers with hardcoded addresses for each
specific machine. For example, vt8500 architecture has i8042.

 v2:
 -Changes in the documentation.
 -Errors fixed in the initialization function.
 -Redundant parameters removed from Device Tree bindings (init-reset, etc.).

 v3:
 -Reduced amount of 'ifdefs' in the i8042.c file. Initialization order
    is now the same for all architectures.

 Warning: the change in v3 is more controversial and needs more testing
 since affects all architectures which use the driver. Please give
 information regarding required steps to get this patch accepted. Would
 be great to find testers to get additional tested-by records.

Roman Volkov (5):
  i8042: intel-8042 DT documentation
  i8042: Kernel configuration handling for DT support
  i8042: Add OF match table
  i8042: Prepare i8042 driver for DT support
  i8042: Add i8042_dt.h glue for DT support

 .../devicetree/bindings/input/intel-8042.txt       |  26 ++++++
 drivers/input/serio/i8042-dt.h                     | 104 +++++++++++++++++++++
 drivers/input/serio/i8042.c                        |  56 ++++++-----
 drivers/input/serio/i8042.h                        |   3 +
 4 files changed, 168 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt
 create mode 100644 drivers/input/serio/i8042-dt.h

-- 
2.3.0

[PATCH v3 1/5] i8042: intel-8042 DT documentation

[Roman Volkov]

Documentation for 'intel,8042' DT compatible node.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 .../devicetree/bindings/input/intel-8042.txt       | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt

diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt b/Documentation/devicetree/bindings/input/intel-8042.txt
new file mode 100644
index 0000000..ab8a3e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/intel-8042.txt
@@ -0,0 +1,26 @@
+Intel 8042 Keyboard Controller
+
+Required properties:
+- compatible: should be "intel,8042"
+- regs: memory for keyboard controller
+- interrupts: usually, two interrupts should be specified (keyboard and aux).
+	However, only one interrupt is also allowed in case of absence of the
+	physical port in the controller. The i8042 driver must be loaded with
+	nokbd/noaux option in this case.
+- interrupt-names: interrupt names corresponding to numbers in the list.
+	"kbd" is the keyboard interrupt and "aux" is the auxiliary (mouse)
+	interrupt.
+- command-reg: offset in memory for command register
+- status-reg: offset in memory for status register
+- data-reg: offset in memory for data register
+
+Example:
+	i8042@d8008800 {
+		compatible = "intel,8042";
+		regs = <0xd8008800 0x100>;
+		interrupts = <23>, <4>;
+		interrupt-names = "kbd", "aux";
+		command-reg = <0x04>;
+		status-reg = <0x04>;
+		data-reg = <0x00>;
+	};
-- 
2.3.0

[PATCH v3 2/5] i8042: Kernel configuration handling for DT support

[Roman Volkov]

i8042_dt.h should be included when CONFIG_ARCH_MIGHT_HAVE_PC_SERIO and
CONFIG_USE_OF are selected. It should be not necessary to create
additional options in the kernel config.

Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h
index fc080be..d2c1761 100644
--- a/drivers/input/serio/i8042.h
+++ b/drivers/input/serio/i8042.h
@@ -28,6 +28,9 @@
 #include "i8042-x86ia64io.h"
 #elif defined(CONFIG_UNICORE32)
 #include "i8042-unicore32io.h"
+#elif defined(CONFIG_ARCH_MIGHT_HAVE_PC_SERIO) && defined(CONFIG_USE_OF)
+#define SERIO_I8042_DT
+#include "i8042-dt.h"
 #else
 #include "i8042-io.h"
 #endif
-- 
2.3.0

[PATCH v3 3/5] i8042: Add OF match table

[Roman Volkov]

The OF device table allows the platform_driver_probe() function to
automatically match device and parse the DT node.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 986a71c..2f09062 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1474,12 +1474,22 @@ static int i8042_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef SERIO_I8042_DT
+static struct of_device_id i8042_dt_ids[] = {
+	{ .compatible = "intel,8042" },
+	{ /* Sentinel */ },
+};
+#endif
+
 static struct platform_driver i8042_driver = {
 	.driver		= {
 		.name	= "i8042",
 #ifdef CONFIG_PM
 		.pm	= &i8042_pm_ops,
 #endif
+#ifdef SERIO_I8042_DT
+		.of_match_table = i8042_dt_ids,
+#endif
 	},
 	.remove		= i8042_remove,
 	.shutdown	= i8042_shutdown,
-- 
2.3.0

[PATCH v3 4/5] i8042: Prepare i8042 driver for DT support

[Roman Volkov]

Move i8042_platform_init() call from i8042_init() to i8042_probe() to
pass the platform_device structure pointer, since the former function
now requires this argument.

Use platform_create_bundle() when there is no DT support in the kernel,
and platform_driver_probe() otherwise, which does not create a device.

Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042.c | 46 ++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
index 2f09062..96b62fd 100644
--- a/drivers/input/serio/i8042.c
+++ b/drivers/input/serio/i8042.c
@@ -1422,6 +1422,18 @@ static int __init i8042_probe(struct platform_device *dev)
 
 	i8042_platform_device = dev;
 
+#ifdef SERIO_I8042_DT
+	error = i8042_platform_init(dev);
+#else
+	error = i8042_platform_init();
+#endif
+	if (error)
+		return error;
+
+	error = i8042_controller_check();
+	if (error)
+		goto out_platform_exit;
+
 	if (i8042_reset) {
 		error = i8042_controller_selftest();
 		if (error)
@@ -1440,13 +1452,13 @@ static int __init i8042_probe(struct platform_device *dev)
 	if (!i8042_noaux) {
 		error = i8042_setup_aux();
 		if (error && error != -ENODEV && error != -EBUSY)
-			goto out_fail;
+			goto out_res_free;
 	}
 
 	if (!i8042_nokbd) {
 		error = i8042_setup_kbd();
 		if (error)
-			goto out_fail;
+			goto out_res_free;
 	}
 /*
  * Ok, everything is ready, let's register all serio ports
@@ -1455,11 +1467,13 @@ static int __init i8042_probe(struct platform_device *dev)
 
 	return 0;
 
- out_fail:
+ out_res_free:
 	i8042_free_aux_ports();	/* in case KBD failed but AUX not */
 	i8042_free_irqs();
 	i8042_controller_reset(false);
 	i8042_platform_device = NULL;
+ out_platform_exit:
+	i8042_platform_exit();
 
 	return error;
 }
@@ -1498,36 +1512,26 @@ static struct platform_driver i8042_driver = {
 static int __init i8042_init(void)
 {
 	struct platform_device *pdev;
-	int err;
 
 	dbg_init();
-
-	err = i8042_platform_init();
-	if (err)
-		return err;
-
-	err = i8042_controller_check();
-	if (err)
-		goto err_platform_exit;
-
+#ifdef SERIO_I8042_DT
+	pdev = ERR_PTR(platform_driver_probe(&i8042_driver, i8042_probe));
+#else
 	pdev = platform_create_bundle(&i8042_driver, i8042_probe, NULL, 0, NULL, 0);
-	if (IS_ERR(pdev)) {
-		err = PTR_ERR(pdev);
-		goto err_platform_exit;
-	}
+#endif
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
 
 	panic_blink = i8042_panic_blink;
 
 	return 0;
-
- err_platform_exit:
-	i8042_platform_exit();
-	return err;
 }
 
 static void __exit i8042_exit(void)
 {
+#ifndef SERIO_I8042_DT
 	platform_device_unregister(i8042_platform_device);
+#endif
 	platform_driver_unregister(&i8042_driver);
 	i8042_platform_exit();
 
-- 
2.3.0

[PATCH v3 5/5] i8042: Add i8042_dt.h glue for DT support

[Roman Volkov]

This header file designed to be similar to other glue layers found
for i8042. The difference is that interrupt numbers, device address,
and other information should be retrieved from the device tree.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Signed-off-by: Roman Volkov <v1ron@v1ros.org>
---
 drivers/input/serio/i8042-dt.h | 104 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100644 drivers/input/serio/i8042-dt.h

diff --git a/drivers/input/serio/i8042-dt.h b/drivers/input/serio/i8042-dt.h
new file mode 100644
index 0000000..c0b319a
--- /dev/null
+++ b/drivers/input/serio/i8042-dt.h
@@ -0,0 +1,104 @@
+#ifndef _I8042_DT_H
+#define _I8042_DT_H
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+static void __iomem *i8042_base;
+static unsigned int i8042_command_reg;
+static unsigned int i8042_status_reg;
+static unsigned int i8042_data_reg;
+#define I8042_COMMAND_REG i8042_command_reg
+#define I8042_STATUS_REG i8042_status_reg
+#define I8042_DATA_REG i8042_data_reg
+
+/*
+ * Names.
+ */
+
+#define I8042_KBD_PHYS_DESC "i8042/serio0"
+#define I8042_AUX_PHYS_DESC "i8042/serio1"
+#define I8042_MUX_PHYS_DESC "i8042/serio%d"
+
+/*
+ * IRQs.
+ */
+static int i8042_kbd_irq;
+static int i8042_aux_irq;
+#define I8042_KBD_IRQ i8042_kbd_irq
+#define I8042_AUX_IRQ i8042_aux_irq
+
+static inline int i8042_read_data(void)
+{
+	return readb(i8042_base + i8042_data_reg);
+}
+
+static inline int i8042_read_status(void)
+{
+	return readb(i8042_base + i8042_status_reg);
+}
+
+static inline void i8042_write_data(int val)
+{
+	writeb(val, i8042_base + i8042_data_reg);
+}
+
+static inline void i8042_write_command(int val)
+{
+	writeb(val, i8042_base + i8042_command_reg);
+}
+
+static inline int i8042_platform_init(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const __be32 *regbase_p;
+	u64 regsize;
+	int status;
+
+	regbase_p = of_get_address(np, 0, &regsize, NULL);
+	if (!regbase_p)
+		return -EINVAL;
+
+	status = of_property_read_u32(np, "command-reg", &i8042_command_reg);
+	if (status)
+		return status;
+
+	status = of_property_read_u32(np, "status-reg", &i8042_status_reg);
+	if (status)
+		return status;
+
+	status = of_property_read_u32(np, "data-reg", &i8042_data_reg);
+	if (status)
+		return status;
+
+	if ((i8042_command_reg >= regsize) || (i8042_status_reg >= regsize) ||
+			(i8042_data_reg >= regsize))
+		return -EINVAL;
+
+	i8042_kbd_irq = platform_get_irq_byname(pdev, "kbd");
+	i8042_aux_irq = platform_get_irq_byname(pdev, "aux");
+
+	i8042_base = ioremap((unsigned long)of_translate_address(np, regbase_p),
+			(unsigned long)regsize);
+	if (!i8042_base)
+		return -ENOMEM;
+
+	i8042_reset = true;
+
+	return 0;
+}
+
+static inline void i8042_platform_exit(void)
+{
+	if (i8042_base)
+		iounmap(i8042_base);
+}
+
+#endif
-- 
2.3.0

Re: [PATCH v3 1/5] i8042: intel-8042 DT documentation

[Dmitry Torokhov]

Hi Roman,

On Mon, Feb 16, 2015 at 12:11:43AM +0300, Roman Volkov wrote:
> Documentation for 'intel,8042' DT compatible node.
> 
> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> ---
>  .../devicetree/bindings/input/intel-8042.txt       | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/intel-8042.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/intel-8042.txt b/Documentation/devicetree/bindings/input/intel-8042.txt
> new file mode 100644
> index 0000000..ab8a3e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/intel-8042.txt
> @@ -0,0 +1,26 @@
> +Intel 8042 Keyboard Controller
> +
> +Required properties:
> +- compatible: should be "intel,8042"
> +- regs: memory for keyboard controller
> +- interrupts: usually, two interrupts should be specified (keyboard and aux).
> +	However, only one interrupt is also allowed in case of absence of the
> +	physical port in the controller. The i8042 driver must be loaded with
> +	nokbd/noaux option in this case.
> +- interrupt-names: interrupt names corresponding to numbers in the list.
> +	"kbd" is the keyboard interrupt and "aux" is the auxiliary (mouse)
> +	interrupt.
> +- command-reg: offset in memory for command register
> +- status-reg: offset in memory for status register
> +- data-reg: offset in memory for data register
> +
> +Example:
> +	i8042@d8008800 {
> +		compatible = "intel,8042";
> +		regs = <0xd8008800 0x100>;
> +		interrupts = <23>, <4>;
> +		interrupt-names = "kbd", "aux";
> +		command-reg = <0x04>;
> +		status-reg = <0x04>;
> +		data-reg = <0x00>;
> +	};

No, we already have existing OF bindings for i8042 on sparc and powerpc,
I do not think we need to invent a brand new one.

Thanks.

-- 
Dmitry

Re: [PATCH v3 5/5] i8042: Add i8042_dt.h glue for DT support

[Roman Volkov]

Something prevents me from following the thread, replying to myself.

> В Sat, 14 Mar 2015 20:20:38 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>   
> > 
> > Hi Roman,
> > 
> > On Mon, Feb 16, 2015 at 12:11:43AM +0300, Roman Volkov wrote:    
> > > Documentation for 'intel,8042' DT compatible node.
> > > 
> > > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > > Signed-off-by: Roman Volkov <v1ron@v1ros.org>
> > > ---
> > >  .../devicetree/bindings/input/intel-8042.txt       | 26
> > > ++++++++++++++++++++++ 1 file changed, 26 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/input/intel-8042.txt
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/input/intel-8042.txt
> > > b/Documentation/devicetree/bindings/input/intel-8042.txt new file
> > > mode 100644 index 0000000..ab8a3e0 --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/intel-8042.txt
> > > @@ -0,0 +1,26 @@
> > > +Intel 8042 Keyboard Controller
> > > +
> > > +Required properties:
> > > +- compatible: should be "intel,8042"
> > > +- regs: memory for keyboard controller
> > > +- interrupts: usually, two interrupts should be specified
> > > (keyboard and aux).
> > > +	However, only one interrupt is also allowed in case of
> > > absence of the
> > > +	physical port in the controller. The i8042 driver must be
> > > loaded with
> > > +	nokbd/noaux option in this case.
> > > +- interrupt-names: interrupt names corresponding to numbers in
> > > the list.
> > > +	"kbd" is the keyboard interrupt and "aux" is the
> > > auxiliary (mouse)
> > > +	interrupt.
> > > +- command-reg: offset in memory for command register
> > > +- status-reg: offset in memory for status register
> > > +- data-reg: offset in memory for data register
> > > +
> > > +Example:
> > > +	i8042@d8008800 {
> > > +		compatible = "intel,8042";
> > > +		regs = <0xd8008800 0x100>;
> > > +		interrupts = <23>, <4>;
> > > +		interrupt-names = "kbd", "aux";
> > > +		command-reg = <0x04>;
> > > +		status-reg = <0x04>;
> > > +		data-reg = <0x00>;
> > > +	};    
> > 
> > No, we already have existing OF bindings for i8042 on sparc and
> > powerpc, I do not think we need to invent a brand new one.
> > 
> > Thanks.
> > 

I have looked more into that header (i8042-sparcio.h). Probably 8042 is
broken for SPARCs, because there is the platform_driver registered twice
with the same name "i8042". One driver is registered by
platform_create_bundle() from i8042.c and another from
platform_driver_register() from the i8042_sparcio.h. Kernel prints a
message "Error: Driver 'i8042' is already registered, aborting...".

I have not tested SPARC version of the kernel, just used the same code
for ARM, to rewrite my patch set.

There is no bindings at all for i8042, because a binding requires
the 'compatible' property to be defined, according to ePAPR. In the
SPARC header there is an ancient surrogate with searching by name
instead of the 'compatible' property (I understand, no standards were
defined for that hardware those days).

Regards,
Roman