DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=date:from:to:cc:subject:message-id:references:mime-version
         :content-type:content-disposition:in-reply-to:user-agent;
        b=Raqfv1+nhEfmH1CQwNhQzxq9BwEdqNFfw0m84AveI3sagVwjX40GSzYcuJSpAplRA3
         YGPX57+lHS9VgSsf9kzLZkq228wlWm1igUGxHFGxZOJrL1/3buOS/djTH2iE3w1DSQhx
         jHgO03yIvkrIX9VhgkaD/NI99mxUOZHxooc84=
Date: Wed, 25 Nov 2009 22:08:54 -0800
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Wu Zhangjin <wuzhangjin@gmail.com>
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org,
       Richard Purdie <rpurdie@rpsys.net>, lm-sensors@lm-sensors.org,
       linux-input@vger.kernel.org, linux-laptop@vger.kernel.org,
       Stephen Rothwell <sfr@canb.auug.org.au>, sfr@linuxcare.com,
       linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] [loongson] yeeloong2f: add platform specific support
Message-ID: <20091126060854.GK23244@core.coreip.homeip.net>
References: <9ef5a0038ec5e9b584be8c960693c434a323620a.1258803311.git.wuzhangjin@gmail.com> <9ef5a0038ec5e9b584be8c960693c434a323620a.1258803311.git.wuzhangjin@gmail.com> <20091126041047.GC23244@core.coreip.homeip.net> <1259215215.4989.38.camel@falcon.domain.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1259215215.4989.38.camel@falcon.domain.org>
User-Agent: Mutt/1.5.19 (2009-01-05)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6550
Lines: 213

