[PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[VishnuPatekar]

v2 --> v3
1. changed config to SERIO_SUN4I_PS2 from SERIO_SUNXI_PS2
2. changed driver name to sun4i-ps2 from sunxi-ps2.
3. changed the function names to sun4i_ps2_*.
4. added locking in sun4i_ps2_open.
5. kept compatible "sun4i-a10-ps2" for A10 and A20, as A10 is earlier SOC.
6. corrected the style errors.
7. separated the dts patches.
8. removed commented ps2 notes from lime2 dts.
9. added note that ps2 pins confilt with hdmi.
10. corrected the interrupt property for A10.
11. moved dt-bindings to Documentation/devicetree/bindings/serio

v1 --> v2: 
1. added default n depends on ARCH_SUNXI || COMPILE_TEST in Kconfig. 
2. handled errors and free resources on errors. 
3. used BIT(x), DIV_ROUND_UP macros. 
4. corrected style errors. 
5. added support for A10 also, A10 and A2 have same properties of PS2 controller. 
6. by default commented ps20 and ps21 nodes,as ps20 pins conflict with HDMI.
7. added compatible as allwinner,sun4i-a10-ps2. 
8. corrected the possible race condition. 


VishnuPatekar (5):
  sunxi:dts-bindings:input:ps2 bindings for A10/A20 ps2.
  ARM:sunxi:drivers:input Add support for A10/A20 PS2
  ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20
  ARM: sunxi: dts: Add A10/A20 PS2 pin muxing options
  ARM: sunxi: dts: Add note ps2 pins conflict with hdmi

 .../bindings/serio/allwinner,sun4i-ps2.txt         |   23 ++
 arch/arm/boot/dts/sun4i-a10.dtsi                   |   31 ++
 arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts    |    6 +-
 arch/arm/boot/dts/sun7i-a20.dtsi                   |   32 ++
 drivers/input/serio/Kconfig                        |   11 +
 drivers/input/serio/Makefile                       |    1 +
 drivers/input/serio/sun4i-ps2.c                    |  362 ++++++++++++++++++++
 7 files changed, 465 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
 create mode 100644 drivers/input/serio/sun4i-ps2.c

-- 
1.7.9.5

[PATCHv3 1/5] sunxi:dts-bindings:input:ps2 bindings for A10/A20 ps2.

[VishnuPatekar]

1. dt bindings should use the compat string for the earliest version of the
hardware which has the relevant hardware block, unless there are differences,
the A10 and A20 ps2 controllers are identical, so for both sun4i-a10-ps2
should be used as compat string.
2. ps2 / serio bindings belong under Documentation/devicetree/bindings/serio

Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../bindings/serio/allwinner,sun4i-ps2.txt         |   23 ++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt

diff --git a/Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt b/Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
new file mode 100644
index 0000000..0c30440
--- /dev/null
+++ b/Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
@@ -0,0 +1,23 @@
+* Device tree bindings for Allwinner A10, A20 PS2 host controller
+
+A20 PS2 is dual role controller(PS2 host and PS2 device). These bindings are
+for PS2 A10/A20 host controller. IBM compliant IBM PS2 and AT-compatible keyboard
+and mouse can be connected.
+
+Required properties:
+
+ - reg             : Offset and length of the register set for the device.
+ - compatible      : Should be as of the following:
+                     - "allwinner,sun4i-a10-ps2"
+ - interrupts      : The interrupt line connected to the PS2.
+ - clocks          : The gate clk connected to the PS2.
+
+
+Example:
+	ps20: ps2@0x01c2a000 {
+		compatible = "allwinner,sun4i-a10-ps2";
+		reg = <0x01c2a000 0x400>;
+		interrupts = <0 62 4>;
+		clocks = <&apb1_gates 6>;
+		status = "disabled";
+	};
-- 
1.7.9.5

[PATCHv3 2/5] ARM:sunxi:drivers:input Add support for A10/A20 PS2

[VishnuPatekar]

Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
---
 drivers/input/serio/Kconfig     |   11 ++
 drivers/input/serio/Makefile    |    1 +
 drivers/input/serio/sun4i-ps2.c |  362 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 374 insertions(+)
 create mode 100644 drivers/input/serio/sun4i-ps2.c

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..964afc5 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -281,4 +281,15 @@ config HYPERV_KEYBOARD
 	  To compile this driver as a module, choose M here: the module will
 	  be called hyperv_keyboard.
 
+config SERIO_SUN4I_PS2
+	tristate "Allwinner A10 PS/2 controller support"
+	default n
+	depends on ARCH_SUNXI || COMPILE_TEST
+	help
+	  This selects support for the PS/2 Host Controller on
+	  Allwinner A10.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sun4i-ps2.
+
 endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 815d874..c600089 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_SERIO_ARC_PS2)	+= arc_ps2.o
 obj-$(CONFIG_SERIO_APBPS2)	+= apbps2.o
 obj-$(CONFIG_SERIO_OLPC_APSP)	+= olpc_apsp.o
 obj-$(CONFIG_HYPERV_KEYBOARD)	+= hyperv-keyboard.o
