Messages in this thread | | | Date | Fri, 14 Nov 2008 14:25:57 -0500 | From | "Devin Heitmueller" <> | Subject | Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16 |
| |
On Fri, Nov 14, 2008 at 2:20 PM, Harvey Harrison <harvey.harrison@gmail.com> wrote: > On Fri, 2008-11-14 at 14:15 -0500, Michael Krufky wrote: >> On Fri, Nov 14, 2008 at 2:07 PM, Harvey Harrison >> <harvey.harrison@gmail.com> wrote: >> > On Fri, 2008-11-14 at 14:05 -0500, Michael Krufky wrote: >> >> Wouldn't it be nicer to just switch on >> >> cpu_to_le16(udev->descriptor.idVendor) ? This would be a 1-line change, >> >> compile to a smaller footprint, and be easier to read. >> >> >> >> Personally, I prefer to try to avoid duplicating code in places where a >> >> single operation may occur centrally. >> > >> > On a little-endian arch it makes no difference obviously, but on a >> > big-endian arch it's the difference between compile-time and runtime >> > byteswapping. >> > >> >> Its not my driver, but I think that doing the operation on the switch >> is prettier than doing it on each case statement. Also, it would >> avoid future bugs, if somebody decides to add new cases to the switch >> block. > > The alternative is to define the vendor ids as little-endian in the headers > then you don't need the endian swap in the switch or the case, but that would > require looking at the other uses first....or gently encourage more people to run sparse ;-) > > Harvey
Harvey,
If I may offer my opinion, this is not remotely performance-critical code. That said, I would favor on the side of maintainability/reliability in this case, and mkrufky's approach seems to be better on both accounts.
Regards,
Devin
-- Devin J. Heitmueller http://www.devinheitmueller.com AIM: devinheitmueller
| |