Re: [PATCH] cxusb: Use enum to represent table offsets rather than hard-coding numbers

[Mauro Carvalho Chehab <mchehab@osg.samsung.com>]

Em Tue, 17 Feb 2015 13:45:50 +0000
David Howells <dhowells@redhat.com> escreveu:

> Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote:
> 
> > I would do a s/ix_USB_PID_// in the above, in order to simplify the
> > namespace and to avoid giving the false impression that those are vendor
> > IDs.
> 
> Okay.
> 
> > If you look below on your patch, even you forgot to add a "ix_" prefix into
> > one of the entires ;)
> 
> Bah.  I realised I'd forgotten and went back to try and fix them up.
> 
> > Just calling MEDION_MD95700..MYGICA_T230 would be enough and shorter.
> 
> True.
> 
> > static struct usb_device_id cxusb_table [] = {
> > 	[VID_MEDION] = {USB_VID_MEDION,	USB_PID_MEDION_MD95700},
> > ...
> 
> That should really be:
> 
> 	[VID_MEDION_MD95700] = {USB_VID_MEDION,	USB_PID_MEDION_MD95700},

Actually, MEDION_MD95700 :)

> 
> since the index number is the model, not the vendor, which brings me to:
> 
> 	[DVICO_BLUEBIRD_DVB_T_NANO_2_NFW_WARM] = {USB_VID_DVICO, USB_PID_DVICO_BLUEBIRD_DVB_T_NANO_2_NFW_WARM},
> 
> which would be excessively long.

True. Perhaps:

	[DVICO_BLUEBIRD_DVB_T_NANO_2_NFW_WARM] = {
		USB_VID_DVICO, USB_PID_DVICO_BLUEBIRD_DVB_T_NANO_2_NFW_WARM
	},

Would be better, as it follows better the CodingStyle.

> 
> > > +	_(USB_VID_MEDION,	USB_PID_MEDION_MD95700), // 0
> > 
> > Please don't use c99 comments. Also, I don't think that the comments would
> > help, as the entries on this table doesn't need to follow the same order
> > as defined at the enum.
> 
> Sorry, yes, I meant those as guides purely for when I was converting numbers
> to symbols.
> 
> David

Thanks!
Mauro