+obj-$(CONFIG_SERIO_SUN4I_PS2)	+= sun4i-ps2.o
diff --git a/drivers/input/serio/sun4i-ps2.c b/drivers/input/serio/sun4i-ps2.c
new file mode 100644
index 0000000..e6d22ae
--- /dev/null
+++ b/drivers/input/serio/sun4i-ps2.c
@@ -0,0 +1,362 @@
+/*
+ *	Driver for Allwinner A10 PS2 host controller
+ *
+ *	Author: Vishnu Patekar <vishnupatekar0510@gmail.com>
+ *		Aaron.maoye <leafy.myeh@newbietech.com>
+ *
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/serio.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+
+#define DRIVER_NAME		"sun4i-ps2"
+
+/* register offset definitions */
+#define PS2_REG_GCTL		(0x00)	/*  PS2 Module Global Control Reg */
+#define PS2_REG_DATA		(0x04)  /*  PS2 Module Data Reg		*/
+#define PS2_REG_LCTL		(0x08)	/*  PS2 Module Line Control Reg */
+#define PS2_REG_LSTS		(0x0C)  /*  PS2 Module Line Status Reg	*/
+#define PS2_REG_FCTL		(0x10)	/*  PS2 Module FIFO Control Reg */
+#define PS2_REG_FSTS		(0x14)	/*  PS2 Module FIFO Status Reg	*/
+#define PS2_REG_CLKDR		(0x18)	/*  PS2 Module Clock Divider Reg*/
+
+/*  PS2 GLOBAL CONTROL REGISTER PS2_GCTL */
+#define PS2_GCTL_INTFLAG	BIT(4)
+#define PS2_GCTL_INTEN		BIT(3)
+#define PS2_GCTL_RESET		BIT(2)
+#define PS2_GCTL_MASTER		BIT(1)
+#define PS2_GCTL_BUSEN		BIT(0)
+
+/* PS2 LINE CONTROL REGISTER */
+#define PS2_LCTL_NOACK		BIT(18)
+#define PS2_LCTL_TXDTOEN	BIT(8)
+#define PS2_LCTL_STOPERREN	BIT(3)
+#define PS2_LCTL_ACKERREN	BIT(2)
+#define PS2_LCTL_PARERREN	BIT(1)
+#define PS2_LCTL_RXDTOEN	BIT(0)
+
+/* PS2 LINE STATUS REGISTER */
+#define PS2_LSTS_TXTDO		BIT(8)
+#define PS2_LSTS_STOPERR	BIT(3)
+#define PS2_LSTS_ACKERR		BIT(2)
+#define PS2_LSTS_PARERR		BIT(1)
+#define PS2_LSTS_RXTDO		BIT(0)
+
+#define PS2_LINE_ERROR_BIT \
+	(PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR | \
+	PS2_LSTS_PARERR | PS2_LSTS_RXTDO)
+
+/* PS2 FIFO CONTROL REGISTER */
+#define PS2_FCTL_TXRST		BIT(17)
+#define PS2_FCTL_RXRST		BIT(16)
+#define PS2_FCTL_TXUFIEN	BIT(10)
+#define PS2_FCTL_TXOFIEN	BIT(9)
+#define PS2_FCTL_TXRDYIEN	BIT(8)
+#define PS2_FCTL_RXUFIEN	BIT(2)
+#define PS2_FCTL_RXOFIEN	BIT(1)
+#define PS2_FCTL_RXRDYIEN	BIT(0)
+
+/* PS2 FIFO STATUS REGISTER */
+#define PS2_FSTS_TXUF		BIT(10)
+#define PS2_FSTS_TXOF		BIT(9)
+#define PS2_FSTS_TXRDY		BIT(8)
+#define PS2_FSTS_RXUF		BIT(2)
+#define PS2_FSTS_RXOF		BIT(1)
+#define PS2_FSTS_RXRDY		BIT(0)
+
+#define PS2_FIFO_ERROR_BIT \
+	(PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_RXUF | PS2_FSTS_RXOF)
+
+#define PS2_SAMPLE_CLK		(1000000)
+#define PS2_SCLK		(125000)
+
+struct sun4i_ps2data {
+	struct serio *serio;
+	struct device *dev;
+
+	/* IO mapping base */
+	void __iomem	*reg_base;
+
+	/* clock management */
+	struct clk	*clk;
+
+	/* irq */
+	spinlock_t	lock;
+	int		irq;
+};
+
+/*********************/
+/* Interrupt handler */
+/*********************/
+static irqreturn_t sun4i_ps2_interrupt(int irq, void *dev_id)
+{
+	struct sun4i_ps2data *drvdata = dev_id;
+	u32 intr_status;
+	u32 fifo_status;
+	unsigned char byte;
+	u32 rval;
+	u32 error = 0;
+
+	spin_lock(&drvdata->lock);
+
+	/* Get the PS/2 interrupts and clear them */
+	intr_status  = readl(drvdata->reg_base + PS2_REG_LSTS);
+	fifo_status  = readl(drvdata->reg_base + PS2_REG_FSTS);
+
+	/*Check Line Status Register*/
+	if (intr_status & PS2_LINE_ERROR_BIT) {
+		if (intr_status & PS2_LSTS_STOPERR)
+			dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
+		if (intr_status & PS2_LSTS_ACKERR)
+			dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
+		if (intr_status & PS2_LSTS_PARERR)
+			dev_info(drvdata->dev, "PS/2 Parity Error!\n");
+		if (intr_status & PS2_LSTS_TXTDO)
+			dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
+		if (intr_status & PS2_LSTS_RXTDO)
+			dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
+
+		/*reset PS/2 controller*/
+		rval = readl(drvdata->reg_base + PS2_REG_GCTL);
+		writel(rval | PS2_GCTL_RESET, drvdata->reg_base + PS2_REG_GCTL);
+
+		rval = PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR |
+			PS2_LSTS_PARERR | PS2_LSTS_RXTDO;
+		writel(rval, drvdata->reg_base + PS2_REG_LSTS);
+		error = 1;
+	}
+
+	/*Check FIFO Status Register*/
+	if (fifo_status & PS2_FIFO_ERROR_BIT) {
+		if (fifo_status & PS2_FSTS_TXUF)
+			dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
+		if (fifo_status & PS2_FSTS_TXOF)
+			dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
+		if (fifo_status & PS2_FSTS_RXUF)
+			dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
+		if (fifo_status & PS2_FSTS_RXOF)
+			dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
+		/*reset PS/2 controller*/
+		writel(readl(drvdata->reg_base + PS2_REG_GCTL) | PS2_GCTL_RESET,
+			drvdata->reg_base + PS2_REG_GCTL);
+
+		rval = PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_TXRDY |
+			PS2_FSTS_RXUF | PS2_FSTS_RXOF | PS2_FSTS_RXRDY;
+		writel(rval, drvdata->reg_base + PS2_REG_FSTS);
+		error = 1;
+	}
+
+	rval = (fifo_status >> 16) & 0x3;
+	while (!error && rval--) {
+		byte = readl(drvdata->reg_base + PS2_REG_DATA) & 0xff;
+		serio_interrupt(drvdata->serio, byte, 0);
+	}
+
+	writel(intr_status, drvdata->reg_base + PS2_REG_LSTS);
+	writel(fifo_status, drvdata->reg_base + PS2_REG_FSTS);
+
+	spin_unlock(&drvdata->lock);
+
+	return IRQ_HANDLED;
+}
+
+
+static int sun4i_ps2_open(struct serio *pserio)
+{
+	struct sun4i_ps2data *drvdata = pserio->port_data;
+	u32 src_clk = 0;
+	u32 clk_scdf;
+	u32 clk_pcdf;
+	u32 rval;
+	unsigned long flags;
+
+	/*Set Line Control And Enable Interrupt*/
+	rval = PS2_LCTL_TXDTOEN | PS2_LCTL_STOPERREN | PS2_LCTL_ACKERREN
+		| PS2_LCTL_PARERREN | PS2_LCTL_RXDTOEN;
+	writel(rval, drvdata->reg_base + PS2_REG_LCTL);
+
+	/*Reset FIFO*/
+	rval = PS2_FCTL_TXRST | PS2_FCTL_RXRST | PS2_FCTL_TXUFIEN
+		| PS2_FCTL_TXOFIEN | PS2_FCTL_RXUFIEN
+		| PS2_FCTL_RXOFIEN | PS2_FCTL_RXRDYIEN;
+
+	writel(rval, drvdata->reg_base + PS2_REG_FCTL);
+
+	src_clk = clk_get_rate(drvdata->clk);
+	/*Set Clock Divider Register*/
+	clk_scdf = DIV_ROUND_UP(src_clk, PS2_SAMPLE_CLK) - 1;
+	clk_pcdf = DIV_ROUND_UP(PS2_SAMPLE_CLK, PS2_SCLK) - 1;
+	rval = (clk_scdf<<8) | clk_pcdf;
+	writel(rval, drvdata->reg_base + PS2_REG_CLKDR);
+
+
+	/*Set Global Control Register*/
+	rval = PS2_GCTL_RESET | PS2_GCTL_INTEN | PS2_GCTL_MASTER
+		| PS2_GCTL_BUSEN;
+
+	spin_lock_irqsave(&drvdata->lock, flags);
+	writel(rval, drvdata->reg_base + PS2_REG_GCTL);
+	spin_unlock_irqrestore(&drvdata->lock, flags);
+
+	return 0;
+}
+
+static void sun4i_ps2_close(struct serio *pserio)
+{
+	struct sun4i_ps2data *drvdata = pserio->port_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(&drvdata->lock, flags);
+	/* Disable the PS2 interrupts */
+	writel(0, drvdata->reg_base + PS2_REG_GCTL);
+	spin_unlock_irqrestore(&drvdata->lock, flags);
+}
+
+static int sun4i_ps2_write(struct serio *pserio, unsigned char val)
+{
+	unsigned long expire = jiffies + msecs_to_jiffies(10000);
+	struct sun4i_ps2data *drvdata;
+
+	drvdata = (struct sun4i_ps2data *)pserio->port_data;
+
+	do {
+		if (readl(drvdata->reg_base + PS2_REG_FSTS) & PS2_FSTS_TXRDY) {
+			writel(val, drvdata->reg_base + PS2_REG_DATA);
+			return 0;
+		}
+	} while (time_before(jiffies, expire));
+
+	return 0;
+}
+
+static int sun4i_ps2_probe(struct platform_device *pdev)
+{
+	struct resource *res; /* IO mem resources */
+	struct sun4i_ps2data *drvdata;
+	struct serio *serio;
+	struct device *dev = &pdev->dev;
+	unsigned int irq;
+	int error;
+
+	drvdata = devm_kzalloc(dev, sizeof(struct sun4i_ps2data), GFP_KERNEL);
+	serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
+	if (!drvdata || !serio) {
+		error = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	spin_lock_init(&drvdata->lock);
+
+	/* IO */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	drvdata->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(drvdata->reg_base)) {
+		dev_err(dev, "failed to map registers\n");
+		error = PTR_ERR(drvdata->reg_base);
+		goto err_free_mem;
+	}
+
+	drvdata->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(drvdata->clk)) {
+		error = PTR_ERR(drvdata->clk);
+		dev_err(dev, "couldn't get clock %d\n", error);
+		goto err_free_mem;
+	}
+
+	error = clk_prepare_enable(drvdata->clk);
+	if (error) {
+		dev_err(dev, "failed to enable clock %d\n", error);
+		goto err_free_mem;
+	}
+
+	serio->id.type = SERIO_8042;
+	serio->write = sun4i_ps2_write;
+	serio->open = sun4i_ps2_open;
+	serio->close = sun4i_ps2_close;
+	serio->port_data = drvdata;
+	serio->dev.parent = dev;
+	strlcpy(serio->name, dev_name(dev), sizeof(serio->name));
+	strlcpy(serio->phys, dev_name(dev), sizeof(serio->phys));
+
+	/* Get IRQ for the device */
+	irq = platform_get_irq(pdev, 0);
+	if (!irq) {
+		dev_err(dev, "no IRQ found\n");
+		error = -ENXIO;
+		goto error_disable_clk;
+	}
+
+	drvdata->irq = irq;
+	drvdata->serio = serio;
+	drvdata->dev = dev;
+	error = devm_request_threaded_irq(dev, drvdata->irq,
+		sun4i_ps2_interrupt, NULL, 0, DRIVER_NAME, drvdata);
+
+	if (error) {
+		dev_err(drvdata->dev, "Interrupt alloc failed %d:error:%d\n",
+			drvdata->irq, error);
+		goto error_disable_clk;
+	}
+
+	serio_register_port(serio);
+	platform_set_drvdata(pdev, drvdata);
+
+	return 0;	/* success */
+
+error_disable_clk:
+	clk_disable_unprepare(drvdata->clk);
+
+err_free_mem:
+	kfree(serio);
+	return error;
+}
+
+static int sun4i_ps2_remove(struct platform_device *pdev)
+{
+	struct sun4i_ps2data *drvdata = platform_get_drvdata(pdev);
+
+	serio_unregister_port(drvdata->serio);
+	disable_irq(drvdata->irq);
+
+	if (!IS_ERR(drvdata->clk))
+		clk_disable_unprepare(drvdata->clk);
+	kfree(drvdata->serio);
+
+	return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id sun4i_ps2_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ps2", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, sun4i_ps2_match);
+
+/*platform driver structure*/
+static struct platform_driver sun4i_ps2_driver = {
+	.probe		= sun4i_ps2_probe,
+	.remove		= sun4i_ps2_remove,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = sun4i_ps2_match,
+	},
+};
+module_platform_driver(sun4i_ps2_driver);
+
+MODULE_AUTHOR("Vishnu Patekar <vishnupatekar0510@gmail.com>");
+MODULE_AUTHOR("Aaron.maoye <leafy.myeh@newbietech.com>");
+MODULE_DESCRIPTION("Allwinner A10/Sun4i PS/2 driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCHv3 3/5] ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20

[VishnuPatekar]

1) Fixup the sun4i ps/2 nodes interrupt property, sun4i interrupts take
only 1 specifier

2) dt bindings should use the compat string for the earliest version of the
hardware which has the relevant hardware block, unless there are differences,
the A10 and A20 ps2 controllers are identical, so for both sun4i-a10-ps2
should be used as compat string, update the sun7i.dtsi ps2 entries to
use the sun4i-a10-ps2 compat string.



Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 arch/arm/boot/dts/sun4i-a10.dtsi |   17 +++++++++++++++++
 arch/arm/boot/dts/sun7i-a20.dtsi |   18 ++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 7b4099f..ef9a01c 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -629,6 +629,7 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
 		};
 
 		timer@01c20c00 {
@@ -795,5 +796,21 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
+
+		ps20: ps2@01c2a000 {
+			compatible = "allwinner,sun4i-a10-ps2";
+			reg = <0x01c2a000 0x400>;
+			interrupts = <62>;
+			clocks = <&apb1_gates 6>;
+			status = "disabled";
+		};
+
+		ps21: ps2@01c2a400 {
+			compatible = "allwinner,sun4i-a10-ps2";
+			reg = <0x01c2a400 0x400>;
+			interrupts = <63>;
+			clocks = <&apb1_gates 7>;
+			status = "disabled";
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index e21ce59..6ab7714 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -866,6 +866,7 @@
 				    allwinner,drive = <0>;
 				    allwinner,pull = <0>;
 			};
+
 		};
 
 		timer@01c20c00 {
@@ -1093,5 +1094,22 @@
 			#interrupt-cells = <3>;
 			interrupts = <1 9 0xf04>;
 		};
+
+		ps20: ps2@01c2a000 {
+			compatible = "allwinner,sun4i-a10-ps2";
+			reg = <0x01c2a000 0x400>;
+			interrupts = <0 62 4>;
+			clocks = <&apb1_gates 6>;
+			status = "disabled";
+		};
+
+		ps21: ps2@01c2a400 {
+			compatible = "allwinner,sun4i-a10-ps2";
+			reg = <0x01c2a400 0x400>;
+			interrupts = <0 63 4>;
+			clocks = <&apb1_gates 7>;
+			status = "disabled";
+		};
+
 	};
 };
-- 
1.7.9.5

[PATCHv3 4/5] ARM: sunxi: dts: Add A10/A20 PS2 pin muxing options

[VishnuPatekar]

Added the pinmuxing options for Allwinner A10 and A20.

Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
---
 arch/arm/boot/dts/sun4i-a10.dtsi |   14 ++++++++++++++
 arch/arm/boot/dts/sun7i-a20.dtsi |   14 ++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index ef9a01c..5a29039 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -630,6 +630,20 @@
 				allwinner,pull = <0>;
 			};
 
+			ps20_pins_a: ps20@0 {
+				    allwinner,pins = "PI20", "PI21";
+				    allwinner,function = "ps2";
+				    allwinner,drive = <0>;
+				    allwinner,pull = <0>;
+			};
+
+			ps21_pins_a: ps21@0 {
+				    allwinner,pins = "PH12", "PH13";
+				    allwinner,function = "ps2";
+				    allwinner,drive = <0>;
+				    allwinner,pull = <0>;
+			};
+
 		};
 
 		timer@01c20c00 {
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 6ab7714..247def0 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -867,6 +867,20 @@
 				    allwinner,pull = <0>;
 			};
 
