* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-03 22:53 ` vishnupatekar
0 siblings, 0 replies; 22+ messages in thread
From: vishnupatekar @ 2014-12-03 22:53 UTC (permalink / raw
To: maxime.ripard, linux-sunxi
Cc: robh+dt, benh, msalter, ralf, jdelvare, linux-kernel, linux-input,
linux-arm-kernel, devicetree, dmitry.torokhov, vishnupatekar
---
drivers/input/serio/Kconfig | 9 ++
drivers/input/serio/Makefile | 1 +
drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
3 files changed, 315 insertions(+)
create mode 100644 drivers/input/serio/sunxi-ps2.c
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..1a86e41 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
To compile this driver as a module, choose M here: the module will
be called hyperv_keyboard.
+config SERIO_SUNXI_PS2
+ tristate "Allwinner Sun7i-A20 PS/2 controller"
+ default m
+ help
+ Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sunxi-ps2.
+
endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
new file mode 100644
index 0000000..ccd7b29
--- /dev/null
+++ b/drivers/input/serio/sunxi-ps2.c
@@ -0,0 +1,305 @@
+/*
+ * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
+ *
+ * Author: Aaron.maoye <leafy.myeh@newbietech.com>
+ * Vishnu Patekar <vishnupatekar0510@gmail.com>
+ * Based on sunxi-ps2.c 3.0 kernel
+ *
+*/
+
+#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 "sunxi-ps2"
+
+#define RESSIZE(res) (((res)->end - (res)->start)+1)
+
+#define SW_PS2_GCTRL (0x00)
+#define SW_PS2_DATA (0x04)
+#define SW_PS2_LCTRL (0x08)
+#define SW_PS2_LSTAT (0x0c)
+#define SW_PS2_FCTRL (0x10)
+#define SW_PS2_FSTAT (0x14)
+#define SW_PS2_CLKDR (0x18)
+
+/* SW_PS2_GCTRL */
+#define SWPS2_BUSEN (1 << 0)
+#define SWPS2_MASTER (1 << 1)
+#define SWPS2_RESET (1 << 2)
+#define SWPS2_INTEN (1 << 3)
+#define SWPS2_INTFLAG (1 << 3)
+
+/* SW_PS2_LCTRL */
+#define SWPS2_LCTL_NOACK (0x0 << 18)
+#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
+#define SWPS2_LCTL_STOPERREN (0x1 << 3)
+#define SWPS2_LCTL_ACKERREN (0x1 << 2)
+#define SWPS2_LCTL_PARERREN (0x1 << 1)
+#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
+
+/* SW_PS2_FSTAT */
+#define SWPS2_FSTA_RXRDY (1 << 0)
+#define SWPS2_FSTA_RXOF (1 << 1)
+#define SWPS2_FSTA_RXUF (1 << 2)
+#define SWPS2_FSTA_TXRDY (1 << 8)
+#define SWPS2_FSTA_TXOF (1 << 9)
+#define SWPS2_FSTA_TXUF (1 << 10)
+
+#define SW_PS2_SAMPLE_CLK (1000000)
+#define SW_PS2_SCLK (125000)
+
+struct sunxips2data {
+ int irq;
+ spinlock_t ps2_lock;
+ void __iomem *base_address; /* virt address of control registers*/
+ struct serio *serio; /* serio*/
+ struct device *dev;
+ struct clk *pclk;
+};
+
+/*********************/
+/* Interrupt handler */
+/*********************/
+static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
+{
+ struct sunxips2data *drvdata = dev_id;
+ u32 intr_status;
+ u32 fifo_status;
+ unsigned char byte;
+ u32 rval;
+ u32 error = 0;
+
+ spin_lock(&drvdata->ps2_lock);
+
+ /* Get the PS/2 interrupts and clear them */
+ intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
+ fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
+
+ /*Check Line Status Register*/
+ if (intr_status & 0x10f) {
+ if (intr_status & 0x08)
+ dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
+ if (intr_status & 0x04)
+ dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
+ if (intr_status & 0x02)
+ dev_info(drvdata->dev, "PS/2 Parity Error!\n");
+ if (intr_status & 0x100)
+ dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
+ if (intr_status & 0x01)
+ dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
+
+ writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
+ writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
+
+ error = 1;
+ }
+
+ /*Check FIFO Status Register*/
+ if (fifo_status & 0x0606) {
+ if (fifo_status & 0x400)
+ dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
+ if (fifo_status & 0x200)
+ dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
+ if (fifo_status & 0x04)
+ dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
+ if (fifo_status & 0x02)
+ dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
+
+ writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
+ writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
+ error = 1;
+ }
+
+ rval = (fifo_status >> 16) & 0x3;
+ while (!error && rval--) {
+ byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
+ dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
+ serio_interrupt(drvdata->serio, byte, 0);
+ }
+
+ writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
+ writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
+
+ spin_unlock(&drvdata->ps2_lock);
+
+ return IRQ_HANDLED;
+}
+
+static int sunxips2_open(struct serio *pserio)
+{
+ struct sunxips2data *drvdata = pserio->port_data;
+ u32 src_clk = 0;
+ u32 clk_scdf;
+ u32 clk_pcdf;
+ u32 rval;
+
+ /*Set Line Control And Enable Interrupt*/
+ rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
+ writel(rval, drvdata->base_address + SW_PS2_LCTRL);
+
+ /*Reset FIFO*/
+ writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
+
+ src_clk = clk_get_rate(drvdata->pclk);
+
+ if (!src_clk) {
+ dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
+ return -1;
+ }
+
+ /*Set Clock Divider Register*/
+ clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
+ clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
+ rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
+ writel(rval, drvdata->base_address + SW_PS2_CLKDR);
+
+ /*Set Global Control Register*/
+ rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
+ writel(rval, drvdata->base_address + SW_PS2_GCTRL);
+
+ udelay(100);
+
+ return 0;
+}
+
+static void sunxips2_close(struct serio *pserio)
+{
+ struct sunxips2data *drvdata = pserio->port_data;
+
+ spin_lock(&drvdata->ps2_lock);
+ /* Disable the PS2 interrupts */
+ writel(0, drvdata->base_address + SW_PS2_GCTRL);
+ spin_unlock(&drvdata->ps2_lock);
+}
+
+static int sunxips2_write(struct serio *pserio, unsigned char val)
+{
+ struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
+ u32 timeout = 10000;
+
+ do {
+ if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
+ writel(val, drvdata->base_address + SW_PS2_DATA);
+ return 0;
+ }
+ } while (timeout--);
+
+ return -1;
+}
+
+static int sunxips2_probe(struct platform_device *ofdev)
+{
+ struct resource *res; /* IO mem resources */
+ struct sunxips2data *drvdata;
+ struct serio *serio;
+ struct device *dev = &ofdev->dev;
+ unsigned int irq;
+ int error;
+
+ drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
+ serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
+ if (!drvdata || !serio)
+ error = -ENOMEM;
+
+ /* Request clock */
+ drvdata->pclk = clk_get(dev, NULL);
+ if (IS_ERR(drvdata->pclk))
+ dev_dbg(dev, "couldn't get clock %li\n",
+ PTR_ERR(drvdata->pclk));
+
+ if (!IS_ERR(drvdata->pclk)) {
+ error = clk_prepare_enable(drvdata->pclk);
+ if (error < 0) {
+ dev_err(dev, "failed to enable clock %d\n", error);
+ return error;
+ }
+ }
+
+ /* IO */
+ res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+ drvdata->base_address = devm_ioremap_resource(dev, res);
+ if (IS_ERR(drvdata->base_address)) {
+ dev_err(dev, "failed to map registers\n");
+ error = PTR_ERR(drvdata->base_address);
+ }
+
+ serio->id.type = SERIO_8042;
+ serio->write = sunxips2_write;
+ serio->open = sunxips2_open;
+ serio->close = sunxips2_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));
+
+ platform_set_drvdata(ofdev, drvdata);
+ serio_register_port(serio);
+
+ /* Get IRQ for the device */
+ irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
+ if (!irq) {
+ dev_err(dev, "no IRQ found\n");
+ return -ENODEV;
+ }
+
+ drvdata->irq = irq;
+ drvdata->serio = serio;
+ drvdata->dev = dev;
+ error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
+ DRIVER_NAME, drvdata);
+ if (error) {
+ dev_err(drvdata->dev,
+ "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
+ return error;
+ }
+ return 0; /* success */
+}
+
+static int sunxips2_remove(struct platform_device *of_dev)
+{
+ struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
+
+ if (!IS_ERR(drvdata->pclk)) {
+ clk_disable_unprepare(drvdata->pclk);
+ clk_put(drvdata->pclk);
+ }
+ serio_unregister_port(drvdata->serio);
+ mdelay(2);
+ return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id sunxips2_of_match[] = {
+ { .compatible = "allwinner,sun7i-a20-ps2", },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, sunxips2_of_match);
+
+/*platform driver structure*/
+static struct platform_driver sunxips2_of_driver = {
+ .probe = sunxips2_probe,
+ .remove = sunxips2_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = sunxips2_of_match,
+ },
+};
+module_platform_driver(sunxips2_of_driver);
+
+MODULE_AUTHOR("Aaron.maoye<leafy.myeh@newbietech.com, "
+ "Vishnu Patekar <vishnupatekar0510@gmail.com>");
+MODULE_DESCRIPTION("Sunxi PS/2 driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-03 22:53 ` vishnupatekar
0 siblings, 0 replies; 22+ messages in thread
From: vishnupatekar @ 2014-12-03 22:53 UTC (permalink / raw
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
msalter-H+wXaHxf7aLQT0dZR+AlfA, ralf-6z/3iImG2C8G8FEW9MqTrA,
jdelvare-l3A5Bk7waGM, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w, vishnupatekar
---
drivers/input/serio/Kconfig | 9 ++
drivers/input/serio/Makefile | 1 +
drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
3 files changed, 315 insertions(+)
create mode 100644 drivers/input/serio/sunxi-ps2.c
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..1a86e41 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
To compile this driver as a module, choose M here: the module will
be called hyperv_keyboard.
+config SERIO_SUNXI_PS2
+ tristate "Allwinner Sun7i-A20 PS/2 controller"
+ default m
+ help
+ Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sunxi-ps2.
+
endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
new file mode 100644
index 0000000..ccd7b29
--- /dev/null
+++ b/drivers/input/serio/sunxi-ps2.c
@@ -0,0 +1,305 @@
+/*
+ * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
+ *
+ * Author: Aaron.maoye <leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org>
+ * Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ * Based on sunxi-ps2.c 3.0 kernel
+ *
+*/
+
+#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 "sunxi-ps2"
+
+#define RESSIZE(res) (((res)->end - (res)->start)+1)
+
+#define SW_PS2_GCTRL (0x00)
+#define SW_PS2_DATA (0x04)
+#define SW_PS2_LCTRL (0x08)
+#define SW_PS2_LSTAT (0x0c)
+#define SW_PS2_FCTRL (0x10)
+#define SW_PS2_FSTAT (0x14)
+#define SW_PS2_CLKDR (0x18)
+
+/* SW_PS2_GCTRL */
+#define SWPS2_BUSEN (1 << 0)
+#define SWPS2_MASTER (1 << 1)
+#define SWPS2_RESET (1 << 2)
+#define SWPS2_INTEN (1 << 3)
+#define SWPS2_INTFLAG (1 << 3)
+
+/* SW_PS2_LCTRL */
+#define SWPS2_LCTL_NOACK (0x0 << 18)
+#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
+#define SWPS2_LCTL_STOPERREN (0x1 << 3)
+#define SWPS2_LCTL_ACKERREN (0x1 << 2)
+#define SWPS2_LCTL_PARERREN (0x1 << 1)
+#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
+
+/* SW_PS2_FSTAT */
+#define SWPS2_FSTA_RXRDY (1 << 0)
+#define SWPS2_FSTA_RXOF (1 << 1)
+#define SWPS2_FSTA_RXUF (1 << 2)
+#define SWPS2_FSTA_TXRDY (1 << 8)
+#define SWPS2_FSTA_TXOF (1 << 9)
+#define SWPS2_FSTA_TXUF (1 << 10)
+
+#define SW_PS2_SAMPLE_CLK (1000000)
+#define SW_PS2_SCLK (125000)
+
+struct sunxips2data {
+ int irq;
+ spinlock_t ps2_lock;
+ void __iomem *base_address; /* virt address of control registers*/
+ struct serio *serio; /* serio*/
+ struct device *dev;
+ struct clk *pclk;
+};
+
+/*********************/
+/* Interrupt handler */
+/*********************/
+static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
+{
+ struct sunxips2data *drvdata = dev_id;
+ u32 intr_status;
+ u32 fifo_status;
+ unsigned char byte;
+ u32 rval;
+ u32 error = 0;
+
+ spin_lock(&drvdata->ps2_lock);
+
+ /* Get the PS/2 interrupts and clear them */
+ intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
+ fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
+
+ /*Check Line Status Register*/
+ if (intr_status & 0x10f) {
+ if (intr_status & 0x08)
+ dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
+ if (intr_status & 0x04)
+ dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
+ if (intr_status & 0x02)
+ dev_info(drvdata->dev, "PS/2 Parity Error!\n");
+ if (intr_status & 0x100)
+ dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
+ if (intr_status & 0x01)
+ dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
+
+ writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
+ writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
+
+ error = 1;
+ }
+
+ /*Check FIFO Status Register*/
+ if (fifo_status & 0x0606) {
+ if (fifo_status & 0x400)
+ dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
+ if (fifo_status & 0x200)
+ dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
+ if (fifo_status & 0x04)
+ dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
+ if (fifo_status & 0x02)
+ dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
+
+ writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
+ writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
+ error = 1;
+ }
+
+ rval = (fifo_status >> 16) & 0x3;
+ while (!error && rval--) {
+ byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
+ dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
+ serio_interrupt(drvdata->serio, byte, 0);
+ }
+
+ writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
+ writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
+
+ spin_unlock(&drvdata->ps2_lock);
+
+ return IRQ_HANDLED;
+}
+
+static int sunxips2_open(struct serio *pserio)
+{
+ struct sunxips2data *drvdata = pserio->port_data;
+ u32 src_clk = 0;
+ u32 clk_scdf;
+ u32 clk_pcdf;
+ u32 rval;
+
+ /*Set Line Control And Enable Interrupt*/
+ rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
+ writel(rval, drvdata->base_address + SW_PS2_LCTRL);
+
+ /*Reset FIFO*/
+ writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
+
+ src_clk = clk_get_rate(drvdata->pclk);
+
+ if (!src_clk) {
+ dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
+ return -1;
+ }
+
+ /*Set Clock Divider Register*/
+ clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
+ clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
+ rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
+ writel(rval, drvdata->base_address + SW_PS2_CLKDR);
+
+ /*Set Global Control Register*/
+ rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
+ writel(rval, drvdata->base_address + SW_PS2_GCTRL);
+
+ udelay(100);
+
+ return 0;
+}
+
+static void sunxips2_close(struct serio *pserio)
+{
+ struct sunxips2data *drvdata = pserio->port_data;
+
+ spin_lock(&drvdata->ps2_lock);
+ /* Disable the PS2 interrupts */
+ writel(0, drvdata->base_address + SW_PS2_GCTRL);
+ spin_unlock(&drvdata->ps2_lock);
+}
+
+static int sunxips2_write(struct serio *pserio, unsigned char val)
+{
+ struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
+ u32 timeout = 10000;
+
+ do {
+ if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
+ writel(val, drvdata->base_address + SW_PS2_DATA);
+ return 0;
+ }
+ } while (timeout--);
+
+ return -1;
+}
+
+static int sunxips2_probe(struct platform_device *ofdev)
+{
+ struct resource *res; /* IO mem resources */
+ struct sunxips2data *drvdata;
+ struct serio *serio;
+ struct device *dev = &ofdev->dev;
+ unsigned int irq;
+ int error;
+
+ drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
+ serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
+ if (!drvdata || !serio)
+ error = -ENOMEM;
+
+ /* Request clock */
+ drvdata->pclk = clk_get(dev, NULL);
+ if (IS_ERR(drvdata->pclk))
+ dev_dbg(dev, "couldn't get clock %li\n",
+ PTR_ERR(drvdata->pclk));
+
+ if (!IS_ERR(drvdata->pclk)) {
+ error = clk_prepare_enable(drvdata->pclk);
+ if (error < 0) {
+ dev_err(dev, "failed to enable clock %d\n", error);
+ return error;
+ }
+ }
+
+ /* IO */
+ res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+ drvdata->base_address = devm_ioremap_resource(dev, res);
+ if (IS_ERR(drvdata->base_address)) {
+ dev_err(dev, "failed to map registers\n");
+ error = PTR_ERR(drvdata->base_address);
+ }
+
+ serio->id.type = SERIO_8042;
+ serio->write = sunxips2_write;
+ serio->open = sunxips2_open;
+ serio->close = sunxips2_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));
+
+ platform_set_drvdata(ofdev, drvdata);
+ serio_register_port(serio);
+
+ /* Get IRQ for the device */
+ irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
+ if (!irq) {
+ dev_err(dev, "no IRQ found\n");
+ return -ENODEV;
+ }
+
+ drvdata->irq = irq;
+ drvdata->serio = serio;
+ drvdata->dev = dev;
+ error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
+ DRIVER_NAME, drvdata);
+ if (error) {
+ dev_err(drvdata->dev,
+ "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
+ return error;
+ }
+ return 0; /* success */
+}
+
+static int sunxips2_remove(struct platform_device *of_dev)
+{
+ struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
+
+ if (!IS_ERR(drvdata->pclk)) {
+ clk_disable_unprepare(drvdata->pclk);
+ clk_put(drvdata->pclk);
+ }
+ serio_unregister_port(drvdata->serio);
+ mdelay(2);
+ return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id sunxips2_of_match[] = {
+ { .compatible = "allwinner,sun7i-a20-ps2", },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, sunxips2_of_match);
+
+/*platform driver structure*/
+static struct platform_driver sunxips2_of_driver = {
+ .probe = sunxips2_probe,
+ .remove = sunxips2_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = sunxips2_of_match,
+ },
+};
+module_platform_driver(sunxips2_of_driver);
+
+MODULE_AUTHOR("Aaron.maoye<leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org, "
+ "Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
+MODULE_DESCRIPTION("Sunxi PS/2 driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-03 22:53 ` vishnupatekar
0 siblings, 0 replies; 22+ messages in thread
From: vishnupatekar @ 2014-12-03 22:53 UTC (permalink / raw
To: linux-arm-kernel
---
drivers/input/serio/Kconfig | 9 ++
drivers/input/serio/Makefile | 1 +
drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
3 files changed, 315 insertions(+)
create mode 100644 drivers/input/serio/sunxi-ps2.c
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..1a86e41 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
To compile this driver as a module, choose M here: the module will
be called hyperv_keyboard.
+config SERIO_SUNXI_PS2
+ tristate "Allwinner Sun7i-A20 PS/2 controller"
+ default m
+ help
+ Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sunxi-ps2.
+
endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
new file mode 100644
index 0000000..ccd7b29
--- /dev/null
+++ b/drivers/input/serio/sunxi-ps2.c
@@ -0,0 +1,305 @@
+/*
+ * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
+ *
+ * Author: Aaron.maoye <leafy.myeh@newbietech.com>
+ * Vishnu Patekar <vishnupatekar0510@gmail.com>
+ * Based on sunxi-ps2.c 3.0 kernel
+ *
+*/
+
+#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 "sunxi-ps2"
+
+#define RESSIZE(res) (((res)->end - (res)->start)+1)
+
+#define SW_PS2_GCTRL (0x00)
+#define SW_PS2_DATA (0x04)
+#define SW_PS2_LCTRL (0x08)
+#define SW_PS2_LSTAT (0x0c)
+#define SW_PS2_FCTRL (0x10)
+#define SW_PS2_FSTAT (0x14)
+#define SW_PS2_CLKDR (0x18)
+
+/* SW_PS2_GCTRL */
+#define SWPS2_BUSEN (1 << 0)
+#define SWPS2_MASTER (1 << 1)
+#define SWPS2_RESET (1 << 2)
+#define SWPS2_INTEN (1 << 3)
+#define SWPS2_INTFLAG (1 << 3)
+
+/* SW_PS2_LCTRL */
+#define SWPS2_LCTL_NOACK (0x0 << 18)
+#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
+#define SWPS2_LCTL_STOPERREN (0x1 << 3)
+#define SWPS2_LCTL_ACKERREN (0x1 << 2)
+#define SWPS2_LCTL_PARERREN (0x1 << 1)
+#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
+
+/* SW_PS2_FSTAT */
+#define SWPS2_FSTA_RXRDY (1 << 0)
+#define SWPS2_FSTA_RXOF (1 << 1)
+#define SWPS2_FSTA_RXUF (1 << 2)
+#define SWPS2_FSTA_TXRDY (1 << 8)
+#define SWPS2_FSTA_TXOF (1 << 9)
+#define SWPS2_FSTA_TXUF (1 << 10)
+
+#define SW_PS2_SAMPLE_CLK (1000000)
+#define SW_PS2_SCLK (125000)
+
+struct sunxips2data {
+ int irq;
+ spinlock_t ps2_lock;
+ void __iomem *base_address; /* virt address of control registers*/
+ struct serio *serio; /* serio*/
+ struct device *dev;
+ struct clk *pclk;
+};
+
+/*********************/
+/* Interrupt handler */
+/*********************/
+static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
+{
+ struct sunxips2data *drvdata = dev_id;
+ u32 intr_status;
+ u32 fifo_status;
+ unsigned char byte;
+ u32 rval;
+ u32 error = 0;
+
+ spin_lock(&drvdata->ps2_lock);
+
+ /* Get the PS/2 interrupts and clear them */
+ intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
+ fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
+
+ /*Check Line Status Register*/
+ if (intr_status & 0x10f) {
+ if (intr_status & 0x08)
+ dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
+ if (intr_status & 0x04)
+ dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
+ if (intr_status & 0x02)
+ dev_info(drvdata->dev, "PS/2 Parity Error!\n");
+ if (intr_status & 0x100)
+ dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
+ if (intr_status & 0x01)
+ dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
+
+ writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
+ writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
+
+ error = 1;
+ }
+
+ /*Check FIFO Status Register*/
+ if (fifo_status & 0x0606) {
+ if (fifo_status & 0x400)
+ dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
+ if (fifo_status & 0x200)
+ dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
+ if (fifo_status & 0x04)
+ dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
+ if (fifo_status & 0x02)
+ dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
+
+ writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
+ writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
+ error = 1;
+ }
+
+ rval = (fifo_status >> 16) & 0x3;
+ while (!error && rval--) {
+ byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
+ dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
+ serio_interrupt(drvdata->serio, byte, 0);
+ }
+
+ writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
+ writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
+
+ spin_unlock(&drvdata->ps2_lock);
+
+ return IRQ_HANDLED;
+}
+
+static int sunxips2_open(struct serio *pserio)
+{
+ struct sunxips2data *drvdata = pserio->port_data;
+ u32 src_clk = 0;
+ u32 clk_scdf;
+ u32 clk_pcdf;
+ u32 rval;
+
+ /*Set Line Control And Enable Interrupt*/
+ rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
+ writel(rval, drvdata->base_address + SW_PS2_LCTRL);
+
+ /*Reset FIFO*/
+ writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
+
+ src_clk = clk_get_rate(drvdata->pclk);
+
+ if (!src_clk) {
+ dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
+ return -1;
+ }
+
+ /*Set Clock Divider Register*/
+ clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
+ clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
+ rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
+ writel(rval, drvdata->base_address + SW_PS2_CLKDR);
+
+ /*Set Global Control Register*/
+ rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
+ writel(rval, drvdata->base_address + SW_PS2_GCTRL);
+
+ udelay(100);
+
+ return 0;
+}
+
+static void sunxips2_close(struct serio *pserio)
+{
+ struct sunxips2data *drvdata = pserio->port_data;
+
+ spin_lock(&drvdata->ps2_lock);
+ /* Disable the PS2 interrupts */
+ writel(0, drvdata->base_address + SW_PS2_GCTRL);
+ spin_unlock(&drvdata->ps2_lock);
+}
+
+static int sunxips2_write(struct serio *pserio, unsigned char val)
+{
+ struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
+ u32 timeout = 10000;
+
+ do {
+ if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
+ writel(val, drvdata->base_address + SW_PS2_DATA);
+ return 0;
+ }
+ } while (timeout--);
+
+ return -1;
+}
+
+static int sunxips2_probe(struct platform_device *ofdev)
+{
+ struct resource *res; /* IO mem resources */
+ struct sunxips2data *drvdata;
+ struct serio *serio;
+ struct device *dev = &ofdev->dev;
+ unsigned int irq;
+ int error;
+
+ drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
+ serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
+ if (!drvdata || !serio)
+ error = -ENOMEM;
+
+ /* Request clock */
+ drvdata->pclk = clk_get(dev, NULL);
+ if (IS_ERR(drvdata->pclk))
+ dev_dbg(dev, "couldn't get clock %li\n",
+ PTR_ERR(drvdata->pclk));
+
+ if (!IS_ERR(drvdata->pclk)) {
+ error = clk_prepare_enable(drvdata->pclk);
+ if (error < 0) {
+ dev_err(dev, "failed to enable clock %d\n", error);
+ return error;
+ }
+ }
+
+ /* IO */
+ res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+ drvdata->base_address = devm_ioremap_resource(dev, res);
+ if (IS_ERR(drvdata->base_address)) {
+ dev_err(dev, "failed to map registers\n");
+ error = PTR_ERR(drvdata->base_address);
+ }
+
+ serio->id.type = SERIO_8042;
+ serio->write = sunxips2_write;
+ serio->open = sunxips2_open;
+ serio->close = sunxips2_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));
+
+ platform_set_drvdata(ofdev, drvdata);
+ serio_register_port(serio);
+
+ /* Get IRQ for the device */
+ irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
+ if (!irq) {
+ dev_err(dev, "no IRQ found\n");
+ return -ENODEV;
+ }
+
+ drvdata->irq = irq;
+ drvdata->serio = serio;
+ drvdata->dev = dev;
+ error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
+ DRIVER_NAME, drvdata);
+ if (error) {
+ dev_err(drvdata->dev,
+ "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
+ return error;
+ }
+ return 0; /* success */
+}
+
+static int sunxips2_remove(struct platform_device *of_dev)
+{
+ struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
+
+ if (!IS_ERR(drvdata->pclk)) {
+ clk_disable_unprepare(drvdata->pclk);
+ clk_put(drvdata->pclk);
+ }
+ serio_unregister_port(drvdata->serio);
+ mdelay(2);
+ return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id sunxips2_of_match[] = {
+ { .compatible = "allwinner,sun7i-a20-ps2", },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, sunxips2_of_match);
+
+/*platform driver structure*/
+static struct platform_driver sunxips2_of_driver = {
+ .probe = sunxips2_probe,
+ .remove = sunxips2_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ .of_match_table = sunxips2_of_match,
+ },
+};
+module_platform_driver(sunxips2_of_driver);
+
+MODULE_AUTHOR("Aaron.maoye<leafy.myeh at newbietech.com, "
+ "Vishnu Patekar <vishnupatekar0510@gmail.com>");
+MODULE_DESCRIPTION("Sunxi PS/2 driver");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-03 23:23 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-03 23:23 UTC (permalink / raw
To: vishnupatekar
Cc: maxime.ripard, linux-sunxi, robh+dt, benh, msalter, ralf,
jdelvare, linux-kernel, linux-input, linux-arm-kernel, devicetree
On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> ---
> drivers/input/serio/Kconfig | 9 ++
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/input/serio/sunxi-ps2.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index bc2d474..1a86e41 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> To compile this driver as a module, choose M here: the module will
> be called hyperv_keyboard.
>
> +config SERIO_SUNXI_PS2
> + tristate "Allwinner Sun7i-A20 PS/2 controller"
> + default m
Why default m? Default should be n. Also it seems it has to depend on
OF. And what about depending on <arch> || COMPILE_TEST?
> + help
> + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-ps2.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
> new file mode 100644
> index 0000000..ccd7b29
> --- /dev/null
> +++ b/drivers/input/serio/sunxi-ps2.c
> @@ -0,0 +1,305 @@
> +/*
> + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
No file names in sources please.
> + *
> + * Author: Aaron.maoye <leafy.myeh@newbietech.com>
> + * Vishnu Patekar <vishnupatekar0510@gmail.com>
> + * Based on sunxi-ps2.c 3.0 kernel
> + *
> +*/
> +
> +#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 "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
I am sure we have something generic for it, no?
> +
> +#define SW_PS2_GCTRL (0x00)
> +#define SW_PS2_DATA (0x04)
> +#define SW_PS2_LCTRL (0x08)
> +#define SW_PS2_LSTAT (0x0c)
> +#define SW_PS2_FCTRL (0x10)
> +#define SW_PS2_FSTAT (0x14)
> +#define SW_PS2_CLKDR (0x18)
> +
> +/* SW_PS2_GCTRL */
> +#define SWPS2_BUSEN (1 << 0)
> +#define SWPS2_MASTER (1 << 1)
> +#define SWPS2_RESET (1 << 2)
> +#define SWPS2_INTEN (1 << 3)
> +#define SWPS2_INTFLAG (1 << 3)
> +
> +/* SW_PS2_LCTRL */
> +#define SWPS2_LCTL_NOACK (0x0 << 18)
> +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> +
> +/* SW_PS2_FSTAT */
> +#define SWPS2_FSTA_RXRDY (1 << 0)
> +#define SWPS2_FSTA_RXOF (1 << 1)
> +#define SWPS2_FSTA_RXUF (1 << 2)
> +#define SWPS2_FSTA_TXRDY (1 << 8)
> +#define SWPS2_FSTA_TXOF (1 << 9)
> +#define SWPS2_FSTA_TXUF (1 << 10)
> +
> +#define SW_PS2_SAMPLE_CLK (1000000)
> +#define SW_PS2_SCLK (125000)
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
> +
> +/*********************/
> +/* Interrupt handler */
> +/*********************/
> +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> +{
> + struct sunxips2data *drvdata = dev_id;
> + u32 intr_status;
> + u32 fifo_status;
> + unsigned char byte;
> + u32 rval;
> + u32 error = 0;
> +
> + spin_lock(&drvdata->ps2_lock);
> +
> + /* Get the PS/2 interrupts and clear them */
> + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> +
> + /*Check Line Status Register*/
> + if (intr_status & 0x10f) {
> + if (intr_status & 0x08)
> + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> + if (intr_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
> + if (intr_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> + if (intr_status & 0x100)
> + dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
> + if (intr_status & 0x01)
> + dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> +
> + error = 1;
> + }
> +
> + /*Check FIFO Status Register*/
> + if (fifo_status & 0x0606) {
> + if (fifo_status & 0x400)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
> + if (fifo_status & 0x200)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> + if (fifo_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
> + if (fifo_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> + error = 1;
> + }
> +
> + rval = (fifo_status >> 16) & 0x3;
> + while (!error && rval--) {
> + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
You really want to log every input byte in the logs?
> + serio_interrupt(drvdata->serio, byte, 0);
> + }
> +
> + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> +
> + spin_unlock(&drvdata->ps2_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
> +
> + return 0;
> +}
> +
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
> +
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
> +
> + return -1;
> +}
> +
> +static int sunxips2_probe(struct platform_device *ofdev)
> +{
> + struct resource *res; /* IO mem resources */
> + struct sunxips2data *drvdata;
> + struct serio *serio;
> + struct device *dev = &ofdev->dev;
> + unsigned int irq;
> + int error;
> +
> + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
> + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
serio is a refcounted structure, it should not be allocated with devm.
> + if (!drvdata || !serio)
> + error = -ENOMEM;
and...?
> +
> + /* Request clock */
> + drvdata->pclk = clk_get(dev, NULL);
> + if (IS_ERR(drvdata->pclk))
> + dev_dbg(dev, "couldn't get clock %li\n",
> + PTR_ERR(drvdata->pclk));
Why %li?
> +
> + if (!IS_ERR(drvdata->pclk)) {
"} else {"
> + error = clk_prepare_enable(drvdata->pclk);
> + if (error < 0) {
> + dev_err(dev, "failed to enable clock %d\n", error);
> + return error;
Leaking clk here.
> + }
> + }
> +
> + /* IO */
> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> + drvdata->base_address = devm_ioremap_resource(dev, res);
> + if (IS_ERR(drvdata->base_address)) {
> + dev_err(dev, "failed to map registers\n");
> + error = PTR_ERR(drvdata->base_address);
aaaand...?
> + }
> +
> + serio->id.type = SERIO_8042;
> + serio->write = sunxips2_write;
> + serio->open = sunxips2_open;
> + serio->close = sunxips2_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));
> +
> + platform_set_drvdata(ofdev, drvdata);
> + serio_register_port(serio);
> +
> + /* Get IRQ for the device */
> + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (!irq) {
> + dev_err(dev, "no IRQ found\n");
> + return -ENODEV;
Leaking clk and serio here.
> + }
> +
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
Please try to fit in 80 columns.
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
> +
> +static int sunxips2_remove(struct platform_device *of_dev)
> +{
> + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> +
> + if (!IS_ERR(drvdata->pclk)) {
> + clk_disable_unprepare(drvdata->pclk);
> + clk_put(drvdata->pclk);
If serio write comes here what will happen?
> + }
> + serio_unregister_port(drvdata->serio);
If interrupt comes here what will happen?
> + mdelay(2);
Why do you need mdelay?
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id sunxips2_of_match[] = {
> + { .compatible = "allwinner,sun7i-a20-ps2", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> +
> +/*platform driver structure*/
> +static struct platform_driver sunxips2_of_driver = {
> + .probe = sunxips2_probe,
> + .remove = sunxips2_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = sunxips2_of_match,
> + },
> +};
> +module_platform_driver(sunxips2_of_driver);
> +
> +MODULE_AUTHOR("Aaron.maoye<leafy.myeh@newbietech.com, "
Space before "<", closing ">" are missing.
> + "Vishnu Patekar <vishnupatekar0510@gmail.com>");
> +MODULE_DESCRIPTION("Sunxi PS/2 driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-03 23:23 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-03 23:23 UTC (permalink / raw
To: vishnupatekar
Cc: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
msalter-H+wXaHxf7aLQT0dZR+AlfA, ralf-6z/3iImG2C8G8FEW9MqTrA,
jdelvare-l3A5Bk7waGM, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> ---
> drivers/input/serio/Kconfig | 9 ++
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/input/serio/sunxi-ps2.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index bc2d474..1a86e41 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> To compile this driver as a module, choose M here: the module will
> be called hyperv_keyboard.
>
> +config SERIO_SUNXI_PS2
> + tristate "Allwinner Sun7i-A20 PS/2 controller"
> + default m
Why default m? Default should be n. Also it seems it has to depend on
OF. And what about depending on <arch> || COMPILE_TEST?
> + help
> + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-ps2.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
> new file mode 100644
> index 0000000..ccd7b29
> --- /dev/null
> +++ b/drivers/input/serio/sunxi-ps2.c
> @@ -0,0 +1,305 @@
> +/*
> + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
No file names in sources please.
> + *
> + * Author: Aaron.maoye <leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org>
> + * Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * Based on sunxi-ps2.c 3.0 kernel
> + *
> +*/
> +
> +#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 "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
I am sure we have something generic for it, no?
> +
> +#define SW_PS2_GCTRL (0x00)
> +#define SW_PS2_DATA (0x04)
> +#define SW_PS2_LCTRL (0x08)
> +#define SW_PS2_LSTAT (0x0c)
> +#define SW_PS2_FCTRL (0x10)
> +#define SW_PS2_FSTAT (0x14)
> +#define SW_PS2_CLKDR (0x18)
> +
> +/* SW_PS2_GCTRL */
> +#define SWPS2_BUSEN (1 << 0)
> +#define SWPS2_MASTER (1 << 1)
> +#define SWPS2_RESET (1 << 2)
> +#define SWPS2_INTEN (1 << 3)
> +#define SWPS2_INTFLAG (1 << 3)
> +
> +/* SW_PS2_LCTRL */
> +#define SWPS2_LCTL_NOACK (0x0 << 18)
> +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> +
> +/* SW_PS2_FSTAT */
> +#define SWPS2_FSTA_RXRDY (1 << 0)
> +#define SWPS2_FSTA_RXOF (1 << 1)
> +#define SWPS2_FSTA_RXUF (1 << 2)
> +#define SWPS2_FSTA_TXRDY (1 << 8)
> +#define SWPS2_FSTA_TXOF (1 << 9)
> +#define SWPS2_FSTA_TXUF (1 << 10)
> +
> +#define SW_PS2_SAMPLE_CLK (1000000)
> +#define SW_PS2_SCLK (125000)
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
> +
> +/*********************/
> +/* Interrupt handler */
> +/*********************/
> +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> +{
> + struct sunxips2data *drvdata = dev_id;
> + u32 intr_status;
> + u32 fifo_status;
> + unsigned char byte;
> + u32 rval;
> + u32 error = 0;
> +
> + spin_lock(&drvdata->ps2_lock);
> +
> + /* Get the PS/2 interrupts and clear them */
> + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> +
> + /*Check Line Status Register*/
> + if (intr_status & 0x10f) {
> + if (intr_status & 0x08)
> + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> + if (intr_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
> + if (intr_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> + if (intr_status & 0x100)
> + dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
> + if (intr_status & 0x01)
> + dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> +
> + error = 1;
> + }
> +
> + /*Check FIFO Status Register*/
> + if (fifo_status & 0x0606) {
> + if (fifo_status & 0x400)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
> + if (fifo_status & 0x200)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> + if (fifo_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
> + if (fifo_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> + error = 1;
> + }
> +
> + rval = (fifo_status >> 16) & 0x3;
> + while (!error && rval--) {
> + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
You really want to log every input byte in the logs?
> + serio_interrupt(drvdata->serio, byte, 0);
> + }
> +
> + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> +
> + spin_unlock(&drvdata->ps2_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
> +
> + return 0;
> +}
> +
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
> +
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
> +
> + return -1;
> +}
> +
> +static int sunxips2_probe(struct platform_device *ofdev)
> +{
> + struct resource *res; /* IO mem resources */
> + struct sunxips2data *drvdata;
> + struct serio *serio;
> + struct device *dev = &ofdev->dev;
> + unsigned int irq;
> + int error;
> +
> + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
> + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
serio is a refcounted structure, it should not be allocated with devm.
> + if (!drvdata || !serio)
> + error = -ENOMEM;
and...?
> +
> + /* Request clock */
> + drvdata->pclk = clk_get(dev, NULL);
> + if (IS_ERR(drvdata->pclk))
> + dev_dbg(dev, "couldn't get clock %li\n",
> + PTR_ERR(drvdata->pclk));
Why %li?
> +
> + if (!IS_ERR(drvdata->pclk)) {
"} else {"
> + error = clk_prepare_enable(drvdata->pclk);
> + if (error < 0) {
> + dev_err(dev, "failed to enable clock %d\n", error);
> + return error;
Leaking clk here.
> + }
> + }
> +
> + /* IO */
> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> + drvdata->base_address = devm_ioremap_resource(dev, res);
> + if (IS_ERR(drvdata->base_address)) {
> + dev_err(dev, "failed to map registers\n");
> + error = PTR_ERR(drvdata->base_address);
aaaand...?
> + }
> +
> + serio->id.type = SERIO_8042;
> + serio->write = sunxips2_write;
> + serio->open = sunxips2_open;
> + serio->close = sunxips2_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));
> +
> + platform_set_drvdata(ofdev, drvdata);
> + serio_register_port(serio);
> +
> + /* Get IRQ for the device */
> + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (!irq) {
> + dev_err(dev, "no IRQ found\n");
> + return -ENODEV;
Leaking clk and serio here.
> + }
> +
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
Please try to fit in 80 columns.
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
> +
> +static int sunxips2_remove(struct platform_device *of_dev)
> +{
> + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> +
> + if (!IS_ERR(drvdata->pclk)) {
> + clk_disable_unprepare(drvdata->pclk);
> + clk_put(drvdata->pclk);
If serio write comes here what will happen?
> + }
> + serio_unregister_port(drvdata->serio);
If interrupt comes here what will happen?
> + mdelay(2);
Why do you need mdelay?
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id sunxips2_of_match[] = {
> + { .compatible = "allwinner,sun7i-a20-ps2", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> +
> +/*platform driver structure*/
> +static struct platform_driver sunxips2_of_driver = {
> + .probe = sunxips2_probe,
> + .remove = sunxips2_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = sunxips2_of_match,
> + },
> +};
> +module_platform_driver(sunxips2_of_driver);
> +
> +MODULE_AUTHOR("Aaron.maoye<leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org, "
Space before "<", closing ">" are missing.
> + "Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("Sunxi PS/2 driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-03 23:23 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-03 23:23 UTC (permalink / raw
To: linux-arm-kernel
On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> ---
> drivers/input/serio/Kconfig | 9 ++
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/input/serio/sunxi-ps2.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index bc2d474..1a86e41 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> To compile this driver as a module, choose M here: the module will
> be called hyperv_keyboard.
>
> +config SERIO_SUNXI_PS2
> + tristate "Allwinner Sun7i-A20 PS/2 controller"
> + default m
Why default m? Default should be n. Also it seems it has to depend on
OF. And what about depending on <arch> || COMPILE_TEST?
> + help
> + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-ps2.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
> new file mode 100644
> index 0000000..ccd7b29
> --- /dev/null
> +++ b/drivers/input/serio/sunxi-ps2.c
> @@ -0,0 +1,305 @@
> +/*
> + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
No file names in sources please.
> + *
> + * Author: Aaron.maoye <leafy.myeh@newbietech.com>
> + * Vishnu Patekar <vishnupatekar0510@gmail.com>
> + * Based on sunxi-ps2.c 3.0 kernel
> + *
> +*/
> +
> +#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 "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
I am sure we have something generic for it, no?
> +
> +#define SW_PS2_GCTRL (0x00)
> +#define SW_PS2_DATA (0x04)
> +#define SW_PS2_LCTRL (0x08)
> +#define SW_PS2_LSTAT (0x0c)
> +#define SW_PS2_FCTRL (0x10)
> +#define SW_PS2_FSTAT (0x14)
> +#define SW_PS2_CLKDR (0x18)
> +
> +/* SW_PS2_GCTRL */
> +#define SWPS2_BUSEN (1 << 0)
> +#define SWPS2_MASTER (1 << 1)
> +#define SWPS2_RESET (1 << 2)
> +#define SWPS2_INTEN (1 << 3)
> +#define SWPS2_INTFLAG (1 << 3)
> +
> +/* SW_PS2_LCTRL */
> +#define SWPS2_LCTL_NOACK (0x0 << 18)
> +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> +
> +/* SW_PS2_FSTAT */
> +#define SWPS2_FSTA_RXRDY (1 << 0)
> +#define SWPS2_FSTA_RXOF (1 << 1)
> +#define SWPS2_FSTA_RXUF (1 << 2)
> +#define SWPS2_FSTA_TXRDY (1 << 8)
> +#define SWPS2_FSTA_TXOF (1 << 9)
> +#define SWPS2_FSTA_TXUF (1 << 10)
> +
> +#define SW_PS2_SAMPLE_CLK (1000000)
> +#define SW_PS2_SCLK (125000)
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
> +
> +/*********************/
> +/* Interrupt handler */
> +/*********************/
> +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> +{
> + struct sunxips2data *drvdata = dev_id;
> + u32 intr_status;
> + u32 fifo_status;
> + unsigned char byte;
> + u32 rval;
> + u32 error = 0;
> +
> + spin_lock(&drvdata->ps2_lock);
> +
> + /* Get the PS/2 interrupts and clear them */
> + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> +
> + /*Check Line Status Register*/
> + if (intr_status & 0x10f) {
> + if (intr_status & 0x08)
> + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> + if (intr_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
> + if (intr_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> + if (intr_status & 0x100)
> + dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
> + if (intr_status & 0x01)
> + dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> +
> + error = 1;
> + }
> +
> + /*Check FIFO Status Register*/
> + if (fifo_status & 0x0606) {
> + if (fifo_status & 0x400)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
> + if (fifo_status & 0x200)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> + if (fifo_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
> + if (fifo_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> + error = 1;
> + }
> +
> + rval = (fifo_status >> 16) & 0x3;
> + while (!error && rval--) {
> + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
You really want to log every input byte in the logs?
> + serio_interrupt(drvdata->serio, byte, 0);
> + }
> +
> + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> +
> + spin_unlock(&drvdata->ps2_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
> +
> + return 0;
> +}
> +
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
> +
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
> +
> + return -1;
> +}
> +
> +static int sunxips2_probe(struct platform_device *ofdev)
> +{
> + struct resource *res; /* IO mem resources */
> + struct sunxips2data *drvdata;
> + struct serio *serio;
> + struct device *dev = &ofdev->dev;
> + unsigned int irq;
> + int error;
> +
> + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
> + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
serio is a refcounted structure, it should not be allocated with devm.
> + if (!drvdata || !serio)
> + error = -ENOMEM;
and...?
> +
> + /* Request clock */
> + drvdata->pclk = clk_get(dev, NULL);
> + if (IS_ERR(drvdata->pclk))
> + dev_dbg(dev, "couldn't get clock %li\n",
> + PTR_ERR(drvdata->pclk));
Why %li?
> +
> + if (!IS_ERR(drvdata->pclk)) {
"} else {"
> + error = clk_prepare_enable(drvdata->pclk);
> + if (error < 0) {
> + dev_err(dev, "failed to enable clock %d\n", error);
> + return error;
Leaking clk here.
> + }
> + }
> +
> + /* IO */
> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> + drvdata->base_address = devm_ioremap_resource(dev, res);
> + if (IS_ERR(drvdata->base_address)) {
> + dev_err(dev, "failed to map registers\n");
> + error = PTR_ERR(drvdata->base_address);
aaaand...?
> + }
> +
> + serio->id.type = SERIO_8042;
> + serio->write = sunxips2_write;
> + serio->open = sunxips2_open;
> + serio->close = sunxips2_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));
> +
> + platform_set_drvdata(ofdev, drvdata);
> + serio_register_port(serio);
> +
> + /* Get IRQ for the device */
> + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (!irq) {
> + dev_err(dev, "no IRQ found\n");
> + return -ENODEV;
Leaking clk and serio here.
> + }
> +
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
Please try to fit in 80 columns.
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
> +
> +static int sunxips2_remove(struct platform_device *of_dev)
> +{
> + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> +
> + if (!IS_ERR(drvdata->pclk)) {
> + clk_disable_unprepare(drvdata->pclk);
> + clk_put(drvdata->pclk);
If serio write comes here what will happen?
> + }
> + serio_unregister_port(drvdata->serio);
If interrupt comes here what will happen?
> + mdelay(2);
Why do you need mdelay?
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id sunxips2_of_match[] = {
> + { .compatible = "allwinner,sun7i-a20-ps2", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> +
> +/*platform driver structure*/
> +static struct platform_driver sunxips2_of_driver = {
> + .probe = sunxips2_probe,
> + .remove = sunxips2_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = sunxips2_of_match,
> + },
> +};
> +module_platform_driver(sunxips2_of_driver);
> +
> +MODULE_AUTHOR("Aaron.maoye<leafy.myeh@newbietech.com, "
Space before "<", closing ">" are missing.
> + "Vishnu Patekar <vishnupatekar0510@gmail.com>");
> +MODULE_DESCRIPTION("Sunxi PS/2 driver");
> +MODULE_LICENSE("GPL");
> --
> 1.7.9.5
>
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
2014-12-03 23:23 ` Dmitry Torokhov
(?)
(?)
@ 2014-12-04 3:08 ` Vishnu Patekar
-1 siblings, 0 replies; 22+ messages in thread
From: Vishnu Patekar @ 2014-12-04 3:08 UTC (permalink / raw
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
Cc: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org,
msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org,
jdelvare-l3A5Bk7waGM@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
[-- Attachment #1: Type: text/plain, Size: 14852 bytes --]
Hello Dmitry,
Thank you for comments.
I'll implement these comments and resend it for review.
On Thursday, December 4, 2014, Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:
> On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> > ---
> > drivers/input/serio/Kconfig | 9 ++
> > drivers/input/serio/Makefile | 1 +
> > drivers/input/serio/sunxi-ps2.c | 305
> +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 315 insertions(+)
> > create mode 100644 drivers/input/serio/sunxi-ps2.c
> >
> > diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> > index bc2d474..1a86e41 100644
> > --- a/drivers/input/serio/Kconfig
> > +++ b/drivers/input/serio/Kconfig
> > @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> > To compile this driver as a module, choose M here: the module
> will
> > be called hyperv_keyboard.
> >
> > +config SERIO_SUNXI_PS2
> > + tristate "Allwinner Sun7i-A20 PS/2 controller"
> > + default m
>
> Why default m? Default should be n. Also it seems it has to depend on
> OF. And what about depending on <arch> || COMPILE_TEST?
>
> > + help
> > + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called sunxi-ps2.
> > +
> > endif
> > diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> > index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> > diff --git a/drivers/input/serio/sunxi-ps2.c
> b/drivers/input/serio/sunxi-ps2.c
> > new file mode 100644
> > index 0000000..ccd7b29
> > --- /dev/null
> > +++ b/drivers/input/serio/sunxi-ps2.c
> > @@ -0,0 +1,305 @@
> > +/*
> > + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
>
> No file names in sources please.
>
> > + *
> > + * Author: Aaron.maoye <leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org <javascript:;>>
> > + * Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <javascript:;>>
> > + * Based on sunxi-ps2.c 3.0 kernel
> > + *
> > +*/
> > +
> > +#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 "sunxi-ps2"
> > +
> > +#define RESSIZE(res) (((res)->end - (res)->start)+1)
>
> I am sure we have something generic for it, no?
>
> > +
> > +#define SW_PS2_GCTRL (0x00)
> > +#define SW_PS2_DATA (0x04)
> > +#define SW_PS2_LCTRL (0x08)
> > +#define SW_PS2_LSTAT (0x0c)
> > +#define SW_PS2_FCTRL (0x10)
> > +#define SW_PS2_FSTAT (0x14)
> > +#define SW_PS2_CLKDR (0x18)
> > +
> > +/* SW_PS2_GCTRL */
> > +#define SWPS2_BUSEN (1 << 0)
> > +#define SWPS2_MASTER (1 << 1)
> > +#define SWPS2_RESET (1 << 2)
> > +#define SWPS2_INTEN (1 << 3)
> > +#define SWPS2_INTFLAG (1 << 3)
> > +
> > +/* SW_PS2_LCTRL */
> > +#define SWPS2_LCTL_NOACK (0x0 << 18)
> > +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> > +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> > +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> > +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> > +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> > +
> > +/* SW_PS2_FSTAT */
> > +#define SWPS2_FSTA_RXRDY (1 << 0)
> > +#define SWPS2_FSTA_RXOF (1 << 1)
> > +#define SWPS2_FSTA_RXUF (1 << 2)
> > +#define SWPS2_FSTA_TXRDY (1 << 8)
> > +#define SWPS2_FSTA_TXOF (1 << 9)
> > +#define SWPS2_FSTA_TXUF (1 << 10)
> > +
> > +#define SW_PS2_SAMPLE_CLK (1000000)
> > +#define SW_PS2_SCLK (125000)
> > +
> > +struct sunxips2data {
> > + int irq;
> > + spinlock_t ps2_lock;
> > + void __iomem *base_address; /* virt address of control
> registers*/
> > + struct serio *serio; /* serio*/
> > + struct device *dev;
> > + struct clk *pclk;
> > +};
> > +
> > +/*********************/
> > +/* Interrupt handler */
> > +/*********************/
> > +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> > +{
> > + struct sunxips2data *drvdata = dev_id;
> > + u32 intr_status;
> > + u32 fifo_status;
> > + unsigned char byte;
> > + u32 rval;
> > + u32 error = 0;
> > +
> > + spin_lock(&drvdata->ps2_lock);
> > +
> > + /* Get the PS/2 interrupts and clear them */
> > + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> > + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> > +
> > + /*Check Line Status Register*/
> > + if (intr_status & 0x10f) {
> > + if (intr_status & 0x08)
> > + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> > + if (intr_status & 0x04)
> > + dev_info(drvdata->dev, "PS/2 Acknowledge
> Error!\n");
> > + if (intr_status & 0x02)
> > + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> > + if (intr_status & 0x100)
> > + dev_info(drvdata->dev, "PS/2 Transmit Data
> Timeout!\n");
> > + if (intr_status & 0x01)
> > + dev_info(drvdata->dev, "PS/2 Receive Data
> Timeout!\n");
> > +
> > + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4,
> drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> > + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> > +
> > + error = 1;
> > + }
> > +
> > + /*Check FIFO Status Register*/
> > + if (fifo_status & 0x0606) {
> > + if (fifo_status & 0x400)
> > + dev_info(drvdata->dev, "PS/2 Tx FIFO
> Underflow!\n");
> > + if (fifo_status & 0x200)
> > + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> > + if (fifo_status & 0x04)
> > + dev_info(drvdata->dev, "PS/2 Rx FIFO
> Underflow!\n");
> > + if (fifo_status & 0x02)
> > + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> > +
> > + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4,
> drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> > + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> > + error = 1;
> > + }
> > +
> > + rval = (fifo_status >> 16) & 0x3;
> > + while (!error && rval--) {
> > + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> > + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
>
> You really want to log every input byte in the logs?
>
> > + serio_interrupt(drvdata->serio, byte, 0);
> > + }
> > +
> > + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> > + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> > +
> > + spin_unlock(&drvdata->ps2_lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int sunxips2_open(struct serio *pserio)
> > +{
> > + struct sunxips2data *drvdata = pserio->port_data;
> > + u32 src_clk = 0;
> > + u32 clk_scdf;
> > + u32 clk_pcdf;
> > + u32 rval;
> > +
> > + /*Set Line Control And Enable Interrupt*/
> > + rval =
> SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> > + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> > +
> > + /*Reset FIFO*/
> > + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> > +
> > + src_clk = clk_get_rate(drvdata->pclk);
> > +
> > + if (!src_clk) {
> > + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source
> clock is 0.");
> > + return -1;
> > + }
> > +
> > + /*Set Clock Divider Register*/
> > + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK
> - 1);
> > + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK -
> 1);
> > + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> > + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> > +
> > + /*Set Global Control Register*/
> > + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> > + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> > +
> > + udelay(100);
> > +
> > + return 0;
> > +}
> > +
> > +static void sunxips2_close(struct serio *pserio)
> > +{
> > + struct sunxips2data *drvdata = pserio->port_data;
> > +
> > + spin_lock(&drvdata->ps2_lock);
> > + /* Disable the PS2 interrupts */
> > + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> > + spin_unlock(&drvdata->ps2_lock);
> > +}
> > +
> > +static int sunxips2_write(struct serio *pserio, unsigned char val)
> > +{
> > + struct sunxips2data *drvdata = (struct sunxips2data
> *)pserio->port_data;
> > + u32 timeout = 10000;
> > +
> > + do {
> > + if (readl(drvdata->base_address + SW_PS2_FSTAT) &
> SWPS2_FSTA_TXRDY) {
> > + writel(val, drvdata->base_address + SW_PS2_DATA);
> > + return 0;
> > + }
> > + } while (timeout--);
> > +
> > + return -1;
> > +}
> > +
> > +static int sunxips2_probe(struct platform_device *ofdev)
> > +{
> > + struct resource *res; /* IO mem resources */
> > + struct sunxips2data *drvdata;
> > + struct serio *serio;
> > + struct device *dev = &ofdev->dev;
> > + unsigned int irq;
> > + int error;
> > +
> > + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data),
> GFP_KERNEL);
> > + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
>
> serio is a refcounted structure, it should not be allocated with devm.
>
> > + if (!drvdata || !serio)
> > + error = -ENOMEM;
>
> and...?
>
> > +
> > + /* Request clock */
> > + drvdata->pclk = clk_get(dev, NULL);
> > + if (IS_ERR(drvdata->pclk))
> > + dev_dbg(dev, "couldn't get clock %li\n",
> > + PTR_ERR(drvdata->pclk));
>
> Why %li?
>
> > +
> > + if (!IS_ERR(drvdata->pclk)) {
>
> "} else {"
>
>
> > + error = clk_prepare_enable(drvdata->pclk);
> > + if (error < 0) {
> > + dev_err(dev, "failed to enable clock %d\n", error);
> > + return error;
>
> Leaking clk here.
>
> > + }
> > + }
> > +
> > + /* IO */
> > + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> > + drvdata->base_address = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(drvdata->base_address)) {
> > + dev_err(dev, "failed to map registers\n");
> > + error = PTR_ERR(drvdata->base_address);
>
> aaaand...?
>
> > + }
> > +
> > + serio->id.type = SERIO_8042;
> > + serio->write = sunxips2_write;
> > + serio->open = sunxips2_open;
> > + serio->close = sunxips2_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));
> > +
> > + platform_set_drvdata(ofdev, drvdata);
> > + serio_register_port(serio);
> > +
> > + /* Get IRQ for the device */
> > + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> > + if (!irq) {
> > + dev_err(dev, "no IRQ found\n");
> > + return -ENODEV;
>
> Leaking clk and serio here.
>
> > + }
> > +
> > + drvdata->irq = irq;
> > + drvdata->serio = serio;
> > + drvdata->dev = dev;
> > + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq,
> &sunxips2_interrupt, 0,
> > + DRIVER_NAME, drvdata);
>
> Please try to fit in 80 columns.
>
> > + if (error) {
> > + dev_err(drvdata->dev,
> > + "Couldn't allocate interrupt %d : error: %d\n",
> drvdata->irq, error);
> > + return error;
> > + }
> > + return 0; /* success */
> > +}
> > +
> > +static int sunxips2_remove(struct platform_device *of_dev)
> > +{
> > + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> > +
> > + if (!IS_ERR(drvdata->pclk)) {
> > + clk_disable_unprepare(drvdata->pclk);
> > + clk_put(drvdata->pclk);
>
> If serio write comes here what will happen?
>
> > + }
> > + serio_unregister_port(drvdata->serio);
>
> If interrupt comes here what will happen?
>
> > + mdelay(2);
>
> Why do you need mdelay?
>
> > + return 0;
> > +}
> > +
> > +/* Match table for of_platform binding */
> > +static const struct of_device_id sunxips2_of_match[] = {
> > + { .compatible = "allwinner,sun7i-a20-ps2", },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> > +
> > +/*platform driver structure*/
> > +static struct platform_driver sunxips2_of_driver = {
> > + .probe = sunxips2_probe,
> > + .remove = sunxips2_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = sunxips2_of_match,
> > + },
> > +};
> > +module_platform_driver(sunxips2_of_driver);
> > +
> > +MODULE_AUTHOR("Aaron.maoye<leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org <javascript:;>, "
>
> Space before "<", closing ">" are missing.
>
> > + "Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> <javascript:;>>");
> > +MODULE_DESCRIPTION("Sunxi PS/2 driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.7.9.5
> >
>
> Thanks.
>
> --
> Dmitry
>
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org <javascript:;>.
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: Type: text/html, Size: 19954 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
2014-12-03 22:53 ` vishnupatekar
@ 2014-12-05 10:01 ` Maxime Ripard
-1 siblings, 0 replies; 22+ messages in thread
From: Maxime Ripard @ 2014-12-05 10:01 UTC (permalink / raw
To: vishnupatekar
Cc: linux-sunxi, robh+dt, benh, msalter, ralf, jdelvare, linux-kernel,
linux-input, linux-arm-kernel, devicetree, dmitry.torokhov
[-- Attachment #1: Type: text/plain, Size: 13381 bytes --]
Hi,
Thanks for working on this.
First, make sure to read Documentation/SubmittingPatches. The
guidelines in there might seem tedious, but it's there to enforce that
we keep the kernel homogenous.
Most notably, you should make sure that:
- you run scripts/checkpatch.pl on your patches
- you have a commit log
- your commit title stays below 80 chars
- you have a Signed-off-by in your commit log
On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> ---
> drivers/input/serio/Kconfig | 9 ++
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/input/serio/sunxi-ps2.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index bc2d474..1a86e41 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> To compile this driver as a module, choose M here: the module will
> be called hyperv_keyboard.
>
> +config SERIO_SUNXI_PS2
> + tristate "Allwinner Sun7i-A20 PS/2 controller"
> + default m
> + help
> + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-ps2.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
> new file mode 100644
> index 0000000..ccd7b29
> --- /dev/null
> +++ b/drivers/input/serio/sunxi-ps2.c
> @@ -0,0 +1,305 @@
> +/*
> + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
> + *
> + * Author: Aaron.maoye <leafy.myeh@newbietech.com>
> + * Vishnu Patekar <vishnupatekar0510@gmail.com>
> + * Based on sunxi-ps2.c 3.0 kernel
Mentionning that it's Allwinner's 3.0 kernel would be good, it doesn't
make much sense otherwise.
> +#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 "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
> +
> +#define SW_PS2_GCTRL (0x00)
> +#define SW_PS2_DATA (0x04)
> +#define SW_PS2_LCTRL (0x08)
> +#define SW_PS2_LSTAT (0x0c)
> +#define SW_PS2_FCTRL (0x10)
> +#define SW_PS2_FSTAT (0x14)
> +#define SW_PS2_CLKDR (0x18)
The SW_ prefix is a left over from Allwinner I suppose?
Using a prefix like sun4i_ps2/sun7i_ps2 depending on the outcome of
the discussion you had with Hans would seem better, across all your
driver.
> +
> +/* SW_PS2_GCTRL */
> +#define SWPS2_BUSEN (1 << 0)
> +#define SWPS2_MASTER (1 << 1)
> +#define SWPS2_RESET (1 << 2)
> +#define SWPS2_INTEN (1 << 3)
We usually have the convention of having the register name mentionned
in the bits offsets, and the bits declared right after their register.
Something like:
#define SUN4I_PS2_GCTRL_REG 0x00
#define SUN4I_PS2_GCTRL_BUS_EN BIT(0)
#define SUN4I_PS2_LCTRL_REG 0x08
#define SUN4I_PS2_LCTRL_PAR_ERR_EN BIT(1)
etc....
> +#define SWPS2_INTFLAG (1 << 3)
This is defined to the same value than _INTEN, is this intentionnal?
> +
> +/* SW_PS2_LCTRL */
> +#define SWPS2_LCTL_NOACK (0x0 << 18)
> +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> +
> +/* SW_PS2_FSTAT */
> +#define SWPS2_FSTA_RXRDY (1 << 0)
> +#define SWPS2_FSTA_RXOF (1 << 1)
> +#define SWPS2_FSTA_RXUF (1 << 2)
> +#define SWPS2_FSTA_TXRDY (1 << 8)
> +#define SWPS2_FSTA_TXOF (1 << 9)
> +#define SWPS2_FSTA_TXUF (1 << 10)
> +
> +#define SW_PS2_SAMPLE_CLK (1000000)
> +#define SW_PS2_SCLK (125000)
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
> +
> +/*********************/
> +/* Interrupt handler */
> +/*********************/
> +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> +{
> + struct sunxips2data *drvdata = dev_id;
> + u32 intr_status;
> + u32 fifo_status;
> + unsigned char byte;
> + u32 rval;
> + u32 error = 0;
> +
> + spin_lock(&drvdata->ps2_lock);
> +
> + /* Get the PS/2 interrupts and clear them */
> + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> +
> + /*Check Line Status Register*/
> + if (intr_status & 0x10f) {
> + if (intr_status & 0x08)
> + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> + if (intr_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
> + if (intr_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> + if (intr_status & 0x100)
> + dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
> + if (intr_status & 0x01)
> + dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> +
> + error = 1;
> + }
> +
> + /*Check FIFO Status Register*/
> + if (fifo_status & 0x0606) {
> + if (fifo_status & 0x400)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
> + if (fifo_status & 0x200)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> + if (fifo_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
> + if (fifo_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> + error = 1;
> + }
> +
> + rval = (fifo_status >> 16) & 0x3;
> + while (!error && rval--) {
> + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
> + serio_interrupt(drvdata->serio, byte, 0);
> + }
> +
> + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> +
> + spin_unlock(&drvdata->ps2_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
What is this supposed to do? Calculating and rounding the dividers?
You should look into DIV_ROUND_* if that's so
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
Why is that udelay needed?
> +
> + return 0;
> +}
> +
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
spin_lock_irqsave would be better I guess.
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
> +
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
Please use time_before() here
> + return -1;
And return a meaningful error.
> +static int sunxips2_probe(struct platform_device *ofdev)
The ofdev name is measleading. This is a platform_device structure, it
could be probed by other mechanisms than OF, so please use a different
name here. Usually pdev is used.
> +{
> + struct resource *res; /* IO mem resources */
> + struct sunxips2data *drvdata;
> + struct serio *serio;
> + struct device *dev = &ofdev->dev;
> + unsigned int irq;
> + int error;
> +
> + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
> + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
> + if (!drvdata || !serio)
> + error = -ENOMEM;
> +
> + /* Request clock */
> + drvdata->pclk = clk_get(dev, NULL);
You can use devm_clk_get.
> + if (IS_ERR(drvdata->pclk))
> + dev_dbg(dev, "couldn't get clock %li\n",
> + PTR_ERR(drvdata->pclk));
This is an error, you should treat it as such.
> + if (!IS_ERR(drvdata->pclk)) {
> + error = clk_prepare_enable(drvdata->pclk);
> + if (error < 0) {
> + dev_err(dev, "failed to enable clock %d\n", error);
> + return error;
> + }
> + }
> +
> + /* IO */
> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> + drvdata->base_address = devm_ioremap_resource(dev, res);
> + if (IS_ERR(drvdata->base_address)) {
> + dev_err(dev, "failed to map registers\n");
> + error = PTR_ERR(drvdata->base_address);
> + }
ditto.
> + serio->id.type = SERIO_8042;
> + serio->write = sunxips2_write;
> + serio->open = sunxips2_open;
> + serio->close = sunxips2_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));
> +
> + platform_set_drvdata(ofdev, drvdata);
> + serio_register_port(serio);
> +
> + /* Get IRQ for the device */
> + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (!irq) {
> + dev_err(dev, "no IRQ found\n");
> + return -ENODEV;
> + }
You can use platform_get_irq, for consistency with how you retrieve
resources.
You're also no unregistering the driver from the serio framework.
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
request_irq is enough here.
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
> +
> +static int sunxips2_remove(struct platform_device *of_dev)
> +{
> + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> +
> + if (!IS_ERR(drvdata->pclk)) {
> + clk_disable_unprepare(drvdata->pclk);
> + clk_put(drvdata->pclk);
> + }
> + serio_unregister_port(drvdata->serio);
> + mdelay(2);
Why a mdelay?
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id sunxips2_of_match[] = {
> + { .compatible = "allwinner,sun7i-a20-ps2", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> +
> +/*platform driver structure*/
> +static struct platform_driver sunxips2_of_driver = {
> + .probe = sunxips2_probe,
> + .remove = sunxips2_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
You can drop the owner field, it's already set by
module_platform_driver.
> + .of_match_table = sunxips2_of_match,
> + },
> +};
> +module_platform_driver(sunxips2_of_driver);
> +
> +MODULE_AUTHOR("Aaron.maoye<leafy.myeh@newbietech.com, "
> + "Vishnu Patekar <vishnupatekar0510@gmail.com>");
Usually, you define two MODULE_AUTHOR.
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 --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 10:01 ` Maxime Ripard
0 siblings, 0 replies; 22+ messages in thread
From: Maxime Ripard @ 2014-12-05 10:01 UTC (permalink / raw
To: linux-arm-kernel
Hi,
Thanks for working on this.
First, make sure to read Documentation/SubmittingPatches. The
guidelines in there might seem tedious, but it's there to enforce that
we keep the kernel homogenous.
Most notably, you should make sure that:
- you run scripts/checkpatch.pl on your patches
- you have a commit log
- your commit title stays below 80 chars
- you have a Signed-off-by in your commit log
On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> ---
> drivers/input/serio/Kconfig | 9 ++
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/sunxi-ps2.c | 305 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 315 insertions(+)
> create mode 100644 drivers/input/serio/sunxi-ps2.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index bc2d474..1a86e41 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> To compile this driver as a module, choose M here: the module will
> be called hyperv_keyboard.
>
> +config SERIO_SUNXI_PS2
> + tristate "Allwinner Sun7i-A20 PS/2 controller"
> + default m
> + help
> + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sunxi-ps2.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> diff --git a/drivers/input/serio/sunxi-ps2.c b/drivers/input/serio/sunxi-ps2.c
> new file mode 100644
> index 0000000..ccd7b29
> --- /dev/null
> +++ b/drivers/input/serio/sunxi-ps2.c
> @@ -0,0 +1,305 @@
> +/*
> + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
> + *
> + * Author: Aaron.maoye <leafy.myeh@newbietech.com>
> + * Vishnu Patekar <vishnupatekar0510@gmail.com>
> + * Based on sunxi-ps2.c 3.0 kernel
Mentionning that it's Allwinner's 3.0 kernel would be good, it doesn't
make much sense otherwise.
> +#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 "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
> +
> +#define SW_PS2_GCTRL (0x00)
> +#define SW_PS2_DATA (0x04)
> +#define SW_PS2_LCTRL (0x08)
> +#define SW_PS2_LSTAT (0x0c)
> +#define SW_PS2_FCTRL (0x10)
> +#define SW_PS2_FSTAT (0x14)
> +#define SW_PS2_CLKDR (0x18)
The SW_ prefix is a left over from Allwinner I suppose?
Using a prefix like sun4i_ps2/sun7i_ps2 depending on the outcome of
the discussion you had with Hans would seem better, across all your
driver.
> +
> +/* SW_PS2_GCTRL */
> +#define SWPS2_BUSEN (1 << 0)
> +#define SWPS2_MASTER (1 << 1)
> +#define SWPS2_RESET (1 << 2)
> +#define SWPS2_INTEN (1 << 3)
We usually have the convention of having the register name mentionned
in the bits offsets, and the bits declared right after their register.
Something like:
#define SUN4I_PS2_GCTRL_REG 0x00
#define SUN4I_PS2_GCTRL_BUS_EN BIT(0)
#define SUN4I_PS2_LCTRL_REG 0x08
#define SUN4I_PS2_LCTRL_PAR_ERR_EN BIT(1)
etc....
> +#define SWPS2_INTFLAG (1 << 3)
This is defined to the same value than _INTEN, is this intentionnal?
> +
> +/* SW_PS2_LCTRL */
> +#define SWPS2_LCTL_NOACK (0x0 << 18)
> +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> +
> +/* SW_PS2_FSTAT */
> +#define SWPS2_FSTA_RXRDY (1 << 0)
> +#define SWPS2_FSTA_RXOF (1 << 1)
> +#define SWPS2_FSTA_RXUF (1 << 2)
> +#define SWPS2_FSTA_TXRDY (1 << 8)
> +#define SWPS2_FSTA_TXOF (1 << 9)
> +#define SWPS2_FSTA_TXUF (1 << 10)
> +
> +#define SW_PS2_SAMPLE_CLK (1000000)
> +#define SW_PS2_SCLK (125000)
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
> +
> +/*********************/
> +/* Interrupt handler */
> +/*********************/
> +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> +{
> + struct sunxips2data *drvdata = dev_id;
> + u32 intr_status;
> + u32 fifo_status;
> + unsigned char byte;
> + u32 rval;
> + u32 error = 0;
> +
> + spin_lock(&drvdata->ps2_lock);
> +
> + /* Get the PS/2 interrupts and clear them */
> + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> +
> + /*Check Line Status Register*/
> + if (intr_status & 0x10f) {
> + if (intr_status & 0x08)
> + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> + if (intr_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Acknowledge Error!\n");
> + if (intr_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> + if (intr_status & 0x100)
> + dev_info(drvdata->dev, "PS/2 Transmit Data Timeout!\n");
> + if (intr_status & 0x01)
> + dev_info(drvdata->dev, "PS/2 Receive Data Timeout!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> +
> + error = 1;
> + }
> +
> + /*Check FIFO Status Register*/
> + if (fifo_status & 0x0606) {
> + if (fifo_status & 0x400)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Underflow!\n");
> + if (fifo_status & 0x200)
> + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> + if (fifo_status & 0x04)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Underflow!\n");
> + if (fifo_status & 0x02)
> + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> +
> + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4, drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> + error = 1;
> + }
> +
> + rval = (fifo_status >> 16) & 0x3;
> + while (!error && rval--) {
> + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
> + serio_interrupt(drvdata->serio, byte, 0);
> + }
> +
> + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> +
> + spin_unlock(&drvdata->ps2_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
What is this supposed to do? Calculating and rounding the dividers?
You should look into DIV_ROUND_* if that's so
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
Why is that udelay needed?
> +
> + return 0;
> +}
> +
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
spin_lock_irqsave would be better I guess.
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
> +
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
Please use time_before() here
> + return -1;
And return a meaningful error.
> +static int sunxips2_probe(struct platform_device *ofdev)
The ofdev name is measleading. This is a platform_device structure, it
could be probed by other mechanisms than OF, so please use a different
name here. Usually pdev is used.
> +{
> + struct resource *res; /* IO mem resources */
> + struct sunxips2data *drvdata;
> + struct serio *serio;
> + struct device *dev = &ofdev->dev;
> + unsigned int irq;
> + int error;
> +
> + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data), GFP_KERNEL);
> + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
> + if (!drvdata || !serio)
> + error = -ENOMEM;
> +
> + /* Request clock */
> + drvdata->pclk = clk_get(dev, NULL);
You can use devm_clk_get.
> + if (IS_ERR(drvdata->pclk))
> + dev_dbg(dev, "couldn't get clock %li\n",
> + PTR_ERR(drvdata->pclk));
This is an error, you should treat it as such.
> + if (!IS_ERR(drvdata->pclk)) {
> + error = clk_prepare_enable(drvdata->pclk);
> + if (error < 0) {
> + dev_err(dev, "failed to enable clock %d\n", error);
> + return error;
> + }
> + }
> +
> + /* IO */
> + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> + drvdata->base_address = devm_ioremap_resource(dev, res);
> + if (IS_ERR(drvdata->base_address)) {
> + dev_err(dev, "failed to map registers\n");
> + error = PTR_ERR(drvdata->base_address);
> + }
ditto.
> + serio->id.type = SERIO_8042;
> + serio->write = sunxips2_write;
> + serio->open = sunxips2_open;
> + serio->close = sunxips2_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));
> +
> + platform_set_drvdata(ofdev, drvdata);
> + serio_register_port(serio);
> +
> + /* Get IRQ for the device */
> + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (!irq) {
> + dev_err(dev, "no IRQ found\n");
> + return -ENODEV;
> + }
You can use platform_get_irq, for consistency with how you retrieve
resources.
You're also no unregistering the driver from the serio framework.
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
request_irq is enough here.
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
> +
> +static int sunxips2_remove(struct platform_device *of_dev)
> +{
> + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> +
> + if (!IS_ERR(drvdata->pclk)) {
> + clk_disable_unprepare(drvdata->pclk);
> + clk_put(drvdata->pclk);
> + }
> + serio_unregister_port(drvdata->serio);
> + mdelay(2);
Why a mdelay?
> + return 0;
> +}
> +
> +/* Match table for of_platform binding */
> +static const struct of_device_id sunxips2_of_match[] = {
> + { .compatible = "allwinner,sun7i-a20-ps2", },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> +
> +/*platform driver structure*/
> +static struct platform_driver sunxips2_of_driver = {
> + .probe = sunxips2_probe,
> + .remove = sunxips2_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
You can drop the owner field, it's already set by
module_platform_driver.
> + .of_match_table = sunxips2_of_match,
> + },
> +};
> +module_platform_driver(sunxips2_of_driver);
> +
> +MODULE_AUTHOR("Aaron.maoye<leafy.myeh@newbietech.com, "
> + "Vishnu Patekar <vishnupatekar0510@gmail.com>");
Usually, you define two MODULE_AUTHOR.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141205/074df38c/attachment.sig>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 10:33 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2014-12-05 10:33 UTC (permalink / raw
To: linux-arm-kernel
Cc: vishnupatekar, maxime.ripard, linux-sunxi, devicetree,
dmitry.torokhov, benh, linux-input, linux-kernel, ralf, robh+dt,
msalter, vishnupatekar, jdelvare
On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
> +
> +#define DRIVER_NAME "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
Remove this and use the existing resource_size() function
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
As this is dynamically allocated, better embed the serio member
directly to avoid allocating both separately.
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
100 microseconds is a rather long time to block the CPU for, so this
needs a comment explaining why the particular delay is needed and why
you can't use usleep_range() instead.
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
The locking is wrong here: you take the lock without disabling the
interrupts first, so if the interrupt happens between the spin_lock()
call and the writel(), the kernel will deadlock.
You will either have to use spin_lock_irq() here, or find a justification
for dropping the lock entirely.
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
> +
> + return -1;
> +}
We never return '-1' from in-kernel functions. Either make this a bool
argument, or return a proper errno.h value. This should probably return
-EIO.
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
Why any_context?
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 10:33 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2014-12-05 10:33 UTC (permalink / raw
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: vishnupatekar, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ralf-6z/3iImG2C8G8FEW9MqTrA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, msalter-H+wXaHxf7aLQT0dZR+AlfA,
vishnupatekar, jdelvare-l3A5Bk7waGM
On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
> +
> +#define DRIVER_NAME "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
Remove this and use the existing resource_size() function
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
As this is dynamically allocated, better embed the serio member
directly to avoid allocating both separately.
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
100 microseconds is a rather long time to block the CPU for, so this
needs a comment explaining why the particular delay is needed and why
you can't use usleep_range() instead.
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
The locking is wrong here: you take the lock without disabling the
interrupts first, so if the interrupt happens between the spin_lock()
call and the writel(), the kernel will deadlock.
You will either have to use spin_lock_irq() here, or find a justification
for dropping the lock entirely.
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
> +
> + return -1;
> +}
We never return '-1' from in-kernel functions. Either make this a bool
argument, or return a proper errno.h value. This should probably return
-EIO.
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
Why any_context?
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 10:33 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2014-12-05 10:33 UTC (permalink / raw
To: linux-arm-kernel
On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
> +
> +#define DRIVER_NAME "sunxi-ps2"
> +
> +#define RESSIZE(res) (((res)->end - (res)->start)+1)
Remove this and use the existing resource_size() function
> +
> +struct sunxips2data {
> + int irq;
> + spinlock_t ps2_lock;
> + void __iomem *base_address; /* virt address of control registers*/
> + struct serio *serio; /* serio*/
> + struct device *dev;
> + struct clk *pclk;
> +};
As this is dynamically allocated, better embed the serio member
directly to avoid allocating both separately.
> +static int sunxips2_open(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> + u32 src_clk = 0;
> + u32 clk_scdf;
> + u32 clk_pcdf;
> + u32 rval;
> +
> + /*Set Line Control And Enable Interrupt*/
> + rval = SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> +
> + /*Reset FIFO*/
> + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> +
> + src_clk = clk_get_rate(drvdata->pclk);
> +
> + if (!src_clk) {
> + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source clock is 0.");
> + return -1;
> + }
> +
> + /*Set Clock Divider Register*/
> + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK - 1);
> + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK - 1);
> + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
> + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> +
> + /*Set Global Control Register*/
> + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> +
> + udelay(100);
100 microseconds is a rather long time to block the CPU for, so this
needs a comment explaining why the particular delay is needed and why
you can't use usleep_range() instead.
> +static void sunxips2_close(struct serio *pserio)
> +{
> + struct sunxips2data *drvdata = pserio->port_data;
> +
> + spin_lock(&drvdata->ps2_lock);
> + /* Disable the PS2 interrupts */
> + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> + spin_unlock(&drvdata->ps2_lock);
> +}
The locking is wrong here: you take the lock without disabling the
interrupts first, so if the interrupt happens between the spin_lock()
call and the writel(), the kernel will deadlock.
You will either have to use spin_lock_irq() here, or find a justification
for dropping the lock entirely.
> +static int sunxips2_write(struct serio *pserio, unsigned char val)
> +{
> + struct sunxips2data *drvdata = (struct sunxips2data *)pserio->port_data;
> + u32 timeout = 10000;
> +
> + do {
> + if (readl(drvdata->base_address + SW_PS2_FSTAT) & SWPS2_FSTA_TXRDY) {
> + writel(val, drvdata->base_address + SW_PS2_DATA);
> + return 0;
> + }
> + } while (timeout--);
> +
> + return -1;
> +}
We never return '-1' from in-kernel functions. Either make this a bool
argument, or return a proper errno.h value. This should probably return
-EIO.
> + drvdata->irq = irq;
> + drvdata->serio = serio;
> + drvdata->dev = dev;
> + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq, &sunxips2_interrupt, 0,
> + DRIVER_NAME, drvdata);
> + if (error) {
> + dev_err(drvdata->dev,
> + "Couldn't allocate interrupt %d : error: %d\n", drvdata->irq, error);
> + return error;
> + }
> + return 0; /* success */
> +}
Why any_context?
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
2014-12-05 10:33 ` Arnd Bergmann
(?)
@ 2014-12-05 15:01 ` Dmitry Torokhov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-05 15:01 UTC (permalink / raw
To: Arnd Bergmann, linux-arm-kernel
Cc: vishnupatekar, maxime.ripard, linux-sunxi, devicetree, benh,
linux-input, linux-kernel, ralf, robh+dt, msalter, vishnupatekar,
jdelvare
Hi Arnd,
On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd@arndb.de> wrote:
>On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
>> +
>> +struct sunxips2data {
>> + int irq;
>> + spinlock_t ps2_lock;
>> + void __iomem *base_address; /* virt address of control registers*/
>> + struct serio *serio; /* serio*/
>> + struct device *dev;
>> + struct clk *pclk;
>> +};
>
>As this is dynamically allocated, better embed the serio member
>directly to avoid allocating both separately.
That would be wrong - serio is refcounted and it may outlive instance of sunxips2data you embedded it into.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:01 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-05 15:01 UTC (permalink / raw
To: Arnd Bergmann, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: vishnupatekar, maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ralf-6z/3iImG2C8G8FEW9MqTrA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, msalter-H+wXaHxf7aLQT0dZR+AlfA,
vishnupatekar, jdelvare-l3A5Bk7waGM
Hi Arnd,
On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
>> +
>> +struct sunxips2data {
>> + int irq;
>> + spinlock_t ps2_lock;
>> + void __iomem *base_address; /* virt address of control registers*/
>> + struct serio *serio; /* serio*/
>> + struct device *dev;
>> + struct clk *pclk;
>> +};
>
>As this is dynamically allocated, better embed the serio member
>directly to avoid allocating both separately.
That would be wrong - serio is refcounted and it may outlive instance of sunxips2data you embedded it into.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:01 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-05 15:01 UTC (permalink / raw
To: linux-arm-kernel
Hi Arnd,
On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd@arndb.de> wrote:
>On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
>> +
>> +struct sunxips2data {
>> + int irq;
>> + spinlock_t ps2_lock;
>> + void __iomem *base_address; /* virt address of control registers*/
>> + struct serio *serio; /* serio*/
>> + struct device *dev;
>> + struct clk *pclk;
>> +};
>
>As this is dynamically allocated, better embed the serio member
>directly to avoid allocating both separately.
That would be wrong - serio is refcounted and it may outlive instance of sunxips2data you embedded it into.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:50 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2014-12-05 15:50 UTC (permalink / raw
To: Dmitry Torokhov
Cc: linux-arm-kernel, vishnupatekar, maxime.ripard, linux-sunxi,
devicetree, benh, linux-input, linux-kernel, ralf, robh+dt,
msalter, vishnupatekar, jdelvare
On Friday 05 December 2014 07:01:17 Dmitry Torokhov wrote:
>
> On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd@arndb.de> wrote:
> >On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
> >> +
> >> +struct sunxips2data {
> >> + int irq;
> >> + spinlock_t ps2_lock;
> >> + void __iomem *base_address; /* virt address of control registers*/
> >> + struct serio *serio; /* serio*/
> >> + struct device *dev;
> >> + struct clk *pclk;
> >> +};
> >
> >As this is dynamically allocated, better embed the serio member
> >directly to avoid allocating both separately.
>
> That would be wrong - serio is refcounted and it may outlive instance of sunxips2data you embedded it into.
Ok, I see. I guess in this case the use of devm_kzalloc for serio is a bug,
because that would lead to a double free upon module unload, right?
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:50 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2014-12-05 15:50 UTC (permalink / raw
To: Dmitry Torokhov
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, vishnupatekar,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ralf-6z/3iImG2C8G8FEW9MqTrA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, msalter-H+wXaHxf7aLQT0dZR+AlfA,
vishnupatekar, jdelvare-l3A5Bk7waGM
On Friday 05 December 2014 07:01:17 Dmitry Torokhov wrote:
>
> On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
> >> +
> >> +struct sunxips2data {
> >> + int irq;
> >> + spinlock_t ps2_lock;
> >> + void __iomem *base_address; /* virt address of control registers*/
> >> + struct serio *serio; /* serio*/
> >> + struct device *dev;
> >> + struct clk *pclk;
> >> +};
> >
> >As this is dynamically allocated, better embed the serio member
> >directly to avoid allocating both separately.
>
> That would be wrong - serio is refcounted and it may outlive instance of sunxips2data you embedded it into.
Ok, I see. I guess in this case the use of devm_kzalloc for serio is a bug,
because that would lead to a double free upon module unload, right?
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:50 ` Arnd Bergmann
0 siblings, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2014-12-05 15:50 UTC (permalink / raw
To: linux-arm-kernel
On Friday 05 December 2014 07:01:17 Dmitry Torokhov wrote:
>
> On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd@arndb.de> wrote:
> >On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
> >> +
> >> +struct sunxips2data {
> >> + int irq;
> >> + spinlock_t ps2_lock;
> >> + void __iomem *base_address; /* virt address of control registers*/
> >> + struct serio *serio; /* serio*/
> >> + struct device *dev;
> >> + struct clk *pclk;
> >> +};
> >
> >As this is dynamically allocated, better embed the serio member
> >directly to avoid allocating both separately.
>
> That would be wrong - serio is refcounted and it may outlive instance of sunxips2data you embedded it into.
Ok, I see. I guess in this case the use of devm_kzalloc for serio is a bug,
because that would lead to a double free upon module unload, right?
Arnd
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
2014-12-05 15:50 ` Arnd Bergmann
(?)
@ 2014-12-05 15:53 ` Dmitry Torokhov
-1 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-05 15:53 UTC (permalink / raw
To: Arnd Bergmann
Cc: linux-arm-kernel, vishnupatekar, maxime.ripard, linux-sunxi,
devicetree, benh, linux-input, linux-kernel, ralf, robh+dt,
msalter, vishnupatekar, jdelvare
On December 5, 2014 7:50:18 AM PST, Arnd Bergmann <arnd@arndb.de> wrote:
>On Friday 05 December 2014 07:01:17 Dmitry Torokhov wrote:
>>
>> On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd@arndb.de>
>wrote:
>> >On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
>> >> +
>> >> +struct sunxips2data {
>> >> + int irq;
>> >> + spinlock_t ps2_lock;
>> >> + void __iomem *base_address; /* virt address of control
>registers*/
>> >> + struct serio *serio; /* serio*/
>> >> + struct device *dev;
>> >> + struct clk *pclk;
>> >> +};
>> >
>> >As this is dynamically allocated, better embed the serio member
>> >directly to avoid allocating both separately.
>>
>> That would be wrong - serio is refcounted and it may outlive instance
>of sunxips2data you embedded it into.
>
>Ok, I see. I guess in this case the use of devm_kzalloc for serio is a
>bug,
>because that would lead to a double free upon module unload, right?
>
Exactly. I already mentioned that in my review.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:53 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-05 15:53 UTC (permalink / raw
To: Arnd Bergmann
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, vishnupatekar,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ralf-6z/3iImG2C8G8FEW9MqTrA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, msalter-H+wXaHxf7aLQT0dZR+AlfA,
vishnupatekar, jdelvare-l3A5Bk7waGM
On December 5, 2014 7:50:18 AM PST, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>On Friday 05 December 2014 07:01:17 Dmitry Torokhov wrote:
>>
>> On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>wrote:
>> >On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
>> >> +
>> >> +struct sunxips2data {
>> >> + int irq;
>> >> + spinlock_t ps2_lock;
>> >> + void __iomem *base_address; /* virt address of control
>registers*/
>> >> + struct serio *serio; /* serio*/
>> >> + struct device *dev;
>> >> + struct clk *pclk;
>> >> +};
>> >
>> >As this is dynamically allocated, better embed the serio member
>> >directly to avoid allocating both separately.
>>
>> That would be wrong - serio is refcounted and it may outlive instance
>of sunxips2data you embedded it into.
>
>Ok, I see. I guess in this case the use of devm_kzalloc for serio is a
>bug,
>because that would lead to a double free upon module unload, right?
>
Exactly. I already mentioned that in my review.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
@ 2014-12-05 15:53 ` Dmitry Torokhov
0 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2014-12-05 15:53 UTC (permalink / raw
To: linux-arm-kernel
On December 5, 2014 7:50:18 AM PST, Arnd Bergmann <arnd@arndb.de> wrote:
>On Friday 05 December 2014 07:01:17 Dmitry Torokhov wrote:
>>
>> On December 5, 2014 2:33:11 AM PST, Arnd Bergmann <arnd@arndb.de>
>wrote:
>> >On Thursday 04 December 2014 04:23:44 vishnupatekar wrote:
>> >> +
>> >> +struct sunxips2data {
>> >> + int irq;
>> >> + spinlock_t ps2_lock;
>> >> + void __iomem *base_address; /* virt address of control
>registers*/
>> >> + struct serio *serio; /* serio*/
>> >> + struct device *dev;
>> >> + struct clk *pclk;
>> >> +};
>> >
>> >As this is dynamically allocated, better embed the serio member
>> >directly to avoid allocating both separately.
>>
>> That would be wrong - serio is refcounted and it may outlive instance
>of sunxips2data you embedded it into.
>
>Ok, I see. I guess in this case the use of devm_kzalloc for serio is a
>bug,
>because that would lead to a double free upon module unload, right?
>
Exactly. I already mentioned that in my review.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig
2014-12-05 10:01 ` Maxime Ripard
(?)
@ 2014-12-06 13:31 ` Vishnu Patekar
-1 siblings, 0 replies; 22+ messages in thread
From: Vishnu Patekar @ 2014-12-06 13:31 UTC (permalink / raw
To: Maxime Ripard
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org,
msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org,
jdelvare-l3A5Bk7waGM@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Dmitry Torokhov
[-- Attachment #1: Type: text/plain, Size: 16674 bytes --]
Hello Maxime,
Sorry for inconvenience. I'll follow the guidelines, will implement the
review comment and resubmit the patch for review.
On Fri, Dec 5, 2014 at 3:31 PM, Maxime Ripard <
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> Thanks for working on this.
>
> First, make sure to read Documentation/SubmittingPatches. The
> guidelines in there might seem tedious, but it's there to enforce that
> we keep the kernel homogenous.
>
> Most notably, you should make sure that:
> - you run scripts/checkpatch.pl on your patches
> - you have a commit log
> - your commit title stays below 80 chars
> - you have a Signed-off-by in your commit log
>
> On Thu, Dec 04, 2014 at 04:23:44AM +0530, vishnupatekar wrote:
> > ---
> > drivers/input/serio/Kconfig | 9 ++
> > drivers/input/serio/Makefile | 1 +
> > drivers/input/serio/sunxi-ps2.c | 305
> +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 315 insertions(+)
> > create mode 100644 drivers/input/serio/sunxi-ps2.c
> >
> > diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> > index bc2d474..1a86e41 100644
> > --- a/drivers/input/serio/Kconfig
> > +++ b/drivers/input/serio/Kconfig
> > @@ -281,4 +281,13 @@ config HYPERV_KEYBOARD
> > To compile this driver as a module, choose M here: the module
> will
> > be called hyperv_keyboard.
> >
> > +config SERIO_SUNXI_PS2
> > + tristate "Allwinner Sun7i-A20 PS/2 controller"
> > + default m
> > + help
> > + Say Y here if you have Sun7i-A20 Allwinner PS/2 ports.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called sunxi-ps2.
> > +
> > endif
> > diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> > index 815d874..0fa0f78 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_SUNXI_PS2) += sunxi-ps2.o
> > diff --git a/drivers/input/serio/sunxi-ps2.c
> b/drivers/input/serio/sunxi-ps2.c
> > new file mode 100644
> > index 0000000..ccd7b29
> > --- /dev/null
> > +++ b/drivers/input/serio/sunxi-ps2.c
> > @@ -0,0 +1,305 @@
> > +/*
> > + * sunxi-ps2.c Support for Allwinner A20 PS2 host controller
> > + *
> > + * Author: Aaron.maoye <leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org>
> > + * Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > + * Based on sunxi-ps2.c 3.0 kernel
>
> Mentionning that it's Allwinner's 3.0 kernel would be good, it doesn't
> make much sense otherwise.
>
> Okie
> > +#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 "sunxi-ps2"
> > +
> > +#define RESSIZE(res) (((res)->end - (res)->start)+1)
> > +
> > +#define SW_PS2_GCTRL (0x00)
> > +#define SW_PS2_DATA (0x04)
> > +#define SW_PS2_LCTRL (0x08)
> > +#define SW_PS2_LSTAT (0x0c)
> > +#define SW_PS2_FCTRL (0x10)
> > +#define SW_PS2_FSTAT (0x14)
> > +#define SW_PS2_CLKDR (0x18)
>
> The SW_ prefix is a left over from Allwinner I suppose?
>
> Using a prefix like sun4i_ps2/sun7i_ps2 depending on the outcome of
> the discussion you had with Hans would seem better, across all your
> driver.
>
> Agreed, as this PS2 driver can be applicable for A10 and A20, I'll keep it
as PS2_REG_* for registers, e.g. PS2_REG_GCTL and PS2_GCTL_BUSEN for bits.
it is similar to SUNXI MMC driver.
> > +
> > +/* SW_PS2_GCTRL */
> > +#define SWPS2_BUSEN (1 << 0)
> > +#define SWPS2_MASTER (1 << 1)
> > +#define SWPS2_RESET (1 << 2)
> > +#define SWPS2_INTEN (1 << 3)
>
> We usually have the convention of having the register name mentionned
> in the bits offsets, and the bits declared right after their register.
>
> Something like:
>
> #define SUN4I_PS2_GCTRL_REG 0x00
> #define SUN4I_PS2_GCTRL_BUS_EN BIT(0)
>
> #define SUN4I_PS2_LCTRL_REG 0x08
> #define SUN4I_PS2_LCTRL_PAR_ERR_EN BIT(1)
>
> etc....
>
> Agreed, BIT(x) macros will be used.
> > +#define SWPS2_INTFLAG (1 << 3)
>
> This is defined to the same value than _INTEN, is this intentionnal?
>
> This macro is not used, will be removed.
> > +
> > +/* SW_PS2_LCTRL */
> > +#define SWPS2_LCTL_NOACK (0x0 << 18)
> > +#define SWPS2_LCTL_TXDTOEN (0x1 << 8)
> > +#define SWPS2_LCTL_STOPERREN (0x1 << 3)
> > +#define SWPS2_LCTL_ACKERREN (0x1 << 2)
> > +#define SWPS2_LCTL_PARERREN (0x1 << 1)
> > +#define SWPS2_LCTL_RXDTOEN (0x1 << 0)
> > +
> > +/* SW_PS2_FSTAT */
> > +#define SWPS2_FSTA_RXRDY (1 << 0)
> > +#define SWPS2_FSTA_RXOF (1 << 1)
> > +#define SWPS2_FSTA_RXUF (1 << 2)
> > +#define SWPS2_FSTA_TXRDY (1 << 8)
> > +#define SWPS2_FSTA_TXOF (1 << 9)
> > +#define SWPS2_FSTA_TXUF (1 << 10)
> > +
> > +#define SW_PS2_SAMPLE_CLK (1000000)
> > +#define SW_PS2_SCLK (125000)
> > +
> > +struct sunxips2data {
> > + int irq;
> > + spinlock_t ps2_lock;
> > + void __iomem *base_address; /* virt address of control
> registers*/
> > + struct serio *serio; /* serio*/
> > + struct device *dev;
> > + struct clk *pclk;
> > +};
> > +
> > +/*********************/
> > +/* Interrupt handler */
> > +/*********************/
> > +static irqreturn_t sunxips2_interrupt(int irq, void *dev_id)
> > +{
> > + struct sunxips2data *drvdata = dev_id;
> > + u32 intr_status;
> > + u32 fifo_status;
> > + unsigned char byte;
> > + u32 rval;
> > + u32 error = 0;
> > +
> > + spin_lock(&drvdata->ps2_lock);
> > +
> > + /* Get the PS/2 interrupts and clear them */
> > + intr_status = readl(drvdata->base_address + SW_PS2_LSTAT);
> > + fifo_status = readl(drvdata->base_address + SW_PS2_FSTAT);
> > +
> > + /*Check Line Status Register*/
> > + if (intr_status & 0x10f) {
> > + if (intr_status & 0x08)
> > + dev_info(drvdata->dev, "PS/2 Stop Bit Error!");
> > + if (intr_status & 0x04)
> > + dev_info(drvdata->dev, "PS/2 Acknowledge
> Error!\n");
> > + if (intr_status & 0x02)
> > + dev_info(drvdata->dev, "PS/2 Parity Error!\n");
> > + if (intr_status & 0x100)
> > + dev_info(drvdata->dev, "PS/2 Transmit Data
> Timeout!\n");
> > + if (intr_status & 0x01)
> > + dev_info(drvdata->dev, "PS/2 Receive Data
> Timeout!\n");
> > +
> > + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4,
> drvdata->base_address + SW_PS2_GCTRL);/*reset PS/2 controller*/
> > + writel(0x10f, drvdata->base_address + SW_PS2_LSTAT);
> > +
> > + error = 1;
> > + }
> > +
> > + /*Check FIFO Status Register*/
> > + if (fifo_status & 0x0606) {
> > + if (fifo_status & 0x400)
> > + dev_info(drvdata->dev, "PS/2 Tx FIFO
> Underflow!\n");
> > + if (fifo_status & 0x200)
> > + dev_info(drvdata->dev, "PS/2 Tx FIFO Overflow!\n");
> > + if (fifo_status & 0x04)
> > + dev_info(drvdata->dev, "PS/2 Rx FIFO
> Underflow!\n");
> > + if (fifo_status & 0x02)
> > + dev_info(drvdata->dev, "PS/2 Rx FIFO Overflow!\n");
> > +
> > + writel(readl(drvdata->base_address + SW_PS2_GCTRL)|0x4,
> drvdata->base_address + SW_PS2_GCTRL); /*reset PS/2 controller*/
> > + writel(0x707, drvdata->base_address + SW_PS2_FSTAT);
> > + error = 1;
> > + }
> > +
> > + rval = (fifo_status >> 16) & 0x3;
> > + while (!error && rval--) {
> > + byte = readl(drvdata->base_address + SW_PS2_DATA) & 0xff;
> > + dev_info(drvdata->dev, "PS/2 Receive %02x\n", byte);
> > + serio_interrupt(drvdata->serio, byte, 0);
> > + }
> > +
> > + writel(intr_status, drvdata->base_address + SW_PS2_LSTAT);
> > + writel(fifo_status, drvdata->base_address + SW_PS2_FSTAT);
> > +
> > + spin_unlock(&drvdata->ps2_lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int sunxips2_open(struct serio *pserio)
> > +{
> > + struct sunxips2data *drvdata = pserio->port_data;
> > + u32 src_clk = 0;
> > + u32 clk_scdf;
> > + u32 clk_pcdf;
> > + u32 rval;
> > +
> > + /*Set Line Control And Enable Interrupt*/
> > + rval =
> SWPS2_LCTL_TXDTOEN|SWPS2_LCTL_STOPERREN|SWPS2_LCTL_ACKERREN|SWPS2_LCTL_PARERREN|SWPS2_LCTL_RXDTOEN;
> > + writel(rval, drvdata->base_address + SW_PS2_LCTRL);
> > +
> > + /*Reset FIFO*/
> > + writel(0x3<<16 | 0x607, drvdata->base_address + SW_PS2_FCTRL);
> > +
> > + src_clk = clk_get_rate(drvdata->pclk);
> > +
> > + if (!src_clk) {
> > + dev_info(drvdata->dev, "w_ps2c_set_sclk error, source
> clock is 0.");
> > + return -1;
> > + }
> > +
> > + /*Set Clock Divider Register*/
> > + clk_scdf = ((src_clk + (SW_PS2_SAMPLE_CLK>>1)) / SW_PS2_SAMPLE_CLK
> - 1);
> > + clk_pcdf = ((SW_PS2_SAMPLE_CLK + (SW_PS2_SCLK>>1)) / SW_PS2_SCLK -
> 1);
> > + rval = (clk_scdf<<8) | clk_pcdf;/* | (PS2_DEBUG_SEL<<16);*/
>
> What is this supposed to do? Calculating and rounding the dividers?
>
> You should look into DIV_ROUND_* if that's so
>
> Yes, Agreed, It'll be used as below, this solves purpose.
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_CLKDR);
> > + writel(rval, drvdata->base_address + SW_PS2_CLKDR);
> > +
> > + /*Set Global Control Register*/
> > + rval = SWPS2_RESET|SWPS2_INTEN|SWPS2_MASTER|SWPS2_BUSEN;
> > + writel(rval, drvdata->base_address + SW_PS2_GCTRL);
> > +
> > + udelay(100);
>
> Why is that udelay needed?
>
> No need, will be removed.
> > +
> > + return 0;
> > +}
> > +
> > +static void sunxips2_close(struct serio *pserio)
> > +{
> > + struct sunxips2data *drvdata = pserio->port_data;
> > +
> > + spin_lock(&drvdata->ps2_lock);
>
> spin_lock_irqsave would be better I guess.
>
> I'll use it.
> > + /* Disable the PS2 interrupts */
> > + writel(0, drvdata->base_address + SW_PS2_GCTRL);
> > + spin_unlock(&drvdata->ps2_lock);
> > +}
> > +
> > +static int sunxips2_write(struct serio *pserio, unsigned char val)
> > +{
> > + struct sunxips2data *drvdata = (struct sunxips2data
> *)pserio->port_data;
> > + u32 timeout = 10000;
> > +
> > + do {
> > + if (readl(drvdata->base_address + SW_PS2_FSTAT) &
> SWPS2_FSTA_TXRDY) {
> > + writel(val, drvdata->base_address + SW_PS2_DATA);
> > + return 0;
> > + }
> > + } while (timeout--);
>
> Please use time_before() here
>
> Agreed, time_before(jiffies, expire) will be used.
> > + return -1;
>
> And return a meaningful error.
>
> Okie
> > +static int sunxips2_probe(struct platform_device *ofdev)
>
> The ofdev name is measleading. This is a platform_device structure, it
> could be probed by other mechanisms than OF, so please use a different
> name here. Usually pdev is used.
>
> > +{
> > + struct resource *res; /* IO mem resources */
> > + struct sunxips2data *drvdata;
> > + struct serio *serio;
> > + struct device *dev = &ofdev->dev;
> > + unsigned int irq;
> > + int error;
> > +
> > + drvdata = devm_kzalloc(dev, sizeof(struct sunxips2data),
> GFP_KERNEL);
> > + serio = devm_kzalloc(dev, sizeof(struct serio), GFP_KERNEL);
> > + if (!drvdata || !serio)
> > + error = -ENOMEM;
> > +
> > + /* Request clock */
> > + drvdata->pclk = clk_get(dev, NULL);
>
> You can use devm_clk_get.
>
> Agreed, devm_clk_get will be used.
> > + if (IS_ERR(drvdata->pclk))
> > + dev_dbg(dev, "couldn't get clock %li\n",
> > + PTR_ERR(drvdata->pclk));
>
> This is an error, you should treat it as such.
>
> > + if (!IS_ERR(drvdata->pclk)) {
> > + error = clk_prepare_enable(drvdata->pclk);
> > + if (error < 0) {
> > + dev_err(dev, "failed to enable clock %d\n", error);
> > + return error;
> > + }
> > + }
> > +
> > + /* IO */
> > + res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
> > + drvdata->base_address = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(drvdata->base_address)) {
> > + dev_err(dev, "failed to map registers\n");
> > + error = PTR_ERR(drvdata->base_address);
> > + }
>
> ditto.
>
> > + serio->id.type = SERIO_8042;
> > + serio->write = sunxips2_write;
> > + serio->open = sunxips2_open;
> > + serio->close = sunxips2_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));
> > +
> > + platform_set_drvdata(ofdev, drvdata);
> > + serio_register_port(serio);
> > +
> > + /* Get IRQ for the device */
> > + irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> > + if (!irq) {
> > + dev_err(dev, "no IRQ found\n");
> > + return -ENODEV;
> > + }
>
> You can use platform_get_irq, for consistency with how you retrieve
> resources.
>
> You're also no unregistering the driver from the serio framework.
>
> I'll put code for error handling and freeing allocated resources.
> > + drvdata->irq = irq;
> > + drvdata->serio = serio;
> > + drvdata->dev = dev;
> > + error = devm_request_any_context_irq(drvdata->dev, drvdata->irq,
> &sunxips2_interrupt, 0,
> > + DRIVER_NAME, drvdata);
>
> request_irq is enough here.
>
> Okie, I'll use devm_request_threaded_irq as request_irq is just wrapper
arroud it.
> > + if (error) {
> > + dev_err(drvdata->dev,
> > + "Couldn't allocate interrupt %d : error: %d\n",
> drvdata->irq, error);
> > + return error;
> > + }
> > + return 0; /* success */
> > +}
> > +
> > +static int sunxips2_remove(struct platform_device *of_dev)
> > +{
> > + struct sunxips2data *drvdata = platform_get_drvdata(of_dev);
> > +
> > + if (!IS_ERR(drvdata->pclk)) {
> > + clk_disable_unprepare(drvdata->pclk);
> > + clk_put(drvdata->pclk);
> > + }
> > + serio_unregister_port(drvdata->serio);
> > + mdelay(2);
>
> Why a mdelay?
>
No need, will be removed.
>
> > + return 0;
> > +}
> > +
> > +/* Match table for of_platform binding */
> > +static const struct of_device_id sunxips2_of_match[] = {
> > + { .compatible = "allwinner,sun7i-a20-ps2", },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, sunxips2_of_match);
> > +
> > +/*platform driver structure*/
> > +static struct platform_driver sunxips2_of_driver = {
> > + .probe = sunxips2_probe,
> > + .remove = sunxips2_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .owner = THIS_MODULE,
>
> You can drop the owner field, it's already set by
> module_platform_driver.
>
> Okie
> > + .of_match_table = sunxips2_of_match,
> > + },
> > +};
> > +module_platform_driver(sunxips2_of_driver);
> > +
> > +MODULE_AUTHOR("Aaron.maoye<leafy.myeh-Q9AEpCAkrSgqDJ6do+/SaQ@public.gmane.org, "
> > + "Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
>
> Usually, you define two MODULE_AUTHOR.
>
> Okie
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
>
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: Type: text/html, Size: 25940 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-12-06 13:31 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-03 22:53 [PATCH 2/3] drivers:input:ps2 Added sunxi A20 ps2 driver, changed makefile and Kconfig vishnupatekar
2014-12-03 22:53 ` vishnupatekar
2014-12-03 22:53 ` vishnupatekar
2014-12-03 23:23 ` Dmitry Torokhov
2014-12-03 23:23 ` Dmitry Torokhov
2014-12-03 23:23 ` Dmitry Torokhov
2014-12-04 3:08 ` Vishnu Patekar
2014-12-05 10:01 ` Maxime Ripard
2014-12-05 10:01 ` Maxime Ripard
2014-12-06 13:31 ` Vishnu Patekar
2014-12-05 10:33 ` Arnd Bergmann
2014-12-05 10:33 ` Arnd Bergmann
2014-12-05 10:33 ` Arnd Bergmann
2014-12-05 15:01 ` Dmitry Torokhov
2014-12-05 15:01 ` Dmitry Torokhov
2014-12-05 15:01 ` Dmitry Torokhov
2014-12-05 15:50 ` Arnd Bergmann
2014-12-05 15:50 ` Arnd Bergmann
2014-12-05 15:50 ` Arnd Bergmann
2014-12-05 15:53 ` Dmitry Torokhov
2014-12-05 15:53 ` Dmitry Torokhov
2014-12-05 15:53 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.