On Thu, Nov 26, 2009 at 02:00:15PM +0800, Wu Zhangjin wrote:
> On Wed, 2009-11-25 at 20:10 -0800, Dmitry Torokhov wrote:
> > Hi Wu,
> > 
> > Overall impression - several drivers crammed into one module, I wonder
> > if it could be split somewhat. 
> 
> Yeah, I have put several drivers into one file, just like the folks did
> in drivers/platform/x86/ for their platforms. I hated it too, will split
> it later(as least, for review is necessary).
> 
> > Some input-related concerns below.
> 
> Thanks very much for your feedback.
> 
> > 
> > On Sat, Nov 21, 2009 at 08:08:40PM +0800, Wu Zhangjin wrote:
> > > +
> > > +/* hotkey input subdriver */
> > > +
> > > +static struct input_dev *yeeloong_hotkey_dev;
> > > +static int event, status;
> > > +
> > > +struct key_entry {
> > > +	char type;		/* See KE_* below */
> > > +	int event;		/* event from SCI */
> > > +	u16 keycode;		/* KEY_* or SW_* */
> > > +};
> > > +
> > > +enum { KE_KEY, KE_SW, KE_END };
> > 
> > I am going to post the sparse keymap library shortly, this driver could
> > use it too...
> > 
> 
> when you posting your keymap library, could you please put me in the CC
> list? thanks ;)
> 
> > > +
> > > +static struct key_entry yeeloong_keymap[] = {
> > > +	{KE_SW, EVENT_LID, SW_LID},
> > > +	/* SW_VIDEOOUT_INSERT? not included in hald-addon-input! */
> > > +	{KE_KEY, EVENT_CRT_DETECT, KEY_PROG1},
> 
> any available event for reporting the inserted CRT in hald-addon-input?

Hmm.. not really. Again, does not seem to be related to input subsystem
but video one.

> 
> > > +	/* Seems battery subdriver should report it */
> > > +	{KE_KEY, EVENT_OVERTEMP, KEY_PROG2},
> > 
> > Does not seem to be an input event?
> > 
> 
> It is not an input event, Will remove it from the keymap table.
>  
> BTW: how can we handle this stuff(overtemp...)? lm-sensors? Perhaps
> temp1_max_alarm or temp1_crit_alarm?
> 

Not really sure... HWMON list is a good place to ask I think.

> > > +	/*{KE_KEY, EVENT_AC_BAT, KEY_BATTERY},*/
> > > +	{KE_KEY, EVENT_CAMERA, KEY_CAMERA},	/* Fn + ESC */
> > > +	{KE_KEY, EVENT_SLEEP, KEY_SLEEP},	/* Fn + F1 */
> > > +	/* Seems not clear? not included in hald-addon-input! */
> > > +	{KE_KEY, EVENT_BLACK_SCREEN, KEY_PROG3},	/* Fn + F2 */
> > 
> > Do you mean "lock screen"?
> 
> not lock, close the backlight of the display, any available event here?

What is it then? A switch that turns off backlight? What is the expected
reaction to it?

> 
> > 
> > > +	{KE_KEY, EVENT_DISPLAY_TOGGLE, KEY_SWITCHVIDEOMODE},	/* Fn + F3 */
> > > +	{KE_KEY, EVENT_AUDIO_MUTE, KEY_MUTE},	/* Fn + F4 */
> > > +	{KE_KEY, EVENT_WLAN, KEY_WLAN},	/* Fn + F5 */
> > > +	{KE_KEY, EVENT_DISPLAY_BRIGHTNESS, KEY_BRIGHTNESSUP},	/* Fn + up */
> > > +	{KE_KEY, EVENT_DISPLAY_BRIGHTNESS, KEY_BRIGHTNESSDOWN},	/* Fn + down */
> > > +	{KE_KEY, EVENT_AUDIO_VOLUME, KEY_VOLUMEUP},	/* Fn + right */
> > > +	{KE_KEY, EVENT_AUDIO_VOLUME, KEY_VOLUMEDOWN},	/* Fn + left */
> > > +	{KE_END, 0}
> > > +};
> > > +
> > 
> > ...
> > 
> > > +
> > > +static ssize_t
> > > +ignore_store(struct device *dev,
> > > +	     struct device_attribute *attr, const char *buf, size_t count)
> > > +{
> > > +	return count;
> > > +}
> > > +
> > > +static ssize_t
> > > +show_hotkeystate(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +	return sprintf(buf, "%d %d\n", event, status);
> > > +}
> > > +
> > > +static DEVICE_ATTR(state, 0444, show_hotkeystate, ignore_store);
> > 
> > Why do you need "ignore_store" and not just use NULL? Also why do you
> > need to export the state at all?
> > 
> 
> Okay, Ignore_store() should be NULL here, show_hotkeystate() is only
> added for debugging when I write this driver and also, it is helpful to
> my own user-space hotkey manager. anyway, it is not needed for upstream,
> will remove it later.
> 
> > > +
> > > +static struct attribute *hotkey_attributes[] = {
> > > +	&dev_attr_state.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static struct attribute_group hotkey_attribute_group = {
> > > +	.attrs = hotkey_attributes
> > > +};
> > > +
> > > +static int camera_set(int status)
> > > +{
> > > +	int value;
> > > +	static int camera_status;
> > > +
> > > +	if (status == 2)
> > > +		/* resume the old camera status */
> > > +		camera_set(camera_status);
> > > +	else if (status == 3) {
> > > +		/* revert the camera status */
> > > +		value = ec_read(REG_CAMERA_CONTROL);
> > > +		ec_write(REG_CAMERA_CONTROL, value | (1 << 1));
> > > +	} else {/* status == 0 or status == 1 */
> > > +		status = !!status;
> > > +		camera_status = ec_read(REG_CAMERA_STATUS);
> > > +		if (status != camera_status)
> > > +			camera_set(3);
> > > +	}
> > > +	return ec_read(REG_CAMERA_STATUS);
> > > +}
> > > +
> > > +#define I8042_STATUS_REG	0x64
> > > +#define I8042_DATA_REG		0x60
> > > +#define i8042_read_status() inb(I8042_STATUS_REG)
> > > +#define i8042_read_data() inb(I8042_DATA_REG)
> > > +#define I8042_STR_OBF		0x01
> > > +#define I8042_BUFFER_SIZE	16
> > > +
> > > +static void i8042_flush(void)
> > > +{
> > > +	int i;
> > > +
> > > +	while ((i8042_read_status() & I8042_STR_OBF)
> > > +		&& (i < I8042_BUFFER_SIZE)) {
> > > +		udelay(50);
> > > +		i8042_read_data();
> > > +		i++;
> > > +	}
> > > +}
> > > +
> > > +static int yeeloong_hotkey_init(struct device *dev)
> > > +{
> > > +	int ret;
> > > +	struct key_entry *key;
> > > +
> > > +	/* flush the buffer of keyboard */
> > > +	i8042_flush();
> > 
> > Why??? Why does this driver tries to touch stuff that does not belong to
> > it?
> > 
> 
> I doubted the keys in the buffer of keyboard will generate weird
> problems, so, added the above lines. and another guy just cleared this
> part to me that the Fnkey stuff not go to the buffer of keyboard, so,
> I'm doing stupid job here, will remove it later too.
> 
> > > +
> > > +	/* setup the system control interface */
> > > +	setup_sci();
> > 
> > No failures?
> 
> Need to check the return value here, and also, the setup_sci() itself.
> 
> > 
> > > +
> > > +	yeeloong_hotkey_dev = input_allocate_device();
> > > +
> > > +	if (!yeeloong_hotkey_dev)
> > > +		return -ENOMEM;
> > 
> > Error unwinding?
> 
> Sorry, not clear what do you mean here?

If you just return you will have the SCI stuff that you did a few lines
above installed... I am concerned whether it is a good idea.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/