+			ps20_pins_a: ps20@0 {
+				    allwinner,pins = "PI20", "PI21";
+				    allwinner,function = "ps2";
+				    allwinner,drive = <0>;
+				    allwinner,pull = <0>;
+			};
+
+			ps21_pins_a: ps21@0 {
+				    allwinner,pins = "PH12", "PH13";
+				    allwinner,function = "ps2";
+				    allwinner,drive = <0>;
+				    allwinner,pull = <0>;
+			};
+
 		};
 
 		timer@01c20c00 {
-- 
1.7.9.5

[PATCHv3 5/5] ARM: sunxi: dts: Add note ps2 pins conflict with hdmi

[VishnuPatekar]

1. Please note that ps20 pins conflict with HDMI on Lime2 Board
so, by deault ps20 and ps21 are disabled for Lime2 Board.
There is no on board ps2 connector and these pins can be used
for different purpose.

Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
---
 arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
index ed364d5..951b615 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
@@ -112,7 +112,11 @@
 			pinctrl-0 = <&uart0_pins_a>;
 			status = "okay";
 		};
-
+
+		/* PS2 0 and PS2 1 are disabled by default; Please note that
+		ps20 pins conflict with HDMI on Lime2 Board
+		*/
+
 		i2c0: i2c@01c2ac00 {
 			pinctrl-names = "default";
 			pinctrl-0 = <&i2c0_pins_a>;
-- 
1.7.9.5

Re: [PATCHv3 5/5] ARM: sunxi: dts: Add note ps2 pins conflict with hdmi

[Chen-Yu Tsai]

Hi,

On Sat, Dec 13, 2014 at 2:25 AM, VishnuPatekar
<vishnupatekar0510@gmail.com> wrote:
> 1. Please note that ps20 pins conflict with HDMI on Lime2 Board
> so, by deault ps20 and ps21 are disabled for Lime2 Board.
> There is no on board ps2 connector and these pins can be used
> for different purpose.
>
> Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
> ---
>  arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
> index ed364d5..951b615 100644
> --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
> @@ -112,7 +112,11 @@
>                         pinctrl-0 = <&uart0_pins_a>;
>                         status = "okay";
>                 };
> -
> +
> +               /* PS2 0 and PS2 1 are disabled by default; Please note that
> +               ps20 pins conflict with HDMI on Lime2 Board
> +               */
> +

Multi-line comments should be:

/*
 * line 1
 * line 2
 * ...
 */

And why is there a delete and insert for an empty line?
Weird... though git seems to ignore it when applying.

ChenYu

>                 i2c0: i2c@01c2ac00 {
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&i2c0_pins_a>;
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Hans de Goede]

Hi VishnuPatekar,

The patch mangling for this set seems to have gone a bit wrong I'm afraid,
a lot of the patches have my fixup commit messages (which should have
disappeared when squashing in the patches), please replace those by proper
commit messages describing what the patch actually does.

Also the adding of the commented nodes for the lime2 seems to be gone entirely
from the set, instead now only a comment about the conflict with the hdmi
pins is added, but it is above the i2c node instead of above a ps2 node.

Regards,

Hans


On 12-12-14 19:25, VishnuPatekar wrote:
> v2 --> v3
> 1. changed config to SERIO_SUN4I_PS2 from SERIO_SUNXI_PS2
> 2. changed driver name to sun4i-ps2 from sunxi-ps2.
> 3. changed the function names to sun4i_ps2_*.
> 4. added locking in sun4i_ps2_open.
> 5. kept compatible "sun4i-a10-ps2" for A10 and A20, as A10 is earlier SOC.
> 6. corrected the style errors.
> 7. separated the dts patches.
> 8. removed commented ps2 notes from lime2 dts.
> 9. added note that ps2 pins confilt with hdmi.
> 10. corrected the interrupt property for A10.
> 11. moved dt-bindings to Documentation/devicetree/bindings/serio
>
> v1 --> v2:
> 1. added default n depends on ARCH_SUNXI || COMPILE_TEST in Kconfig.
> 2. handled errors and free resources on errors.
> 3. used BIT(x), DIV_ROUND_UP macros.
> 4. corrected style errors.
> 5. added support for A10 also, A10 and A2 have same properties of PS2 controller.
> 6. by default commented ps20 and ps21 nodes,as ps20 pins conflict with HDMI.
> 7. added compatible as allwinner,sun4i-a10-ps2.
> 8. corrected the possible race condition.
>
>
> VishnuPatekar (5):
>    sunxi:dts-bindings:input:ps2 bindings for A10/A20 ps2.
>    ARM:sunxi:drivers:input Add support for A10/A20 PS2
>    ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20
>    ARM: sunxi: dts: Add A10/A20 PS2 pin muxing options
>    ARM: sunxi: dts: Add note ps2 pins conflict with hdmi
>
>   .../bindings/serio/allwinner,sun4i-ps2.txt         |   23 ++
>   arch/arm/boot/dts/sun4i-a10.dtsi                   |   31 ++
>   arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts    |    6 +-
>   arch/arm/boot/dts/sun7i-a20.dtsi                   |   32 ++
>   drivers/input/serio/Kconfig                        |   11 +
>   drivers/input/serio/Makefile                       |    1 +
>   drivers/input/serio/sun4i-ps2.c                    |  362 ++++++++++++++++++++
>   7 files changed, 465 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
>   create mode 100644 drivers/input/serio/sun4i-ps2.c
>

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Vishnu Patekar]

Hello Hans,
Please find my comments inlined.

On 12/13/14, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi VishnuPatekar,
>
> The patch mangling for this set seems to have gone a bit wrong I'm afraid

No, this time I've corrected it. Infact, last version of patch did not
used the status bit error macros.

> a lot of the patches have my fixup commit messages (which should have
> disappeared when squashing in the patches), please replace those by proper
> commit messages describing what the patch actually does.
Yes, [PATCHv3 3/5] uses fixup. I'll correct it.
>
> Also the adding of the commented nodes for the lime2 seems to be gone
> entirely
> from the set, instead now only a comment about the conflict with the hdmi
> pins is added, but it is above the i2c node instead of above a ps2 node.
>
Maxime's suggested that we should not add commented nodes specially
when its trivial to apply. And just note saying ps20 pins conflict
with HDMI.

Should I remove the this comment as well?
Or
Put this note in start of DTS file?
> Regards,
>
> Hans
>
>
> On 12-12-14 19:25, VishnuPatekar wrote:
>> v2 --> v3
>> 1. changed config to SERIO_SUN4I_PS2 from SERIO_SUNXI_PS2
>> 2. changed driver name to sun4i-ps2 from sunxi-ps2.
>> 3. changed the function names to sun4i_ps2_*.
>> 4. added locking in sun4i_ps2_open.
>> 5. kept compatible "sun4i-a10-ps2" for A10 and A20, as A10 is earlier
>> SOC.
>> 6. corrected the style errors.
>> 7. separated the dts patches.
>> 8. removed commented ps2 notes from lime2 dts.
>> 9. added note that ps2 pins confilt with hdmi.
>> 10. corrected the interrupt property for A10.
>> 11. moved dt-bindings to Documentation/devicetree/bindings/serio
>>
>> v1 --> v2:
>> 1. added default n depends on ARCH_SUNXI || COMPILE_TEST in Kconfig.
>> 2. handled errors and free resources on errors.
>> 3. used BIT(x), DIV_ROUND_UP macros.
>> 4. corrected style errors.
>> 5. added support for A10 also, A10 and A2 have same properties of PS2
>> controller.
>> 6. by default commented ps20 and ps21 nodes,as ps20 pins conflict with
>> HDMI.
>> 7. added compatible as allwinner,sun4i-a10-ps2.
>> 8. corrected the possible race condition.
>>
>>
>> VishnuPatekar (5):
>>    sunxi:dts-bindings:input:ps2 bindings for A10/A20 ps2.
>>    ARM:sunxi:drivers:input Add support for A10/A20 PS2
>>    ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20
>>    ARM: sunxi: dts: Add A10/A20 PS2 pin muxing options
>>    ARM: sunxi: dts: Add note ps2 pins conflict with hdmi
>>
>>   .../bindings/serio/allwinner,sun4i-ps2.txt         |   23 ++
>>   arch/arm/boot/dts/sun4i-a10.dtsi                   |   31 ++
>>   arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts    |    6 +-
>>   arch/arm/boot/dts/sun7i-a20.dtsi                   |   32 ++
>>   drivers/input/serio/Kconfig                        |   11 +
>>   drivers/input/serio/Makefile                       |    1 +
>>   drivers/input/serio/sun4i-ps2.c                    |  362
>> ++++++++++++++++++++
>>   7 files changed, 465 insertions(+), 1 deletion(-)
>>   create mode 100644
>> Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
>>   create mode 100644 drivers/input/serio/sun4i-ps2.c
>>
>

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Hans de Goede]

Hi,

On 13-12-14 21:01, Vishnu Patekar wrote:
> Hello Hans,
> Please find my comments inlined.
>
> On 12/13/14, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi VishnuPatekar,
>>
>> The patch mangling for this set seems to have gone a bit wrong I'm afraid
>
> No, this time I've corrected it. Infact, last version of patch did not
> used the status bit error macros.
>
>> a lot of the patches have my fixup commit messages (which should have
>> disappeared when squashing in the patches), please replace those by proper
>> commit messages describing what the patch actually does.
> Yes, [PATCHv3 3/5] uses fixup. I'll correct it.
>>
>> Also the adding of the commented nodes for the lime2 seems to be gone
>> entirely
>> from the set, instead now only a comment about the conflict with the hdmi
>> pins is added, but it is above the i2c node instead of above a ps2 node.
>>
> Maxime's suggested that we should not add commented nodes specially
> when its trivial to apply. And just note saying ps20 pins conflict
> with HDMI.

Ah, I see.

> Should I remove the this comment as well?
> Or
> Put this note in start of DTS file?

The comment should be added to where the pinctrl bits are, not to the lime2
file, ps0 will conflict with hdmi on all boards.

Regards,

Hans


