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
