On Thu, September 28, 2006 1:31, Darren Salt said:
> <URL:http://www.youmustbejoking.demon.co.uk/progs.linux.html#dvb>
>> You might like to look at them since you evidently have functioning
> hardware with which you can test & debug them.
Ok, I've read through the patches...and the overall direction seems to be
good. I've added per-patch comments below:
...
ir_01_ir-common_handle_repeat.patch
Seems like a good idea. Perhaps the debounce-logic should be moved to
ir-common as well since it is a problem common to all IR input drivers?
ir_02_budget-ci_groundwork_and_avoid_name_truncation.patch
ir_03_budget-ci_EVIOCGPHYS.patch
Looks ok
ir_04_budget-ci_error_and_shutdown_fixups_and_verbosity.patch
No objections to the patch, two comments though:
The patch reverses the order of tasklet_kill() and msp430_ir_deinit().
Are you sure that it is correct to deinit the msp430 before killing the
tasklet?
Nitpick: input_event(...EV_KEY...) could be changed to
input_report_key()
ir_05_budget-ci_kzalloc_and_more_verbosity.patch
Looks ok
ir_06_budget-ci_use_ir-common_and_RC5_addresses.patch
Not ok, the "address" (rc5 device) decoding logic will trigger on
invalid data as well (0x80 <= data <= 0xc0), which the MSP430 will
produce from time to time.
In addition there is no logic to read all three bytes and to compare
command1 and command2, meaning that the correlation between rc5 device
and rc5 command may be wrong (the bytes can arrive in any order).
For example, if you receive three bytes from the ir chip:
data(device, rc5dev=30)
data(command1, rc5cmd=18)
data(device, rc5dev=22)
Did you just receive command 18 to device 22 or command 18 to device 30?
I like that it uses the RC5 toggle bit though, and that it only listens
for device 0x1f per default (I had no idea what device code the remotes
usually used so I had the reverse logic in my patch).
ir_07_cx88xx_name_truncation_and_use_RC5_addresses_for_Hauppauge.patch
ir_08_cx88-input_fix_repeat.patch
ir_09_cx88-input_prevent_input_layer_repeat_handler.patch
No comments, unfamiliar hardware...
ir_10_ir-common_compressed_keymaps.patch
ir_11_ir-common_optimise_keymap_sizes.patch
Honestly, this smells of over-engineering. Compressed, compile-time
generated keymaps using a perl-to-c script? I'd rather expect that all
keymaps would be removed from the kernel (long term of course) and moved
to an external program which loads the proper keymap. This would mean
that only the used keymap would take up space and it would be trivial
to add new keymaps without hacking kernel code.
...
So how would you like to proceed with this? It would be nice to cooperate
in order to have the added functionality commited to the dvb repo. Perhaps
it would be a good idea to try to get patches 1 - 5 merged first and I can
try to update the RC5 command decoding of your sixth patch? That way the
first set of changes will also have some time to get some testing before
moving along with more changes...
--
David Härdeman