From: Mauro Carvalho Chehab <mchehab@redhat.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Patrick Boettcher <pboettcher@kernellabs.com>,
"Igor M. Liplianin" <liplianin@me.by>,
linux-media@vger.kernel.org, Eduard Bloch <blade@debian.org>
Subject: Re: [RFC/PATCH] [media] dw2102: use symbolic names for dw2102_table indices
Date: Fri, 06 Jan 2012 11:20:18 -0200 [thread overview]
Message-ID: <4F06F512.9090704@redhat.com> (raw)
In-Reply-To: <20111223230045.GE21769@elie.Belkin>
On 23-12-2011 21:00, Jonathan Nieder wrote:
> dw2102_properties et al refer to entries in the USB-id table using
> hard-coded indices, as in "&dw2102_table[6]", which means adding new
> entries before the end of the list has the potential to introduce bugs
> in code elsewhere in the file.
>
> Use C99-style initializers with symbolic names for each index to avoid
> this. This way, other device tables wanting to reuse the USB ids can
> use expressions like "&dw2102_table[TEVII_S630]" that do not change as
> the entries in the table are reordered.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Patrick Boettcher wrote:
>
>> Due to the use of the reference in the USB-id table adding the new set at
>> the end of the list is actually the best way. Adding them in the middle will
>> cause a lot of changes and bugs.
>
> Good catch. That seems like an accident waiting to happen. How about
> something like this (untested)?
>
> drivers/media/dvb/dvb-usb/dw2102.c | 78 ++++++++++++++++++++++--------------
> 1 files changed, 48 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-usb/dw2102.c b/drivers/media/dvb/dvb-usb/dw2102.c
> index 64add7cb1fd3..9a1863dc7232 100644
> --- a/drivers/media/dvb/dvb-usb/dw2102.c
> +++ b/drivers/media/dvb/dvb-usb/dw2102.c
> @@ -1435,22 +1435,40 @@ static int dw2102_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
> return 0;
> }
>
> +enum dw2102_table_entry {
> + CYPRESS_DW2102,
> + CYPRESS_DW2101,
> + CYPRESS_DW2104,
> + TEVII_S650,
> + TERRATEC_CINERGY_S,
> + CYPRESS_DW3101,
> + TEVII_S630,
> + PROF_1100,
> + TEVII_S660,
> + PROF_7500,
> + GENIATECH_SU3000,
> + TERRATEC_CINERGY_S2,
> + TEVII_S480_1,
> + TEVII_S480_2,
> + X3M_SPC1400HD,
> +};
> +
> static struct usb_device_id dw2102_table[] = {
> - {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2102)},
> - {USB_DEVICE(USB_VID_CYPRESS, 0x2101)},
> - {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2104)},
> - {USB_DEVICE(0x9022, USB_PID_TEVII_S650)},
> - {USB_DEVICE(USB_VID_TERRATEC, USB_PID_CINERGY_S)},
> - {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW3101)},
> - {USB_DEVICE(0x9022, USB_PID_TEVII_S630)},
> - {USB_DEVICE(0x3011, USB_PID_PROF_1100)},
> - {USB_DEVICE(0x9022, USB_PID_TEVII_S660)},
> - {USB_DEVICE(0x3034, 0x7500)},
> - {USB_DEVICE(0x1f4d, 0x3000)},
> - {USB_DEVICE(USB_VID_TERRATEC, 0x00a8)},
> - {USB_DEVICE(0x9022, USB_PID_TEVII_S480_1)},
> - {USB_DEVICE(0x9022, USB_PID_TEVII_S480_2)},
> - {USB_DEVICE(0x1f4d, 0x3100)},
> + [CYPRESS_DW2102] = {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2102)},
> + [CYPRESS_DW2101] = {USB_DEVICE(USB_VID_CYPRESS, 0x2101)},
> + [CYPRESS_DW2104] = {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2104)},
> + [TEVII_S650] = {USB_DEVICE(0x9022, USB_PID_TEVII_S650)},
> + [TERRATEC_CINERGY_S] = {USB_DEVICE(USB_VID_TERRATEC, USB_PID_CINERGY_S)},
> + [CYPRESS_DW3101] = {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW3101)},
> + [TEVII_S630] = {USB_DEVICE(0x9022, USB_PID_TEVII_S630)},
> + [PROF_1100] = {USB_DEVICE(0x3011, USB_PID_PROF_1100)},
> + [TEVII_S660] = {USB_DEVICE(0x9022, USB_PID_TEVII_S660)},
> + [PROF_7500] = {USB_DEVICE(0x3034, 0x7500)},
> + [GENIATECH_SU3000] = {USB_DEVICE(0x1f4d, 0x3000)},
> + [TERRATEC_CINERGY_S2] = {USB_DEVICE(USB_VID_TERRATEC, 0x00a8)},
> + [TEVII_S480_1] = {USB_DEVICE(0x9022, USB_PID_TEVII_S480_1)},
> + [TEVII_S480_2] = {USB_DEVICE(0x9022, USB_PID_TEVII_S480_2)},
> + [X3M_SPC1400HD] = {USB_DEVICE(0x1f4d, 0x3100)},
> { }
> };
>
> @@ -1610,15 +1628,15 @@ static struct dvb_usb_device_properties dw2102_properties = {
> .num_device_descs = 3,
> .devices = {
> {"DVBWorld DVB-S 2102 USB2.0",
> - {&dw2102_table[0], NULL},
> + {&dw2102_table[CYPRESS_DW2102], NULL},
> {NULL},
> },
> {"DVBWorld DVB-S 2101 USB2.0",
> - {&dw2102_table[1], NULL},
> + {&dw2102_table[CYPRESS_DW2101], NULL},
> {NULL},
> },
> {"TerraTec Cinergy S USB",
> - {&dw2102_table[4], NULL},
> + {&dw2102_table[TERRATEC_CINERGY_S], NULL},
> {NULL},
> },
> }
> @@ -1664,11 +1682,11 @@ static struct dvb_usb_device_properties dw2104_properties = {
> .num_device_descs = 2,
> .devices = {
> { "DVBWorld DW2104 USB2.0",
> - {&dw2102_table[2], NULL},
> + {&dw2102_table[CYPRESS_DW2104], NULL},
> {NULL},
> },
> { "TeVii S650 USB2.0",
> - {&dw2102_table[3], NULL},
> + {&dw2102_table[TEVII_S650], NULL},
> {NULL},
> },
> }
> @@ -1715,7 +1733,7 @@ static struct dvb_usb_device_properties dw3101_properties = {
> .num_device_descs = 1,
> .devices = {
> { "DVBWorld DVB-C 3101 USB2.0",
> - {&dw2102_table[5], NULL},
> + {&dw2102_table[CYPRESS_DW3101], NULL},
> {NULL},
> },
> }
> @@ -1761,7 +1779,7 @@ static struct dvb_usb_device_properties s6x0_properties = {
> .num_device_descs = 1,
> .devices = {
> {"TeVii S630 USB",
> - {&dw2102_table[6], NULL},
> + {&dw2102_table[TEVII_S630], NULL},
> {NULL},
> },
> }
> @@ -1770,33 +1788,33 @@ static struct dvb_usb_device_properties s6x0_properties = {
> struct dvb_usb_device_properties *p1100;
> static struct dvb_usb_device_description d1100 = {
> "Prof 1100 USB ",
> - {&dw2102_table[7], NULL},
> + {&dw2102_table[PROF_1100], NULL},
> {NULL},
> };
>
> struct dvb_usb_device_properties *s660;
> static struct dvb_usb_device_description d660 = {
> "TeVii S660 USB",
> - {&dw2102_table[8], NULL},
> + {&dw2102_table[TEVII_S660], NULL},
> {NULL},
> };
>
> static struct dvb_usb_device_description d480_1 = {
> "TeVii S480.1 USB",
> - {&dw2102_table[12], NULL},
> + {&dw2102_table[TEVII_S480_1], NULL},
> {NULL},
> };
>
> static struct dvb_usb_device_description d480_2 = {
> "TeVii S480.2 USB",
> - {&dw2102_table[13], NULL},
> + {&dw2102_table[TEVII_S480_2], NULL},
> {NULL},
> };
>
> struct dvb_usb_device_properties *p7500;
> static struct dvb_usb_device_description d7500 = {
> "Prof 7500 USB DVB-S2",
> - {&dw2102_table[9], NULL},
> + {&dw2102_table[PROF_7500], NULL},
> {NULL},
> };
>
> @@ -1842,15 +1860,15 @@ static struct dvb_usb_device_properties su3000_properties = {
> .num_device_descs = 3,
> .devices = {
> { "SU3000HD DVB-S USB2.0",
> - { &dw2102_table[10], NULL },
> + { &dw2102_table[GENIATECH_SU3000], NULL },
> { NULL },
> },
> { "Terratec Cinergy S2 USB HD",
> - { &dw2102_table[11], NULL },
> + { &dw2102_table[TERRATEC_CINERGY_S2], NULL },
> { NULL },
> },
> { "X3M TV SPC1400HD PCI",
> - { &dw2102_table[14], NULL },
> + { &dw2102_table[X3M_SPC1400HD], NULL },
> { NULL },
> },
> }
This looks like a good idea to me. From time to time, when conflict rises,
sometimes those dvb-usb tables with the magic numbers got unnoticed
conflicts.
So, I'm picking this one.
It should be noticed that this is a common constructor used inside the
dvb-usb drivers. IMHO, an approach like that should be extended to the
other drivers as well.
Regards,
Mauro
next prev parent reply other threads:[~2012-01-06 13:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20111222215356.GA4499@rotes76.wohnheim.uni-kl.de>
2011-12-22 23:44 ` Add support for new Terratec DVB USB IDs Jonathan Nieder
2011-12-23 17:20 ` Patrick Boettcher
2011-12-23 23:00 ` [RFC/PATCH] [media] dw2102: use symbolic names for dw2102_table indices Jonathan Nieder
2012-01-06 13:20 ` Mauro Carvalho Chehab [this message]
2012-01-07 8:01 ` [PATCH 0/2] " Jonathan Nieder
2012-01-07 8:05 ` [PATCH 1/2] [media] a800: use symbolic names for a800_table indices Jonathan Nieder
2012-01-07 8:11 ` [PATCH 2/2] [media] af9005, af9015: use symbolic names for USB id table indices Jonathan Nieder
2012-01-07 13:57 ` Luca Olivetti
2012-01-09 17:49 ` Antti Palosaari
2012-01-06 13:12 ` Add support for new Terratec DVB USB IDs Mauro Carvalho Chehab
2012-03-10 16:04 ` [RFC/PATCH] New Terratec DVB USB IDs, symbolic names in az6027_usb_table Eduard Bloch
2012-03-10 16:40 ` Mauro Carvalho Chehab
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F06F512.9090704@redhat.com \
--to=mchehab@redhat.com \
--cc=blade@debian.org \
--cc=jrnieder@gmail.com \
--cc=linux-media@vger.kernel.org \
--cc=liplianin@me.by \
--cc=pboettcher@kernellabs.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.