>> Regards,
>>
>> Hans
>>
>>
>> On 12-12-14 19:25, VishnuPatekar wrote:
>>> v2 --> v3
>>> 1. changed config to SERIO_SUN4I_PS2 from SERIO_SUNXI_PS2
>>> 2. changed driver name to sun4i-ps2 from sunxi-ps2.
>>> 3. changed the function names to sun4i_ps2_*.
>>> 4. added locking in sun4i_ps2_open.
>>> 5. kept compatible "sun4i-a10-ps2" for A10 and A20, as A10 is earlier
>>> SOC.
>>> 6. corrected the style errors.
>>> 7. separated the dts patches.
>>> 8. removed commented ps2 notes from lime2 dts.
>>> 9. added note that ps2 pins confilt with hdmi.
>>> 10. corrected the interrupt property for A10.
>>> 11. moved dt-bindings to Documentation/devicetree/bindings/serio
>>>
>>> v1 --> v2:
>>> 1. added default n depends on ARCH_SUNXI || COMPILE_TEST in Kconfig.
>>> 2. handled errors and free resources on errors.
>>> 3. used BIT(x), DIV_ROUND_UP macros.
>>> 4. corrected style errors.
>>> 5. added support for A10 also, A10 and A2 have same properties of PS2
>>> controller.
>>> 6. by default commented ps20 and ps21 nodes,as ps20 pins conflict with
>>> HDMI.
>>> 7. added compatible as allwinner,sun4i-a10-ps2.
>>> 8. corrected the possible race condition.
>>>
>>>
>>> VishnuPatekar (5):
>>>     sunxi:dts-bindings:input:ps2 bindings for A10/A20 ps2.
>>>     ARM:sunxi:drivers:input Add support for A10/A20 PS2
>>>     ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20
>>>     ARM: sunxi: dts: Add A10/A20 PS2 pin muxing options
>>>     ARM: sunxi: dts: Add note ps2 pins conflict with hdmi
>>>
>>>    .../bindings/serio/allwinner,sun4i-ps2.txt         |   23 ++
>>>    arch/arm/boot/dts/sun4i-a10.dtsi                   |   31 ++
>>>    arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts    |    6 +-
>>>    arch/arm/boot/dts/sun7i-a20.dtsi                   |   32 ++
>>>    drivers/input/serio/Kconfig                        |   11 +
>>>    drivers/input/serio/Makefile                       |    1 +
>>>    drivers/input/serio/sun4i-ps2.c                    |  362
>>> ++++++++++++++++++++
>>>    7 files changed, 465 insertions(+), 1 deletion(-)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
>>>    create mode 100644 drivers/input/serio/sun4i-ps2.c
>>>
>>

Re: [PATCHv3 2/5] ARM:sunxi:drivers:input Add support for A10/A20 PS2

[Dmitry Torokhov]

Hi Vishnu,

On Fri, Dec 12, 2014 at 11:55:45PM +0530, VishnuPatekar wrote:
> 
> Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
> ---
>  drivers/input/serio/Kconfig     |   11 ++
>  drivers/input/serio/Makefile    |    1 +
>  drivers/input/serio/sun4i-ps2.c |  362 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 374 insertions(+)
>  create mode 100644 drivers/input/serio/sun4i-ps2.c
> 
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index bc2d474..964afc5 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -281,4 +281,15 @@ config HYPERV_KEYBOARD
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called hyperv_keyboard.
>  
> +config SERIO_SUN4I_PS2
> +	tristate "Allwinner A10 PS/2 controller support"
> +	default n
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	help
> +	  This selects support for the PS/2 Host Controller on
> +	  Allwinner A10.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sun4i-ps2.
> +
>  endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 815d874..c600089 100644
> --- a/drivers/input/serio/Makefile
> +++ b/drivers/input/serio/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_SERIO_ARC_PS2)	+= arc_ps2.o
>  obj-$(CONFIG_SERIO_APBPS2)	+= apbps2.o
>  obj-$(CONFIG_SERIO_OLPC_APSP)	+= olpc_apsp.o
>  obj-$(CONFIG_HYPERV_KEYBOARD)	+= hyperv-keyboard.o
> +obj-$(CONFIG_SERIO_SUN4I_PS2)	+= sun4i-ps2.o
> diff --git a/drivers/input/serio/sun4i-ps2.c b/drivers/input/serio/sun4i-ps2.c
> new file mode 100644
> index 0000000..e6d22ae
> --- /dev/null
> +++ b/drivers/input/serio/sun4i-ps2.c
> @@ -0,0 +1,362 @@
> +/*
> + *	Driver for Allwinner A10 PS2 host controller
> + *
> + *	Author: Vishnu Patekar <vishnupatekar0510@gmail.com>
> + *		Aaron.maoye <leafy.myeh@newbietech.com>
> + *
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/serio.h>
> +#include <linux/interrupt.h>
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +
> +#define DRIVER_NAME		"sun4i-ps2"
> +
> +/* register offset definitions */
> +#define PS2_REG_GCTL		(0x00)	/*  PS2 Module Global Control Reg */
> +#define PS2_REG_DATA		(0x04)  /*  PS2 Module Data Reg		*/
> +#define PS2_REG_LCTL		(0x08)	/*  PS2 Module Line Control Reg */
> +#define PS2_REG_LSTS		(0x0C)  /*  PS2 Module Line Status Reg	*/
> +#define PS2_REG_FCTL		(0x10)	/*  PS2 Module FIFO Control Reg */
> +#define PS2_REG_FSTS		(0x14)	/*  PS2 Module FIFO Status Reg	*/
> +#define PS2_REG_CLKDR		(0x18)	/*  PS2 Module Clock Divider Reg*/

You do not need parenthesis around simple constants.

> +
> +/*  PS2 GLOBAL CONTROL REGISTER PS2_GCTL */
> +#define PS2_GCTL_INTFLAG	BIT(4)
> +#define PS2_GCTL_INTEN		BIT(3)
> +#define PS2_GCTL_RESET		BIT(2)
> +#define PS2_GCTL_MASTER		BIT(1)
> +#define PS2_GCTL_BUSEN		BIT(0)
> +
> +/* PS2 LINE CONTROL REGISTER */
> +#define PS2_LCTL_NOACK		BIT(18)
> +#define PS2_LCTL_TXDTOEN	BIT(8)
> +#define PS2_LCTL_STOPERREN	BIT(3)
> +#define PS2_LCTL_ACKERREN	BIT(2)
> +#define PS2_LCTL_PARERREN	BIT(1)
> +#define PS2_LCTL_RXDTOEN	BIT(0)
> +
> +/* PS2 LINE STATUS REGISTER */
> +#define PS2_LSTS_TXTDO		BIT(8)
> +#define PS2_LSTS_STOPERR	BIT(3)
> +#define PS2_LSTS_ACKERR		BIT(2)
> +#define PS2_LSTS_PARERR		BIT(1)
> +#define PS2_LSTS_RXTDO		BIT(0)
> +
> +#define PS2_LINE_ERROR_BIT \
> +	(PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR | \
> +	PS2_LSTS_PARERR | PS2_LSTS_RXTDO)
> +
> +/* PS2 FIFO CONTROL REGISTER */
> +#define PS2_FCTL_TXRST		BIT(17)
> +#define PS2_FCTL_RXRST		BIT(16)
> +#define PS2_FCTL_TXUFIEN	BIT(10)
> +#define PS2_FCTL_TXOFIEN	BIT(9)
> +#define PS2_FCTL_TXRDYIEN	BIT(8)
> +#define PS2_FCTL_RXUFIEN	BIT(2)
> +#define PS2_FCTL_RXOFIEN	BIT(1)
> +#define PS2_FCTL_RXRDYIEN	BIT(0)
> +
> +/* PS2 FIFO STATUS REGISTER */
> +#define PS2_FSTS_TXUF		BIT(10)
> +#define PS2_FSTS_TXOF		BIT(9)
> +#define PS2_FSTS_TXRDY		BIT(8)
> +#define PS2_FSTS_RXUF		BIT(2)
> +#define PS2_FSTS_RXOF		BIT(1)
> +#define PS2_FSTS_RXRDY		BIT(0)
> +
> +#define PS2_FIFO_ERROR_BIT \
> +	(PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_RXUF | PS2_FSTS_RXOF)
> +
> +#define PS2_SAMPLE_CLK		(1000000)
> +#define PS2_SCLK		(125000)
> +
> +struct sun4i_ps2data {
> +	struct serio *serio;
> +	struct device *dev;
> +
> +	/* IO mapping base */
> +	void __iomem	*reg_base;
> +
> +	/* clock management */
> +	struct clk	*clk;
> +
> +	/* irq */
> +	spinlock_t	lock;
> +	int		irq;
> +};
> +
> +/*********************/
> +/* Interrupt handler */
> +/*********************/
> +static irqreturn_t sun4i_ps2_interrupt(int irq, void *dev_id)
> +{
> +	struct sun4i_ps2data *drvdata = dev_id;
> +	u32 intr_status;
> +	u32 fifo_status;
> +	unsigned char byte;
> +	u32 rval;
> +	u32 error = 0;
> +
> +	spin_lock(&drvdata->lock);
> +
> +	/* Get the PS/2 interrupts and clear them */
> +	intr_status  = readl(drvdata->reg_base + PS2_REG_LSTS);
> +	fifo_status  = readl(drvdata->reg_base + PS2_REG_FSTS);
> +
> +	/*Check Line Status Register*/
> +	if (intr_status & PS2_LINE_ERROR_BIT) {
> +		if (intr_status & PS2_LSTS_STOPERR)
> +			dev_info(drvdata->dev, "PS/2 Stop Bit Error!");

The stop bit error is I believe should be reported to consumers as
SSERIO_FRAME, receive timeout as SERIO_TIMEOUT and parity as
SERIO_PARITY.

> +		if (intr_status & PS2_LSTS_ACKERR)
> +			dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");

When is this error signalled?

> +		if (intr_status & PS2_LSTS_PARERR)
> +			dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> +		if (intr_status & PS2_LSTS_TXTDO)
> +			dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");

Should not you check this status in serio_write() instead of here?

> +		if (intr_status & PS2_LSTS_RXTDO)
> +			dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
> +
> +		/*reset PS/2 controller*/
> +		rval = readl(drvdata->reg_base + PS2_REG_GCTL);
> +		writel(rval | PS2_GCTL_RESET, drvdata->reg_base + PS2_REG_GCTL);

Why do we need to reset controller in case of, let's say, parity glitch?

> +
> +		rval = PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR |
> +			PS2_LSTS_PARERR | PS2_LSTS_RXTDO;
> +		writel(rval, drvdata->reg_base + PS2_REG_LSTS);
> +		error = 1;
> +	}
> +
> +	/*Check FIFO Status Register*/
> +	if (fifo_status & PS2_FIFO_ERROR_BIT) {
> +		if (fifo_status & PS2_FSTS_TXUF)
> +			dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
> +		if (fifo_status & PS2_FSTS_TXOF)
> +			dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> +		if (fifo_status & PS2_FSTS_RXUF)
> +			dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
> +		if (fifo_status & PS2_FSTS_RXOF)
> +			dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> +		/*reset PS/2 controller*/
> +		writel(readl(drvdata->reg_base + PS2_REG_GCTL) | PS2_GCTL_RESET,
> +			drvdata->reg_base + PS2_REG_GCTL);

Again, we do we need to reset controller? Does it stop receiving data
after RX overflow condition?

> +
> +		rval = PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_TXRDY |
> +			PS2_FSTS_RXUF | PS2_FSTS_RXOF | PS2_FSTS_RXRDY;
> +		writel(rval, drvdata->reg_base + PS2_REG_FSTS);
> +		error = 1;
> +	}
> +
> +	rval = (fifo_status >> 16) & 0x3;
> +	while (!error && rval--) {
> +		byte = readl(drvdata->reg_base + PS2_REG_DATA) & 0xff;
> +		serio_interrupt(drvdata->serio, byte, 0);
> +	}
> +
> +	writel(intr_status, drvdata->reg_base + PS2_REG_LSTS);
> +	writel(fifo_status, drvdata->reg_base + PS2_REG_FSTS);
> +
> +	spin_unlock(&drvdata->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +
> +static int sun4i_ps2_open(struct serio *pserio)
> +{
> +	struct sun4i_ps2data *drvdata = pserio->port_data;
> +	u32 src_clk = 0;
> +	u32 clk_scdf;
> +	u32 clk_pcdf;
> +	u32 rval;
> +	unsigned long flags;
> +
> +	/*Set Line Control And Enable Interrupt*/
> +	rval = PS2_LCTL_TXDTOEN | PS2_LCTL_STOPERREN | PS2_LCTL_ACKERREN
> +		| PS2_LCTL_PARERREN | PS2_LCTL_RXDTOEN;
> +	writel(rval, drvdata->reg_base + PS2_REG_LCTL);
> +
> +	/*Reset FIFO*/
> +	rval = PS2_FCTL_TXRST | PS2_FCTL_RXRST | PS2_FCTL_TXUFIEN
> +		| PS2_FCTL_TXOFIEN | PS2_FCTL_RXUFIEN
> +		| PS2_FCTL_RXOFIEN | PS2_FCTL_RXRDYIEN;
> +
> +	writel(rval, drvdata->reg_base + PS2_REG_FCTL);
> +
> +	src_clk = clk_get_rate(drvdata->clk);
> +	/*Set Clock Divider Register*/
> +	clk_scdf = DIV_ROUND_UP(src_clk, PS2_SAMPLE_CLK) - 1;
> +	clk_pcdf = DIV_ROUND_UP(PS2_SAMPLE_CLK, PS2_SCLK) - 1;
> +	rval = (clk_scdf<<8) | clk_pcdf;
> +	writel(rval, drvdata->reg_base + PS2_REG_CLKDR);
> +
> +
> +	/*Set Global Control Register*/
> +	rval = PS2_GCTL_RESET | PS2_GCTL_INTEN | PS2_GCTL_MASTER
> +		| PS2_GCTL_BUSEN;
> +
> +	spin_lock_irqsave(&drvdata->lock, flags);
> +	writel(rval, drvdata->reg_base + PS2_REG_GCTL);
> +	spin_unlock_irqrestore(&drvdata->lock, flags);
> +
> +	return 0;
> +}
> +
> +static void sun4i_ps2_close(struct serio *pserio)
> +{
> +	struct sun4i_ps2data *drvdata = pserio->port_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&drvdata->lock, flags);
> +	/* Disable the PS2 interrupts */
> +	writel(0, drvdata->reg_base + PS2_REG_GCTL);
> +	spin_unlock_irqrestore(&drvdata->lock, flags);


I think you want to add synchronize_irq() here to make sure pending
interrupt handler runs to completion before you return.

> +}
> +
> +static int sun4i_ps2_write(struct serio *pserio, unsigned char val)
> +{
> +	unsigned long expire = jiffies + msecs_to_jiffies(10000);
> +	struct sun4i_ps2data *drvdata;
> +
> +	drvdata = (struct sun4i_ps2data *)pserio->port_data;
> +
> +	do {
> +		if (readl(drvdata->reg_base + PS2_REG_FSTS) & PS2_FSTS_TXRDY) {
> +			writel(val, drvdata->reg_base + PS2_REG_DATA);
> +			return 0;
> +		}
> +	} while (time_before(jiffies, expire));
> +
> +	return 0;
> +}
> +
> +static int sun4i_ps2_probe(struct platform_device *pdev)
> +{
> +	struct resource *res; /* IO mem resources */
> +	struct sun4i_ps2data *drvdata;
> +	struct serio *serio;
> +	struct device *dev = &pdev->dev;
> +	unsigned int irq;
> +	int error;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(struct sun4i_ps2data), GFP_KERNEL);
> +	serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +	if (!drvdata || !serio) {
> +		error = -ENOMEM;
> +		goto err_free_mem;
> +	}
> +
> +	spin_lock_init(&drvdata->lock);
> +
> +	/* IO */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	drvdata->reg_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(drvdata->reg_base)) {
> +		dev_err(dev, "failed to map registers\n");
> +		error = PTR_ERR(drvdata->reg_base);
> +		goto err_free_mem;
> +	}
> +
> +	drvdata->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(drvdata->clk)) {
> +		error = PTR_ERR(drvdata->clk);
> +		dev_err(dev, "couldn't get clock %d\n", error);
> +		goto err_free_mem;
> +	}
> +
> +	error = clk_prepare_enable(drvdata->clk);
> +	if (error) {
> +		dev_err(dev, "failed to enable clock %d\n", error);
> +		goto err_free_mem;
> +	}
> +

