Noticed by sparse:
drivers/media/dvb/dvb-usb/af9015.c:756:25: warning: restricted __le16 degrades to integer
drivers/media/dvb/dvb-usb/af9015.c:744:28: warning: restricted __le16 degrades to integer
Signed-off-by: Harvey Harrison <[email protected]>
---
drivers/media/dvb/dvb-usb/af9015.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
index 4b2af32..eb4495a 100644
--- a/drivers/media/dvb/dvb-usb/af9015.c
+++ b/drivers/media/dvb/dvb-usb/af9015.c
@@ -742,7 +742,7 @@ static int af9015_read_config(struct usb_device *udev)
}
} else {
switch (udev->descriptor.idVendor) {
- case USB_VID_LEADTEK:
+ case cpu_to_le16(USB_VID_LEADTEK):
af9015_properties[i].rc_key_map =
af9015_rc_keys_leadtek;
af9015_properties[i].rc_key_map_size =
@@ -752,9 +752,9 @@ static int af9015_read_config(struct usb_device *udev)
af9015_config.ir_table_size =
ARRAY_SIZE(af9015_ir_table_leadtek);
break;
- case USB_VID_VISIONPLUS:
+ case cpu_to_le16(USB_VID_VISIONPLUS):
if (udev->descriptor.idProduct ==
- USB_PID_AZUREWAVE_AD_TU700) {
+ cpu_to_le16(USB_PID_AZUREWAVE_AD_TU700)) {
af9015_properties[i].rc_key_map =
af9015_rc_keys_twinhan;
af9015_properties[i].rc_key_map_size =
@@ -765,7 +765,7 @@ static int af9015_read_config(struct usb_device *udev)
ARRAY_SIZE(af9015_ir_table_twinhan);
}
break;
- case USB_VID_KWORLD_2:
+ case cpu_to_le16(USB_VID_KWORLD_2):
/* TODO: use correct rc keys */
af9015_properties[i].rc_key_map =
af9015_rc_keys_twinhan;
@@ -778,7 +778,7 @@ static int af9015_read_config(struct usb_device *udev)
/* Check USB manufacturer and product strings and try
to determine correct remote in case of chip vendor
reference IDs are used. */
- case USB_VID_AFATECH:
+ case cpu_to_le16(USB_VID_AFATECH):
memset(manufacturer, 0, sizeof(manufacturer));
usb_string(udev, udev->descriptor.iManufacturer,
manufacturer, sizeof(manufacturer));
@@ -806,7 +806,7 @@ static int af9015_read_config(struct usb_device *udev)
ARRAY_SIZE(af9015_ir_table_msi);
}
break;
- case USB_VID_AVERMEDIA:
+ case cpu_to_le16(USB_VID_AVERMEDIA):
af9015_properties[i].rc_key_map =
af9015_rc_keys_avermedia;
af9015_properties[i].rc_key_map_size =
--
1.6.0.4.879.g9d19a
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.
Harvey
Harvey, et al:
Harvey Harrison wrote:
> Noticed by sparse:
> drivers/media/dvb/dvb-usb/af9015.c:756:25: warning: restricted __le16 degrades to integer
> drivers/media/dvb/dvb-usb/af9015.c:744:28: warning: restricted __le16 degrades to integer
>
> Signed-off-by: Harvey Harrison <[email protected]>
> ---
> drivers/media/dvb/dvb-usb/af9015.c | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-usb/af9015.c b/drivers/media/dvb/dvb-usb/af9015.c
> index 4b2af32..eb4495a 100644
> --- a/drivers/media/dvb/dvb-usb/af9015.c
> +++ b/drivers/media/dvb/dvb-usb/af9015.c
> @@ -742,7 +742,7 @@ static int af9015_read_config(struct usb_device *udev)
> }
> } else {
> switch (udev->descriptor.idVendor) {
> - case USB_VID_LEADTEK:
> + case cpu_to_le16(USB_VID_LEADTEK):
> af9015_properties[i].rc_key_map =
> af9015_rc_keys_leadtek;
> af9015_properties[i].rc_key_map_size =
>
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.
Regards,
Mike
On Fri, 2008-11-14 at 14:15 -0500, Michael Krufky wrote:
> On Fri, Nov 14, 2008 at 2:07 PM, Harvey Harrison
> <[email protected]> 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
On Fri, Nov 14, 2008 at 2:07 PM, Harvey Harrison
<[email protected]> 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.
-Mike
On Fri, Nov 14, 2008 at 2:20 PM, Harvey Harrison
<[email protected]> wrote:
> On Fri, 2008-11-14 at 14:15 -0500, Michael Krufky wrote:
>> On Fri, Nov 14, 2008 at 2:07 PM, Harvey Harrison
>> <[email protected]> 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
On Fri, 2008-11-14 at 14:25 -0500, Devin Heitmueller wrote:
> > 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.
Well, I can see your point, and I don't even know what the chances are of
even finding this hardware on a big-endian machine are. But I'd suggest
that if you want to improve the maintainability of this code that someone
in v4l-land start running sparse regularly and work on getting some of these
_trivial_ parts annotated, then any new ones that get introduced will stick
out like sore thumbs.
But, not my driver, not my decision. Someone else can implement Michael's
suggestion if they want.
Harvey
On Fri, 2008-11-14 at 11:44 -0800, Harvey Harrison wrote:
> But I'd suggest
> that if you want to improve the maintainability of this code that someone
> in v4l-land start running sparse regularly
For those who don't get the daily 'bot mail on the v4l-dvb-maintainer
list, Hans Verkuil's v4l-dvb build 'bot runs sparse builds daily for all
of v4l-dvb land:
http://linuxtv.org/pipermail/v4l-dvb-maintainer/2008-November/008595.html
Although developers won't see waring/error outputs from those runs until
after their changes are merged in the main v4l-dvb repo.
The latest kernel this automatic sparse build compiles is currently
2.26.28-rc4. I've used it to monitor the cx18 driver for sparse build
errors so I could address them - it's quite convenient.
Regards,
Andy
On Fri, 2008-11-14 at 15:55 -0500, Andy Walls wrote:
> On Fri, 2008-11-14 at 11:44 -0800, Harvey Harrison wrote:
>
> > But I'd suggest
> > that if you want to improve the maintainability of this code that someone
> > in v4l-land start running sparse regularly
>
> For those who don't get the daily 'bot mail on the v4l-dvb-maintainer
> list, Hans Verkuil's v4l-dvb build 'bot runs sparse builds daily for all
> of v4l-dvb land:
>
> http://linuxtv.org/pipermail/v4l-dvb-maintainer/2008-November/008595.html
>
> Although developers won't see waring/error outputs from those runs until
> after their changes are merged in the main v4l-dvb repo.
>
> The latest kernel this automatic sparse build compiles is currently
> 2.26.28-rc4. I've used it to monitor the cx18 driver for sparse build
> errors so I could address them - it's quite convenient.
>
Does Hans' build turn on __CHECK_ENDIAN__? That would help as well.
Harvey
On Fri, 2008-11-14 at 11:44 -0800, Harvey Harrison wrote:
> But, not my driver, not my decision. Someone else can implement Michael's
> suggestion if they want.
A simpler solution could be to pass the struct usb_device_id *id to the
function af9015_read_config. There, the idVendor and idProduct are host
native...
--
Ken ar c'henta? | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On Friday 14 November 2008 22:08:45 Harvey Harrison wrote:
> On Fri, 2008-11-14 at 15:55 -0500, Andy Walls wrote:
> > On Fri, 2008-11-14 at 11:44 -0800, Harvey Harrison wrote:
> > > But I'd suggest
> > > that if you want to improve the maintainability of this code that
> > > someone in v4l-land start running sparse regularly
> >
> > For those who don't get the daily 'bot mail on the
> > v4l-dvb-maintainer list, Hans Verkuil's v4l-dvb build 'bot runs
> > sparse builds daily for all of v4l-dvb land:
> >
> > http://linuxtv.org/pipermail/v4l-dvb-maintainer/2008-November/00859
> >5.html
> >
> > Although developers won't see waring/error outputs from those runs
> > until after their changes are merged in the main v4l-dvb repo.
> >
> > The latest kernel this automatic sparse build compiles is currently
> > 2.26.28-rc4. I've used it to monitor the cx18 driver for sparse
> > build errors so I could address them - it's quite convenient.
>
> Does Hans' build turn on __CHECK_ENDIAN__? That would help as well.
Now it does. Thanks for the tip!
Hans