[PATCH] dvb: usb vendor_ids/product_ids are __le16

[PATCH] dvb: usb vendor_ids/product_ids are __le16

[Harvey Harrison]

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 <harvey.harrison@gmail.com>
---
 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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Michael Krufky]

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 <harvey.harrison@gmail.com>
> ---
>  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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Harvey Harrison]

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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Michael Krufky]

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.

-Mike

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Harvey Harrison]

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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Devin Heitmueller]

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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Harvey Harrison]

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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Andy Walls]

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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Harvey Harrison]

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

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Jean-Francois Moine]

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/

Re: [v4l-dvb-maintainer] [PATCH] dvb: usb vendor_ids/product_ids are __le16

[Hans Verkuil]

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