I think you should explicitly prohibit the device generating interrupts
by writing to PS2_ERG_CTRL here.

> +	serio->id.type = SERIO_8042;
> +	serio->write = sun4i_ps2_write;
> +	serio->open = sun4i_ps2_open;
> +	serio->close = sun4i_ps2_close;
> +	serio->port_data = drvdata;
> +	serio->dev.parent = dev;
> +	strlcpy(serio->name, dev_name(dev), sizeof(serio->name));
> +	strlcpy(serio->phys, dev_name(dev), sizeof(serio->phys));
> +
> +	/* Get IRQ for the device */
> +	irq = platform_get_irq(pdev, 0);
> +	if (!irq) {
> +		dev_err(dev, "no IRQ found\n");
> +		error = -ENXIO;
> +		goto error_disable_clk;
> +	}
> +
> +	drvdata->irq = irq;
> +	drvdata->serio = serio;
> +	drvdata->dev = dev;
> +	error = devm_request_threaded_irq(dev, drvdata->irq,
> +		sun4i_ps2_interrupt, NULL, 0, DRIVER_NAME, drvdata);
> +
> +	if (error) {
> +		dev_err(drvdata->dev, "Interrupt alloc failed %d:error:%d\n",
> +			drvdata->irq, error);
> +		goto error_disable_clk;
> +	}
> +
> +	serio_register_port(serio);
> +	platform_set_drvdata(pdev, drvdata);
> +
> +	return 0;	/* success */
> +
> +error_disable_clk:
> +	clk_disable_unprepare(drvdata->clk);
> +
> +err_free_mem:
> +	kfree(serio);
> +	return error;
> +}
> +
> +static int sun4i_ps2_remove(struct platform_device *pdev)
> +{
> +	struct sun4i_ps2data *drvdata = platform_get_drvdata(pdev);
> +
> +	serio_unregister_port(drvdata->serio);
> +	disable_irq(drvdata->irq);

I do not think you need to disable irq here - serio_close will make sure
that the device does not generate interrupts.

> +
> +	if (!IS_ERR(drvdata->clk))
> +		clk_disable_unprepare(drvdata->clk);
> +	kfree(drvdata->serio);
> +
> +	return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id sun4i_ps2_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-ps2", },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sun4i_ps2_match);
> +
> +/*platform driver structure*/

I do not think we should keep the obvious comments like this one.

> +static struct platform_driver sun4i_ps2_driver = {
> +	.probe		= sun4i_ps2_probe,
> +	.remove		= sun4i_ps2_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = sun4i_ps2_match,
> +	},
> +};
> +module_platform_driver(sun4i_ps2_driver);
> +
> +MODULE_AUTHOR("Vishnu Patekar <vishnupatekar0510@gmail.com>");
> +MODULE_AUTHOR("Aaron.maoye <leafy.myeh@newbietech.com>");
> +MODULE_DESCRIPTION("Allwinner A10/Sun4i PS/2 driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.9.5
> 

Thanks.

-- 
Dmitry

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Vishnu Patekar]

Hi,

On Sun, Dec 14, 2014 at 2:31 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 13-12-14 21:01, Vishnu Patekar wrote:
>>
>> Hello Hans,
>> Please find my comments inlined.
>>
>> On 12/13/14, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi VishnuPatekar,
>>>
>>> The patch mangling for this set seems to have gone a bit wrong I'm afraid
>>
>>
>> No, this time I've corrected it. Infact, last version of patch did not
>> used the status bit error macros.
>>
>>> a lot of the patches have my fixup commit messages (which should have
>>> disappeared when squashing in the patches), please replace those by
>>> proper
>>> commit messages describing what the patch actually does.
>>
>> Yes, [PATCHv3 3/5] uses fixup. I'll correct it.
>>>
>>>
>>> Also the adding of the commented nodes for the lime2 seems to be gone
>>> entirely
>>> from the set, instead now only a comment about the conflict with the hdmi
>>> pins is added, but it is above the i2c node instead of above a ps2 node.
>>>
>> Maxime's suggested that we should not add commented nodes specially
>> when its trivial to apply. And just note saying ps20 pins conflict
>> with HDMI.
>
>
> Ah, I see.
>
>> Should I remove the this comment as well?
>> Or
>> Put this note in start of DTS file?
>
>
> The comment should be added to where the pinctrl bits are, not to the lime2
> file, ps0 will conflict with hdmi on all boards.

Well, In that case,PS2_0 pins conflicts with HDMI, PS2_1 pins
conflicts with LCD. Every pin has multiplexing.

Do we really need to add this comment?
Can't we just  keep PS2 pins disabled, without any comment in dtsi or dts file?

Whoever wants, can enable PS2 pins in respective board dts file by
taking care that it does not conflict with any other peripheral.

>
> Regards,
>
> Hans
>
>
>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>> On 12-12-14 19:25, VishnuPatekar wrote:
>>>>
>>>> v2 --> v3
>>>> 1. changed config to SERIO_SUN4I_PS2 from SERIO_SUNXI_PS2
>>>> 2. changed driver name to sun4i-ps2 from sunxi-ps2.
>>>> 3. changed the function names to sun4i_ps2_*.
>>>> 4. added locking in sun4i_ps2_open.
>>>> 5. kept compatible "sun4i-a10-ps2" for A10 and A20, as A10 is earlier
>>>> SOC.
>>>> 6. corrected the style errors.
>>>> 7. separated the dts patches.
>>>> 8. removed commented ps2 notes from lime2 dts.
>>>> 9. added note that ps2 pins confilt with hdmi.
>>>> 10. corrected the interrupt property for A10.
>>>> 11. moved dt-bindings to Documentation/devicetree/bindings/serio
>>>>
>>>> v1 --> v2:
>>>> 1. added default n depends on ARCH_SUNXI || COMPILE_TEST in Kconfig.
>>>> 2. handled errors and free resources on errors.
>>>> 3. used BIT(x), DIV_ROUND_UP macros.
>>>> 4. corrected style errors.
>>>> 5. added support for A10 also, A10 and A2 have same properties of PS2
>>>> controller.
>>>> 6. by default commented ps20 and ps21 nodes,as ps20 pins conflict with
>>>> HDMI.
>>>> 7. added compatible as allwinner,sun4i-a10-ps2.
>>>> 8. corrected the possible race condition.
>>>>
>>>>
>>>> VishnuPatekar (5):
>>>>     sunxi:dts-bindings:input:ps2 bindings for A10/A20 ps2.
>>>>     ARM:sunxi:drivers:input Add support for A10/A20 PS2
>>>>     ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20
>>>>     ARM: sunxi: dts: Add A10/A20 PS2 pin muxing options
>>>>     ARM: sunxi: dts: Add note ps2 pins conflict with hdmi
>>>>
>>>>    .../bindings/serio/allwinner,sun4i-ps2.txt         |   23 ++
>>>>    arch/arm/boot/dts/sun4i-a10.dtsi                   |   31 ++
>>>>    arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts    |    6 +-
>>>>    arch/arm/boot/dts/sun7i-a20.dtsi                   |   32 ++
>>>>    drivers/input/serio/Kconfig                        |   11 +
>>>>    drivers/input/serio/Makefile                       |    1 +
>>>>    drivers/input/serio/sun4i-ps2.c                    |  362
>>>> ++++++++++++++++++++
>>>>    7 files changed, 465 insertions(+), 1 deletion(-)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/serio/allwinner,sun4i-ps2.txt
>>>>    create mode 100644 drivers/input/serio/sun4i-ps2.c
>>>>
>>>
>

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Hans de Goede]

