> -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > Sent: Tuesday, January 18, 2011 1:40 AM > To: Guan Xuetao > Cc: linux-input@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] i8042 driver for unicore32 architecture > > Hi Guan, > > On Sun, Jan 16, 2011 at 11:45:23PM +0800, Guan Xuetao wrote: > > This patch implements i8042 driver for unicore32 architecture. > > > > Signed-off-by: Guan Xuetao <gxt@xxxxxxxxxxxxxxx> > > --- > > drivers/input/serio/i8042.h | 2 + > > drivers/staging/puv3/i8042-ucio.h | 89 +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 91 insertions(+), 0 deletions(-) > > create mode 100644 drivers/staging/puv3/i8042-ucio.h > > > > diff --git a/drivers/input/serio/i8042.h b/drivers/input/serio/i8042.h > > index cbc1beb..711b41c 100644 > > --- a/drivers/input/serio/i8042.h > > +++ b/drivers/input/serio/i8042.h > > @@ -26,6 +26,8 @@ > > #include "i8042-sparcio.h" > > #elif defined(CONFIG_X86) || defined(CONFIG_IA64) > > #include "i8042-x86ia64io.h" > > +#elif defined(CONFIG_UNICORE32) > > +#include "../../staging/puv3/i8042-ucio.h" > > I'd rather you put it directly into drivers/input/serio/ > Also, maybe we should call it i8042-unicore32io.h Ok. > > > #else > > #include "i8042-io.h" > > #endif > > diff --git a/drivers/staging/puv3/i8042-ucio.h b/drivers/staging/puv3/i8042-ucio.h > > new file mode 100644 > > index 0000000..c3221df > > --- /dev/null > > +++ b/drivers/staging/puv3/i8042-ucio.h > > @@ -0,0 +1,89 @@ > > +/* > > + * linux/drivers/staging/puv3/i8042-ucio.h > > Please do not put filenames in the comments. That's ok. But why? > > > + * > > + * Code specific to PKUnity SoC and UniCore ISA > > + * > > + * Maintained by GUAN Xue-tao <gxt@xxxxxxxxxxxxxxx> > > + * Copyright (C) 2001-2010 Guan Xuetao > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 as > > + * published by the Free Software Foundation. > > + */ > > +#ifndef _I8042_UCIO_H > > +#define _I8042_UCIO_H > > + > > +#include <linux/clk.h> > > +#include <mach/hardware.h> > > + > > +/* > > + * Names. > > + */ > > + > > +#define I8042_KBD_PHYS_DESC "isa0060/serio0" > > +#define I8042_AUX_PHYS_DESC "isa0060/serio1" > > +#define I8042_MUX_PHYS_DESC "isa0060/serio%d" > > + > > +/* > > + * IRQs. > > + */ > > +#define I8042_KBD_IRQ IRQ_PS2_KBD > > +#define I8042_AUX_IRQ IRQ_PS2_AUX > > + > > +/* > > + * Register numbers. > > + */ > > + > > +#define I8042_COMMAND_REG ((unsigned long)&PS2_COMMAND) > > +#define I8042_STATUS_REG ((unsigned long)&PS2_STATUS) > > +#define I8042_DATA_REG ((unsigned long)&PS2_DATA) > > This looks a bit iffy... any chance to simplify register definitions? All hardware registers on unicore32 use __REG macro, which acts well for both left value and right value in assignment sentence. However, when __REG is used for inb/outb, the registers need "&" prefix to be used as io addresses. > > > + > > +void puv3_ps2_init(void) > > Should it be static? Sorry for using not-static function in header file. We have a counter register for ps2, which need initialization when machine startup, both cold startup and wakeup from sleep. Its value is computed by using BUS32_CLK. > > > +{ > > + struct clk *bclk32; > > + > > + bclk32 = clk_get(NULL, "BUS32_CLK"); > > Error hanlding? Not error handling, see above explanation. > > > + PS2_CNT = clk_get_rate(bclk32) / 200000; /* should > 5us */ > > +} > > + > > +static inline int i8042_read_data(void) > > +{ > > + return inb(I8042_DATA_REG); > > +} > > + > > +static inline int i8042_read_status(void) > > +{ > > + return inb(I8042_STATUS_REG); > > +} > > + > > +static inline void i8042_write_data(int val) > > +{ > > + outb(val, I8042_DATA_REG); > > +} > > + > > +static inline void i8042_write_command(int val) > > +{ > > + outb(val, I8042_COMMAND_REG); > > +} > > + > > +static inline int i8042_platform_init(void) > > +{ > > +/* > > + * On some platforms touching the i8042 data register region can do really > > + * bad things. Because of this the region is always reserved on such boxes. > > + */ > > The comment is in wrong place and is not applicable to your arch I > think. Ok, removed. Request_region and release_region are also removed. > > > + puv3_ps2_init(); > > + > > + if (!request_region(I8042_DATA_REG, 16, "i8042")) > > + return -EBUSY; > > + > > + i8042_reset = 1; > > + return 0; > > +} > > + > > +static inline void i8042_platform_exit(void) > > +{ > > + release_region(I8042_DATA_REG, 16); > > +} > > + > > +#endif /* _I8042_UCIO_H */ > > Thanks. > > -- > Dmitry Thanks Dmitry. Guan Xuetao -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
