Re: [PATCHv2 2/3] sunxi:drivers:input:ps2 Added sunxi A10/A20 ps2 driver

Hi,

Remember to reply to all the recipients.

On Tue, Dec 09, 2014 at 04:40:36PM +0530, Vishnu Patekar wrote:

quoted

quoted

Signed-off-by: vishnupatekar <redacted>
---
 drivers/input/serio/Kconfig     |   10 ++
 drivers/input/serio/Makefile    |    1 +
 drivers/input/serio/sunxi-ps2.c |  364

+++++++++++++++++++++++++++++++++++++++

quoted

 3 files changed, 375 insertions(+)
 create mode 100644 drivers/input/serio/sunxi-ps2.c

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index bc2d474..3a7599c 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig

@@ -281,4 +281,14 @@ config HYPERV_KEYBOARD
        To compile this driver as a module, choose M here: the module

will

quoted

        be called hyperv_keyboard.

+config SERIO_SUNXI_PS2
+     tristate "Allwinner Sun4i-A10/Sun7i-A20 PS/2 controller"

Allwinner A10 is enough

Okie, Should I change the config option as well, like SERIO_SUN4I_PS2 ?

Yep.

quoted

quoted

+     /*Set Clock Divider Register*/
+     clk_scdf = DIV_ROUND_UP(src_clk, PS2_SAMPLE_CLK) - 1;
+     clk_pcdf = DIV_ROUND_UP(PS2_SAMPLE_CLK, PS2_SCLK) - 1;

So this is actually a rounding down?

Why not just using src_clk / PS2_SAMPLE_CLK directly?

quoted

+     rval = (clk_scdf<<8) | clk_pcdf;

Spaces between the operators. Remember, run checkpatch.

Okie.

checkpatch could  not catch this!!

Well, then it should have :)

quoted

quoted

+     writel(rval, drvdata->reg_base + PS2_REG_CLKDR);
+
+     /*Set Global Control Register*/
+     rval = PS2_GCTL_RESET | PS2_GCTL_INTEN | PS2_GCTL_MASTER
+             | PS2_GCTL_BUSEN;
+     writel(rval, drvdata->reg_base + PS2_REG_GCTL);

You seem to be reading/writing from the same registers than in your
interrupt handler, don't you need some locking in here?

Interrupt is getting enabled only after writing to GCTL register.

do you think still we need to locking mechanism here?

Not really, you don't know the state of the device before your driver
is probed, and the interrupts are enabled as soon as request_irq. So
you can very well have the device running with the interrupts running
before this function is called.

Plus you might get spurious interrupts.

Maxime

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