Hi,

On 15-12-14 15:13, Vishnu Patekar wrote:
> Hi,
>
> On Sun, Dec 14, 2014 at 2:31 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 13-12-14 21:01, Vishnu Patekar wrote:
>>>
>>> Hello Hans,
>>> Please find my comments inlined.
>>>
>>> On 12/13/14, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi VishnuPatekar,
>>>>
>>>> The patch mangling for this set seems to have gone a bit wrong I'm afraid
>>>
>>>
>>> No, this time I've corrected it. Infact, last version of patch did not
>>> used the status bit error macros.
>>>
>>>> a lot of the patches have my fixup commit messages (which should have
>>>> disappeared when squashing in the patches), please replace those by
>>>> proper
>>>> commit messages describing what the patch actually does.
>>>
>>> Yes, [PATCHv3 3/5] uses fixup. I'll correct it.
>>>>
>>>>
>>>> Also the adding of the commented nodes for the lime2 seems to be gone
>>>> entirely
>>>> from the set, instead now only a comment about the conflict with the hdmi
>>>> pins is added, but it is above the i2c node instead of above a ps2 node.
>>>>
>>> Maxime's suggested that we should not add commented nodes specially
>>> when its trivial to apply. And just note saying ps20 pins conflict
>>> with HDMI.
>>
>>
>> Ah, I see.
>>
>>> Should I remove the this comment as well?
>>> Or
>>> Put this note in start of DTS file?
>>
>>
>> The comment should be added to where the pinctrl bits are, not to the lime2
>> file, ps0 will conflict with hdmi on all boards.
>
> Well, In that case,PS2_0 pins conflicts with HDMI, PS2_1 pins
> conflicts with LCD. Every pin has multiplexing.

Almost every A10 board has a hdmi connector, and only the hdmi ddc pins
are multiplexed, so this is sort of special, but yes if we do not have
the commented nodes in the dts file, then this comment can be dropped too
I guess.

Regards,

Hans

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Chen-Yu Tsai]

Hi,

On Mon, Dec 15, 2014 at 11:13 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 15-12-14 15:13, Vishnu Patekar wrote:
>>
>> Hi,
>>
>> On Sun, Dec 14, 2014 at 2:31 PM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 13-12-14 21:01, Vishnu Patekar wrote:
>>>>
>>>>
>>>> Hello Hans,
>>>> Please find my comments inlined.
>>>>
>>>> On 12/13/14, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>>
>>>>> Hi VishnuPatekar,
>>>>>
>>>>> The patch mangling for this set seems to have gone a bit wrong I'm
>>>>> afraid
>>>>
>>>>
>>>>
>>>> No, this time I've corrected it. Infact, last version of patch did not
>>>> used the status bit error macros.
>>>>
>>>>> a lot of the patches have my fixup commit messages (which should have
>>>>> disappeared when squashing in the patches), please replace those by
>>>>> proper
>>>>> commit messages describing what the patch actually does.
>>>>
>>>>
>>>> Yes, [PATCHv3 3/5] uses fixup. I'll correct it.
>>>>>
>>>>>
>>>>>
>>>>> Also the adding of the commented nodes for the lime2 seems to be gone
>>>>> entirely
>>>>> from the set, instead now only a comment about the conflict with the
>>>>> hdmi
>>>>> pins is added, but it is above the i2c node instead of above a ps2
>>>>> node.
>>>>>
>>>> Maxime's suggested that we should not add commented nodes specially
>>>> when its trivial to apply. And just note saying ps20 pins conflict
>>>> with HDMI.
>>>
>>>
>>>
>>> Ah, I see.
>>>
>>>> Should I remove the this comment as well?
>>>> Or
>>>> Put this note in start of DTS file?
>>>
>>>
>>>
>>> The comment should be added to where the pinctrl bits are, not to the
>>> lime2
>>> file, ps0 will conflict with hdmi on all boards.
>>
>>
>> Well, In that case,PS2_0 pins conflicts with HDMI, PS2_1 pins
>> conflicts with LCD. Every pin has multiplexing.
>
>
> Almost every A10 board has a hdmi connector, and only the hdmi ddc pins
> are multiplexed, so this is sort of special, but yes if we do not have
> the commented nodes in the dts file, then this comment can be dropped too
> I guess.

All the HDMI pins have dedicated pins. In addition, the DDC pins are
multiplexed. On the Lime, the dedicated pins (ball # R23, R22) are
used. I just checked the schematics. There should be no conflict.

ChenYu

Re: [PATCHv3 3/5] ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]

Hi Vishnu,

On Fri, Dec 12, 2014 at 11:55:46PM +0530, VishnuPatekar wrote:
> 1) Fixup the sun4i ps/2 nodes interrupt property, sun4i interrupts take
> only 1 specifier
> 
> 2) dt bindings should use the compat string for the earliest version of the
> hardware which has the relevant hardware block, unless there are differences,
> the A10 and A20 ps2 controllers are identical, so for both sun4i-a10-ps2
> should be used as compat string, update the sun7i.dtsi ps2 entries to
> use the sun4i-a10-ps2 compat string.

This shouldn't be a changelog, but what this patch actually does.

> Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  arch/arm/boot/dts/sun4i-a10.dtsi |   17 +++++++++++++++++
>  arch/arm/boot/dts/sun7i-a20.dtsi |   18 ++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index 7b4099f..ef9a01c 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -629,6 +629,7 @@
>  				allwinner,drive = <0>;
>  				allwinner,pull = <0>;
>  			};
> +

This is an uneeded change

>  		};
>  
>  		timer@01c20c00 {
> @@ -795,5 +796,21 @@
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  		};
> +
> +		ps20: ps2@01c2a000 {
> +			compatible = "allwinner,sun4i-a10-ps2";
> +			reg = <0x01c2a000 0x400>;
> +			interrupts = <62>;
> +			clocks = <&apb1_gates 6>;
> +			status = "disabled";
> +		};
> +
> +		ps21: ps2@01c2a400 {
> +			compatible = "allwinner,sun4i-a10-ps2";
> +			reg = <0x01c2a400 0x400>;
> +			interrupts = <63>;
> +			clocks = <&apb1_gates 7>;
> +			status = "disabled";
> +		};
>  	};
>  };
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index e21ce59..6ab7714 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -866,6 +866,7 @@
>  				    allwinner,drive = <0>;
>  				    allwinner,pull = <0>;
>  			};
> +

Ditto.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 5/5] ARM: sunxi: dts: Add note ps2 pins conflict with hdmi

[Chen-Yu Tsai]

Hi,

On Sat, Dec 13, 2014 at 11:18 AM, Vishnu Patekar
<vishnupatekar0510@gmail.com> wrote:
> Hello Chen-Yu,
> Thank you for pointing out styling error.
>
> On Sat, Dec 13, 2014 at 7:36 AM, Chen-Yu Tsai <wens@csie.org> wrote:
>>
>> Hi,
>>
>> On Sat, Dec 13, 2014 at 2:25 AM, VishnuPatekar
>> <vishnupatekar0510@gmail.com> wrote:
>> > 1. Please note that ps20 pins conflict with HDMI on Lime2 Board
>> > so, by deault ps20 and ps21 are disabled for Lime2 Board.
>> > There is no on board ps2 connector and these pins can be used
>> > for different purpose.
>> >
>> > Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
>> > ---
>> >  arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts |    6 +++++-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > index ed364d5..951b615 100644
>> > --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > @@ -112,7 +112,11 @@
>> >                         pinctrl-0 = <&uart0_pins_a>;
>> >                         status = "okay";
>> >                 };
>> > -
>> > +
>> > +               /* PS2 0 and PS2 1 are disabled by default; Please note
>> > that
>> > +               ps20 pins conflict with HDMI on Lime2 Board
>> > +               */
>> > +
>>
>> Multi-line comments should be:
>>
>> /*
>>  * line 1
>>  * line 2
>>  * ...
>>  */
>
> Okie.

So, in fact this patch shouldnt be needed. The LIME2 uses the dedicated
pins for HDMI DDC. PS2 0 pins PI20 and PI21 are in fact routed to
the GPIO-2 header. They are _not_ used for HDMI. Nothing bad should
happen unless you also multiplex PI20/PI21 to HSCL/HDSA.

If you check the schematics Olimex published and trace the HDMI lines,
you will see that this is the case.

ChenYu

>>
>>
>> And why is there a delete and insert for an empty line?
>> Weird... though git seems to ignore it when applying.
>
> I used clean_patch to clear the styling error. that might have modified it.
>
>
>>
>>
>> ChenYu
>>
>> >                 i2c0: i2c@01c2ac00 {
>> >                         pinctrl-names = "default";
>> >                         pinctrl-0 = <&i2c0_pins_a>;
>> > --
>> > 1.7.9.5
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv3 0/5] ARM:sunxi:ps2 Added support for A10/A20 ps2 controller.

[Vishnu Patekar]

Sorry for delayed response.

Thanks, Chen-Yu.
So, I'll keep ps2 enabled for Lime2 board without any comment.

