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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20141211210919.GI8739@lukather>
Date:	Thu, 11 Dec 2014 22:09:19 +0100
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Vishnu Patekar <vishnupatekar0510@...il.com>
Cc:	linux-sunxi@...glegroups.com, dmitry.torokhov@...il.com,
	linux@....linux.org.uk, leafy.myeh@...bietech.com,
	robh+dt@...nel.org, pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: 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:
> > > Signed-off-by: vishnupatekar <VishnuPatekar0510@...il.com>
> > > ---
> > >  drivers/input/serio/Kconfig     |   10 ++
> > >  drivers/input/serio/Makefile    |    1 +
> > >  drivers/input/serio/sunxi-ps2.c |  364
> > +++++++++++++++++++++++++++++++++++++++
> > >  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
> > >         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.

> > > +     /*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?
> >
> > > +     rval = (clk_scdf<<8) | clk_pcdf;
> >
> > Spaces between the operators. Remember, run checkpatch.
> >
> Okie.
> 
> checkpatch could  not catch this!!

Well, then it should have :)

> > > +     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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)