On Dec 29, 2009, at 5:30 PM, Dmitry Torokhov wrote: > On Tue, Dec 29, 2009 at 12:04:00AM -0500, Jarod Wilson wrote: >> On Dec 28, 2009, at 4:31 AM, Dmitry Torokhov wrote: >>> >>> Hm, will this work on big-endian? >> >> Good question. Not sure offhand. Probably not. Unfortunately, the only >> devices I have to test with at the moment are integrated into cases with x86 >> boards in them, so testing isn't particularly straight-forward. I should >> probably get ahold of one of the plain external usb devices to play with... >> Mind if I just add a TODO marker near that for now? >> > > How about just do le32_to_cpu() instead of memcpy()ing and that's > probably it?
Hrm. My endian-fu sucks. Not sure what the right way to do it without the memcpy is. I thought I had something together than would work, using 2 lines (memcpy, then le32_to_cpu), but I just realized that'll go south on the keys where we're only looking at half the buffer (i.e., the wrong half on big-endian)... Will see what I can do to fix that up in the morning, was hoping to get this out tonight, but its nearly 3am (again)... Also, forgot to reply to this bit last time: >> + init_completion(&context->tx.finished); >> + atomic_set(&(context->tx.busy), 1); > > What does this atomic give you? Atomic operations do not imply memory > barriers IIRC... Code is borrowed from lirc_imon from before my time, honestly hadn't really looked into that much until now. I'm guessing it was *supposed* to provide an assurance that later readers saw the correct value for busy, but indeed, I don't think its actually guaranteeing that. I've changed busy to a bool and inserted smp_rmb()'s after each change to it, which I think *will* provide the desired guarantee. (In practice, its been working just fine either way for me so far). >>> We have pretty different behavior depending on the interface, maybe the >>> driver should be split further? >> >> This is what we'll call a "fun" topic... These devices expose two >> interfaces, and a while back in the lirc_imon days, they actually loaded up >> as two separate lirc devices. But there's a catch: they can't operate >> independently. Some keys come in via intf0, some via intf1, even from the >> very same remote. And the interfaces share a hardware-internal buffer (or >> something), and if you're only listening to one of the two devices, and a >> key is decoded and sent via the interface you're not listening to, it wedges >> the entire device until you flush the other interface. Horribly bad hardware >> design at play there, imo, but meh. What exactly did you have in mind as far >> as a split? (And/or does it still apply with the above info taken into >> consideration? ;). > > Ok, fair enough. I'd still want to see larger functions split up a bit though. I've hacked imon_probe() up into 6 different functions now: imon_probe() -> imon_init_intf0() -> imon_init_idev() -> imon_init_display() -> imon_init_intf1() -> imon_init_touch() (and there was an existing imon_set_ir_protocol() in the mix) Quite a bit more manageable and readable now, I think. Haven't looked yet for other candidates for similar refactoring. Were there others you had in mind? At a glance, imon_incoming_packet() seems to be the only remaining function that is particularly hefty. Well, imon_probe() is still ~180 lines, but it used to be north of 400, I think... So perhaps I try trimming imon_probe() a bit more, and see what can be done to make imon_incoming_packet() less chunky. Current work-in-progress pushed to git.kernel.org again. Thanks much, -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