On Mon, Dec 15, 2014 at 9:11 PM, Chen-Yu Tsai <wens@csie.org> wrote:
> Hi,
>
> On Mon, Dec 15, 2014 at 11:13 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 15-12-14 15:13, Vishnu Patekar wrote:
>>>
>>> Hi,
>>>
>>> On Sun, Dec 14, 2014 at 2:31 PM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 13-12-14 21:01, Vishnu Patekar wrote:
>>>>>
>>>>>
>>>>> Hello Hans,
>>>>> Please find my comments inlined.
>>>>>
>>>>> On 12/13/14, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>> Hi VishnuPatekar,
>>>>>>
>>>>>> The patch mangling for this set seems to have gone a bit wrong I'm
>>>>>> afraid
>>>>>
>>>>>
>>>>>
>>>>> No, this time I've corrected it. Infact, last version of patch did not
>>>>> used the status bit error macros.
>>>>>
>>>>>> a lot of the patches have my fixup commit messages (which should have
>>>>>> disappeared when squashing in the patches), please replace those by
>>>>>> proper
>>>>>> commit messages describing what the patch actually does.
>>>>>
>>>>>
>>>>> Yes, [PATCHv3 3/5] uses fixup. I'll correct it.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Also the adding of the commented nodes for the lime2 seems to be gone
>>>>>> entirely
>>>>>> from the set, instead now only a comment about the conflict with the
>>>>>> hdmi
>>>>>> pins is added, but it is above the i2c node instead of above a ps2
>>>>>> node.
>>>>>>
>>>>> Maxime's suggested that we should not add commented nodes specially
>>>>> when its trivial to apply. And just note saying ps20 pins conflict
>>>>> with HDMI.
>>>>
>>>>
>>>>
>>>> Ah, I see.
>>>>
>>>>> Should I remove the this comment as well?
>>>>> Or
>>>>> Put this note in start of DTS file?
>>>>
>>>>
>>>>
>>>> The comment should be added to where the pinctrl bits are, not to the
>>>> lime2
>>>> file, ps0 will conflict with hdmi on all boards.
>>>
>>>
>>> Well, In that case,PS2_0 pins conflicts with HDMI, PS2_1 pins
>>> conflicts with LCD. Every pin has multiplexing.
>>
>>
>> Almost every A10 board has a hdmi connector, and only the hdmi ddc pins
>> are multiplexed, so this is sort of special, but yes if we do not have
>> the commented nodes in the dts file, then this comment can be dropped too
>> I guess.
>
> All the HDMI pins have dedicated pins. In addition, the DDC pins are
> multiplexed. On the Lime, the dedicated pins (ball # R23, R22) are
> used. I just checked the schematics. There should be no conflict.
>
> ChenYu

Re: [PATCHv3 2/5] ARM:sunxi:drivers:input Add support for A10/A20 PS2

[Vishnu Patekar]

Hello Dmitry,
Sorry for delayed response. please find my comment in-lined.
Please let me know in case I am wrong.

On Mon, Dec 15, 2014 at 2:07 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Vishnu,
>
> On Fri, Dec 12, 2014 at 11:55:45PM +0530, VishnuPatekar wrote:
>>
>> Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
>> ---
>>  drivers/input/serio/Kconfig     |   11 ++
>>  drivers/input/serio/Makefile    |    1 +
>>  drivers/input/serio/sun4i-ps2.c |  362 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 374 insertions(+)
>>  create mode 100644 drivers/input/serio/sun4i-ps2.c
>>
>> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
>> index bc2d474..964afc5 100644
>> --- a/drivers/input/serio/Kconfig
>> +++ b/drivers/input/serio/Kconfig
>> @@ -281,4 +281,15 @@ config HYPERV_KEYBOARD
>>         To compile this driver as a module, choose M here: the module will
>>         be called hyperv_keyboard.
>>
>> +config SERIO_SUN4I_PS2
>> +     tristate "Allwinner A10 PS/2 controller support"
>> +     default n
>> +     depends on ARCH_SUNXI || COMPILE_TEST
>> +     help
>> +       This selects support for the PS/2 Host Controller on
>> +       Allwinner A10.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called sun4i-ps2.
>> +
>>  endif
>> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
>> index 815d874..c600089 100644
>> --- a/drivers/input/serio/Makefile
>> +++ b/drivers/input/serio/Makefile
>> @@ -29,3 +29,4 @@ obj-$(CONFIG_SERIO_ARC_PS2) += arc_ps2.o
>>  obj-$(CONFIG_SERIO_APBPS2)   += apbps2.o
>>  obj-$(CONFIG_SERIO_OLPC_APSP)        += olpc_apsp.o
>>  obj-$(CONFIG_HYPERV_KEYBOARD)        += hyperv-keyboard.o
>> +obj-$(CONFIG_SERIO_SUN4I_PS2)        += sun4i-ps2.o
>> diff --git a/drivers/input/serio/sun4i-ps2.c b/drivers/input/serio/sun4i-ps2.c
>> new file mode 100644
>> index 0000000..e6d22ae
>> --- /dev/null
>> +++ b/drivers/input/serio/sun4i-ps2.c
>> @@ -0,0 +1,362 @@
>> +/*
>> + *   Driver for Allwinner A10 PS2 host controller
>> + *
>> + *   Author: Vishnu Patekar <vishnupatekar0510@gmail.com>
>> + *           Aaron.maoye <leafy.myeh@newbietech.com>
>> + *
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/serio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/errno.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>> +#include <linux/io.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +
>> +#define DRIVER_NAME          "sun4i-ps2"
>> +
>> +/* register offset definitions */
>> +#define PS2_REG_GCTL         (0x00)  /*  PS2 Module Global Control Reg */
>> +#define PS2_REG_DATA         (0x04)  /*  PS2 Module Data Reg         */
>> +#define PS2_REG_LCTL         (0x08)  /*  PS2 Module Line Control Reg */
>> +#define PS2_REG_LSTS         (0x0C)  /*  PS2 Module Line Status Reg  */
>> +#define PS2_REG_FCTL         (0x10)  /*  PS2 Module FIFO Control Reg */
>> +#define PS2_REG_FSTS         (0x14)  /*  PS2 Module FIFO Status Reg  */
>> +#define PS2_REG_CLKDR                (0x18)  /*  PS2 Module Clock Divider Reg*/
>
> You do not need parenthesis around simple constants.
Okie, I'll remove.
>
>> +
>> +/*  PS2 GLOBAL CONTROL REGISTER PS2_GCTL */
>> +#define PS2_GCTL_INTFLAG     BIT(4)
>> +#define PS2_GCTL_INTEN               BIT(3)
>> +#define PS2_GCTL_RESET               BIT(2)
>> +#define PS2_GCTL_MASTER              BIT(1)
>> +#define PS2_GCTL_BUSEN               BIT(0)
>> +
>> +/* PS2 LINE CONTROL REGISTER */
>> +#define PS2_LCTL_NOACK               BIT(18)
>> +#define PS2_LCTL_TXDTOEN     BIT(8)
>> +#define PS2_LCTL_STOPERREN   BIT(3)
>> +#define PS2_LCTL_ACKERREN    BIT(2)
>> +#define PS2_LCTL_PARERREN    BIT(1)
>> +#define PS2_LCTL_RXDTOEN     BIT(0)
>> +
>> +/* PS2 LINE STATUS REGISTER */
>> +#define PS2_LSTS_TXTDO               BIT(8)
>> +#define PS2_LSTS_STOPERR     BIT(3)
>> +#define PS2_LSTS_ACKERR              BIT(2)
>> +#define PS2_LSTS_PARERR              BIT(1)
>> +#define PS2_LSTS_RXTDO               BIT(0)
>> +
>> +#define PS2_LINE_ERROR_BIT \
>> +     (PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR | \
>> +     PS2_LSTS_PARERR | PS2_LSTS_RXTDO)
>> +
>> +/* PS2 FIFO CONTROL REGISTER */
>> +#define PS2_FCTL_TXRST               BIT(17)
>> +#define PS2_FCTL_RXRST               BIT(16)
>> +#define PS2_FCTL_TXUFIEN     BIT(10)
>> +#define PS2_FCTL_TXOFIEN     BIT(9)
>> +#define PS2_FCTL_TXRDYIEN    BIT(8)
>> +#define PS2_FCTL_RXUFIEN     BIT(2)
>> +#define PS2_FCTL_RXOFIEN     BIT(1)
>> +#define PS2_FCTL_RXRDYIEN    BIT(0)
>> +
>> +/* PS2 FIFO STATUS REGISTER */
>> +#define PS2_FSTS_TXUF                BIT(10)
>> +#define PS2_FSTS_TXOF                BIT(9)
>> +#define PS2_FSTS_TXRDY               BIT(8)
>> +#define PS2_FSTS_RXUF                BIT(2)
>> +#define PS2_FSTS_RXOF                BIT(1)
>> +#define PS2_FSTS_RXRDY               BIT(0)
>> +
>> +#define PS2_FIFO_ERROR_BIT \
>> +     (PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_RXUF | PS2_FSTS_RXOF)
>> +
>> +#define PS2_SAMPLE_CLK               (1000000)
>> +#define PS2_SCLK             (125000)
I'll remove these parenthesis around simple constants.
>> +
>> +struct sun4i_ps2data {
>> +     struct serio *serio;
>> +     struct device *dev;
>> +
>> +     /* IO mapping base */
>> +     void __iomem    *reg_base;
>> +
>> +     /* clock management */
>> +     struct clk      *clk;
>> +
>> +     /* irq */
>> +     spinlock_t      lock;
>> +     int             irq;
>> +};
>> +
>> +/*********************/
>> +/* Interrupt handler */
>> +/*********************/
>> +static irqreturn_t sun4i_ps2_interrupt(int irq, void *dev_id)
>> +{
>> +     struct sun4i_ps2data *drvdata = dev_id;
>> +     u32 intr_status;
>> +     u32 fifo_status;
>> +     unsigned char byte;
>> +     u32 rval;
>> +     u32 error = 0;
>> +
>> +     spin_lock(&drvdata->lock);
>> +
>> +     /* Get the PS/2 interrupts and clear them */
>> +     intr_status  = readl(drvdata->reg_base + PS2_REG_LSTS);
>> +     fifo_status  = readl(drvdata->reg_base + PS2_REG_FSTS);
>> +
>> +     /*Check Line Status Register*/
>> +     if (intr_status & PS2_LINE_ERROR_BIT) {
>> +             if (intr_status & PS2_LSTS_STOPERR)
>> +                     dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
>
> The stop bit error is I believe should be reported to consumers as
> SSERIO_FRAME, receive timeout as SERIO_TIMEOUT and parity as
> SERIO_PARITY.
Yes, this can be passed via flags in setio_interrpt. I'll add this.
>
>> +             if (intr_status & PS2_LSTS_ACKERR)
>> +                     dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
>
> When is this error signalled?
this is signaled when ACK is not received after data transmitted.

>
>> +             if (intr_status & PS2_LSTS_PARERR)
>> +                     dev_info(drvdata->dev, "PS/2 Parity Error!\n");
>> +             if (intr_status & PS2_LSTS_TXTDO)
>> +                     dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
>
> Should not you check this status in serio_write() instead of here?
I never get transmit data timeout in serio_write(), even if PS2
keyboard is not connected.
However, it generates this interrupt.
Yes, in case of timeout/failure in serio_write SERIO_TIMEOUT can be returned.
>
>> +             if (intr_status & PS2_LSTS_RXTDO)
>> +                     dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
>> +
>> +             /*reset PS/2 controller*/
>> +             rval = readl(drvdata->reg_base + PS2_REG_GCTL);
>> +             writel(rval | PS2_GCTL_RESET, drvdata->reg_base + PS2_REG_GCTL);
>
> Why do we need to reset controller in case of, let's say, parity glitch?
As per the Reference Manual :
Setting SOFT RESET bit will reset transmitter and receiver of PS2
Module, and the status of transmitter and receiver will revert
to the default state, but not affect any control bits in register,
and data in TXFIFO/RXFIFO.

I think, there is no harm in soft resetting PS2 instead of writing to
multiple registers.
>
>> +
>> +             rval = PS2_LSTS_TXTDO | PS2_LSTS_STOPERR | PS2_LSTS_ACKERR |
>> +                     PS2_LSTS_PARERR | PS2_LSTS_RXTDO;
>> +             writel(rval, drvdata->reg_base + PS2_REG_LSTS);
>> +             error = 1;
>> +     }
>> +
>> +     /*Check FIFO Status Register*/
>> +     if (fifo_status & PS2_FIFO_ERROR_BIT) {
>> +             if (fifo_status & PS2_FSTS_TXUF)
>> +                     dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
>> +             if (fifo_status & PS2_FSTS_TXOF)
>> +                     dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
>> +             if (fifo_status & PS2_FSTS_RXUF)
>> +                     dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
>> +             if (fifo_status & PS2_FSTS_RXOF)
>> +                     dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
>> +             /*reset PS/2 controller*/
>> +             writel(readl(drvdata->reg_base + PS2_REG_GCTL) | PS2_GCTL_RESET,
>> +                     drvdata->reg_base + PS2_REG_GCTL);
>
> Again, we do we need to reset controller? Does it stop receiving data
> after RX overflow condition?
After RX overflow it just set the overflow flag in FSTS reg, then we
can reset FIFO to get more data.

>
>> +
>> +             rval = PS2_FSTS_TXUF | PS2_FSTS_TXOF | PS2_FSTS_TXRDY |
>> +                     PS2_FSTS_RXUF | PS2_FSTS_RXOF | PS2_FSTS_RXRDY;
>> +             writel(rval, drvdata->reg_base + PS2_REG_FSTS);
>> +             error = 1;
>> +     }
>> +
>> +     rval = (fifo_status >> 16) & 0x3;
>> +     while (!error && rval--) {
>> +             byte = readl(drvdata->reg_base + PS2_REG_DATA) & 0xff;
>> +             serio_interrupt(drvdata->serio, byte, 0);
>> +     }
>> +
>> +     writel(intr_status, drvdata->reg_base + PS2_REG_LSTS);
>> +     writel(fifo_status, drvdata->reg_base + PS2_REG_FSTS);
>> +
>> +     spin_unlock(&drvdata->lock);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +
>> +static int sun4i_ps2_open(struct serio *pserio)
>> +{
>> +     struct sun4i_ps2data *drvdata = pserio->port_data;
>> +     u32 src_clk = 0;
>> +     u32 clk_scdf;
>> +     u32 clk_pcdf;
>> +     u32 rval;
>> +     unsigned long flags;
>> +
>> +     /*Set Line Control And Enable Interrupt*/
>> +     rval = PS2_LCTL_TXDTOEN | PS2_LCTL_STOPERREN | PS2_LCTL_ACKERREN
>> +             | PS2_LCTL_PARERREN | PS2_LCTL_RXDTOEN;
>> +     writel(rval, drvdata->reg_base + PS2_REG_LCTL);
>> +
>> +     /*Reset FIFO*/
>> +     rval = PS2_FCTL_TXRST | PS2_FCTL_RXRST | PS2_FCTL_TXUFIEN
>> +             | PS2_FCTL_TXOFIEN | PS2_FCTL_RXUFIEN
>> +             | PS2_FCTL_RXOFIEN | PS2_FCTL_RXRDYIEN;
>> +
>> +     writel(rval, drvdata->reg_base + PS2_REG_FCTL);
>> +
>> +     src_clk = clk_get_rate(drvdata->clk);
>> +     /*Set Clock Divider Register*/
>> +     clk_scdf = DIV_ROUND_UP(src_clk, PS2_SAMPLE_CLK) - 1;
>> +     clk_pcdf = DIV_ROUND_UP(PS2_SAMPLE_CLK, PS2_SCLK) - 1;
>> +     rval = (clk_scdf<<8) | clk_pcdf;
>> +     writel(rval, drvdata->reg_base + PS2_REG_CLKDR);
>> +
>> +
>> +     /*Set Global Control Register*/
>> +     rval = PS2_GCTL_RESET | PS2_GCTL_INTEN | PS2_GCTL_MASTER
>> +             | PS2_GCTL_BUSEN;
>> +
>> +     spin_lock_irqsave(&drvdata->lock, flags);
>> +     writel(rval, drvdata->reg_base + PS2_REG_GCTL);
>> +     spin_unlock_irqrestore(&drvdata->lock, flags);
>> +
>> +     return 0;
>> +}
>> +
>> +static void sun4i_ps2_close(struct serio *pserio)
>> +{
>> +     struct sun4i_ps2data *drvdata = pserio->port_data;
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&drvdata->lock, flags);
>> +     /* Disable the PS2 interrupts */
>> +     writel(0, drvdata->reg_base + PS2_REG_GCTL);
>> +     spin_unlock_irqrestore(&drvdata->lock, flags);
>
>
> I think you want to add synchronize_irq() here to make sure pending
> interrupt handler runs to completion before you return.
Yes, synchronize_irq()
>
>> +}
>> +
>> +static int sun4i_ps2_write(struct serio *pserio, unsigned char val)
>> +{
>> +     unsigned long expire = jiffies + msecs_to_jiffies(10000);
>> +     struct sun4i_ps2data *drvdata;
>> +
>> +     drvdata = (struct sun4i_ps2data *)pserio->port_data;
>> +
>> +     do {
>> +             if (readl(drvdata->reg_base + PS2_REG_FSTS) & PS2_FSTS_TXRDY) {
>> +                     writel(val, drvdata->reg_base + PS2_REG_DATA);
>> +                     return 0;
>> +             }
>> +     } while (time_before(jiffies, expire));
>> +
>> +     return 0;
>> +}
>> +
>> +static int sun4i_ps2_probe(struct platform_device *pdev)
>> +{
>> +     struct resource *res; /* IO mem resources */
>> +     struct sun4i_ps2data *drvdata;
>> +     struct serio *serio;
>> +     struct device *dev = &pdev->dev;
>> +     unsigned int irq;
>> +     int error;
>> +
>> +     drvdata = devm_kzalloc(dev, sizeof(struct sun4i_ps2data), GFP_KERNEL);
>> +     serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
>> +     if (!drvdata || !serio) {
>> +             error = -ENOMEM;
>> +             goto err_free_mem;
>> +     }
>> +
>> +     spin_lock_init(&drvdata->lock);
>> +
>> +     /* IO */
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     drvdata->reg_base = devm_ioremap_resource(dev, res);
>> +     if (IS_ERR(drvdata->reg_base)) {
>> +             dev_err(dev, "failed to map registers\n");
>> +             error = PTR_ERR(drvdata->reg_base);
>> +             goto err_free_mem;
>> +     }
>> +
>> +     drvdata->clk = devm_clk_get(dev, NULL);
>> +     if (IS_ERR(drvdata->clk)) {
>> +             error = PTR_ERR(drvdata->clk);
>> +             dev_err(dev, "couldn't get clock %d\n", error);
>> +             goto err_free_mem;
>> +     }
>> +
>> +     error = clk_prepare_enable(drvdata->clk);
>> +     if (error) {
>> +             dev_err(dev, "failed to enable clock %d\n", error);
>> +             goto err_free_mem;
>> +     }
>> +
>
> I think you should explicitly prohibit the device generating interrupts
> by writing to PS2_ERG_CTRL here.
Okie.
>
>> +     serio->id.type = SERIO_8042;
>> +     serio->write = sun4i_ps2_write;
>> +     serio->open = sun4i_ps2_open;
>> +     serio->close = sun4i_ps2_close;
>> +     serio->port_data = drvdata;
>> +     serio->dev.parent = dev;
>> +     strlcpy(serio->name, dev_name(dev), sizeof(serio->name));
>> +     strlcpy(serio->phys, dev_name(dev), sizeof(serio->phys));
>> +
>> +     /* Get IRQ for the device */
>> +     irq = platform_get_irq(pdev, 0);
>> +     if (!irq) {
>> +             dev_err(dev, "no IRQ found\n");
>> +             error = -ENXIO;
>> +             goto error_disable_clk;
>> +     }
>> +
>> +     drvdata->irq = irq;
>> +     drvdata->serio = serio;
>> +     drvdata->dev = dev;
>> +     error = devm_request_threaded_irq(dev, drvdata->irq,
>> +             sun4i_ps2_interrupt, NULL, 0, DRIVER_NAME, drvdata);
>> +
>> +     if (error) {
>> +             dev_err(drvdata->dev, "Interrupt alloc failed %d:error:%d\n",
>> +                     drvdata->irq, error);
>> +             goto error_disable_clk;
>> +     }
>> +
>> +     serio_register_port(serio);
>> +     platform_set_drvdata(pdev, drvdata);
>> +
>> +     return 0;       /* success */
>> +
>> +error_disable_clk:
>> +     clk_disable_unprepare(drvdata->clk);
>> +
>> +err_free_mem:
>> +     kfree(serio);
>> +     return error;
>> +}
>> +
>> +static int sun4i_ps2_remove(struct platform_device *pdev)
>> +{
>> +     struct sun4i_ps2data *drvdata = platform_get_drvdata(pdev);
>> +
>> +     serio_unregister_port(drvdata->serio);
>> +     disable_irq(drvdata->irq);
>
> I do not think you need to disable irq here - serio_close will make sure
> that the device does not generate interrupts.
Okie
>
>> +
>> +     if (!IS_ERR(drvdata->clk))
>> +             clk_disable_unprepare(drvdata->clk);
>> +     kfree(drvdata->serio);
>> +
>> +     return 0;
>> +}
>> +
>> +/* Match table for of_platform binding */
>> +static const struct of_device_id sun4i_ps2_match[] = {
>> +     { .compatible = "allwinner,sun4i-a10-ps2", },
>> +     { },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, sun4i_ps2_match);
>> +
>> +/*platform driver structure*/
>
> I do not think we should keep the obvious comments like this one.
Okie, I'll remove it.
>
>> +static struct platform_driver sun4i_ps2_driver = {
>> +     .probe          = sun4i_ps2_probe,
>> +     .remove         = sun4i_ps2_remove,
>> +     .driver = {
>> +             .name = DRIVER_NAME,
>> +             .of_match_table = sun4i_ps2_match,
>> +     },
>> +};
>> +module_platform_driver(sun4i_ps2_driver);
>> +
>> +MODULE_AUTHOR("Vishnu Patekar <vishnupatekar0510@gmail.com>");
>> +MODULE_AUTHOR("Aaron.maoye <leafy.myeh@newbietech.com>");
>> +MODULE_DESCRIPTION("Allwinner A10/Sun4i PS/2 driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.7.9.5
>>
>
> Thanks.
>
> --
> Dmitry