On Fri, Apr 18, 2008 at 08:42:29PM +0100, Peter Stokes wrote:
>
> Ville,
>
> My apologies for not responding to your feedback sooner. I had not noticed the
> comments that you had inserted within the patch. Please find my responses
> below:
Sorry it took so long to respond. I'll just throw some comments here but
I'll also post a modifed patch that I cooked up.
> On Tuesday 08 April 2008 21:18:26 Ville Syrjälä wrote:
> > On Sat, Apr 05, 2008 at 07:30:10PM +0100, Peter Stokes wrote:
> > > +static int ati_remote2_getkeycode(struct input_dev *idev,
> > > + int scancode, int *keycode)
> > > +{
> > > + struct ati_remote2 *ar2 = input_get_drvdata(idev);
> > > + int index, mode;
> > > +
> > > + if (((scancode >> 8) & mode_mask) != (scancode >> 8))
> > > + return -EINVAL;
> > > +
> > > + index = ati_remote2_lookup(scancode & 0xFF);
> > > + if (index == -1)
> > > + return -EINVAL;
> > > +
> > > + for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
> > > + if ((1 << mode) & (scancode >> 8)) {
> > > + *keycode = ar2->keycode[index][mode];
> > > + return 0;
> > > + }
> > > + }
> >
> > Is there a reason you using the high bits like a mask? IIRC the hardware
> > mode byte isn't a mask. So something like this should do:
> >
> > mode = scancode >> 8;
> > if (mode < 0 || mode > MAX_MODE)
> > return -EINVAL;
> > index = ati_remote2_lookup(scancode & 0xff);
> > if (index < 0)
> > return -EINVAL;
> >
> >
> > I'm not sure if mode_mask should affect the keymap handling at all since
> > both can be changed runtime.
> >
>
> The reason I am using the high bits as a mode mask is in order to address the
> issues I felt some people might have regarding the interpretation of the
> device that allows mapping separate keycodes to each physical button
> dependant upon which mode the remote control is currently in. This approach
> address two issues.
>
> Firstly, whilst the input system provides a mechanism to configure the
> scancode to keycode mapping on a per-device basis (each device can have it's
> own mapping table) unfortunately the mechanism by which the mapping tables
> can be manipulated (via the 'setkeycodes' utility) does not allow for the
> device that should be modified to be specified. What actually appears to
> happen is that the first device that accepts the specified scancode as being
> one it can produce changes it's mapping for that scancode to use the new
> keycode provided. The practical upshot of this is that I wish to be able to
> remap the keycodes associated with the remote control but if the scancodes
> the remote control reports are within the normal range associated with a
> regular keyboard (i.e. <=255) then the keyboard driver accepts the change
> first. This not only results in the ati_remote2 not even seeing the request
> for the new mapping it also has the undesirable side effect of corrupting the
> mappings for the regular keyboard. To overcome this problem, I decided that
> instead of using the mode byte as is (i.e. a number between 0 and 4) I would
> convert it into a bit mask, thus moving all the remote's reported scancodes
> (0x0100 -> 0x1FF9) outside to the regular keyboard range.
That just tells me that setkeycodes is the wrong tool for this job.
Trying to work around bugs/misfeatures in a simple tool like that inside
the kernel is no good. Writing a small tool that directly uses the event
device (or patching setkeycodes) is trivial enough.
> Secondly. whilst I decided to provide the flexibility of a mechanism by which
> a single key may be mapped to different keycodes dependant upon which mode
> the remote is currently in, I expected that not everyone would need that
> functionality. A potential downside to providing the flexible implementation
> could be that if a user simply wanted to change the mapping for a given
> physical button regardless of what mode the remote is currently in then it
> might be necessary to submit five requests to update the mappings for each
> mode. By converting the mode byte into a mode bit mask it allows a mapping
> request to simultaneously update the mapping for a single physical button for
> all modes. So, for example:
>
> setkeycodes 0x015C 28 - Would map the OK button for AUX1 mode to be 'enter'
> setkeycodes 0x1F5C 28 - Would map the OK button for all modes to be 'enter'
>
> The usage of the mode_mask module parameter in the 'ati_remote2_getkeycode'
> and 'ati_remote2_setkeycode' is only used determine whether the driver, as
> currently configured, will respond to the specified scancode. If the driver
> will not currently respond, because a given mode is masked out, then it is
> not possible to request or modify the keymapping for a physical button in
> that mode.
I understand but I still think it's the wrong thing to do. The API is
documented to do 1:1 mapping and you're making it do something else.
Also getkeycode can't really handle a mask since you'd need to return
multiple keycodes for one scancode which would make API strangely
assymmetric (and not to mention confusing for unwary users).
> > > +
> > > + return -EINVAL;
> > > +}
> > > +
> > > +static int ati_remote2_setkeycode(struct input_dev *idev,
> > > + int scancode, int keycode)
> > > +{
> > > + struct ati_remote2 *ar2 = input_get_drvdata(idev);
> > > + int old_keycode = -1;
> > > + int index, mode;
> > > +
> > > + if (((scancode >> 8) & mode_mask) != (scancode >> 8))
> > > + return -EINVAL;
> > > +
> >
> > Same comments as above.
> >
> > > + index = ati_remote2_lookup(scancode & 0xFF);
> > > + if (index == -1)
> > > + return -EINVAL;
> > > +
> > > + if (keycode < 0 || keycode > KEY_MAX)
> >
> > Are KEY_RESERVED/KEY_MAX valid here?
> >
>
> Yes, this is merely checking that the keycode provided is within the range
> expected by the linux input system.
I meant that it now accepts KEY_RESERVED and KEY_MAX. Are those supposed
to be valid keycodes? Hmm, I suppose KEY_MAX at least is supposed to valid
judging from REP_MAX and FF_STATUS_MAX.
> > > + return -EINVAL;
> > > +
> > > + for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
> > > + if ((1 << mode) & (scancode >> 8)) {
> > > + old_keycode = ar2->keycode[index][mode];
> > > + ar2->keycode[index][mode] = keycode;
> > > + }
> > > + }
> > > +
> > > + set_bit(keycode, idev->keybit);
> > > +
> > > + for (index = 0; index < ARRAY_SIZE(ati_remote2_keycodes); index++) {
> > > + for (mode = 0; mode < ARRAY_SIZE(ati_remote2_modes); mode++) {
> > > + if (ar2->keycode[index][mode] == old_keycode)
> > > + return 0;
> > > + }
> > > + }
> >
> > Because of using the scancode high bits as a mask this could leave some
> > bits set even though they are no longer in the map.
> >
>
> No, in this function the 'mode' variable is an index into the ar2->keycode
> table (i.e. a number between 0 and 4). The implementation is attempting to
> ascertain whether or not the 'old_keycode' is still in use within the key
> table. The implementation does not filter the contents of idev->keybit based
> upon whether a given keycode is only used by the remote in modes that are
> currently masked off. Is that what you meant?
No. I meant that scancode is a mask so you could end up updating several
keys in the keycode table but you clear the keybit only for the last keycode
to be overwritten. One or more of the other overwritten keycodes could
have also disappeared from the keycode table but still be present in keybit.
Example:
Let's say you have KEY_FOO and KEY_BAR mapped to the same button in
modes AUX1 and AUX2. No other key is mapped to KEY_FOO or KEY_BAR.
You then call setkeycode with the mask set to AUX1|AUX2 and KEY_NEW.
The code would basically do this:
old_keycode = KEY_FOO;
keycode[button][AUX1] = KEY_NEW;
old_keycode = KEY_BAR;
keycode[button][AUX2] = KEY_NEW;
set_bit(KEY_NEW, keybit);
clear_bit(KEY_BAR, keybit);
In the end KEY_FOO would still be present in keybit even though it was
overwritten in the keycode table.
But anyways I got rid of this mask feature in my modifed patch.
--
Ville Syrjälä
syrjala@xxxxxx
http://www.sci.fi/~syrjala/
--
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
