Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

Re: [PATCH] ene_ub6250: Use macros for firmware names

2 views
Skip to first unread message

Greg Kroah-Hartman

unread,
Jul 24, 2012, 4:40:02 PM7/24/12
to
On Tue, Jul 24, 2012 at 02:31:09PM -0600, Tim Gardner wrote:
> Advertise firmware files using MODULE_FIRMWARE macros.
>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: linu...@vger.kernel.org
> Cc: usb-s...@lists.one-eyed-alien.net
> Signed-off-by: Tim Gardner <tim.g...@canonical.com>
> ---
> drivers/usb/storage/ene_ub6250.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
> index b28f2ad..3fec82f 100644
> --- a/drivers/usb/storage/ene_ub6250.c
> +++ b/drivers/usb/storage/ene_ub6250.c
> @@ -29,9 +29,21 @@
> #include "protocol.h"
> #include "debug.h"
>
> +#define SD_INIT1_FIRMWARE "ene-ub6250/sd_init1.bin"
> +#define SD_INIT2_FIRMWARE "ene-ub6250/sd_init2.bin"
> +#define SD_RW_FIRMWARE "ene-ub6250/sd_rdwr.bin"
> +#define MS_INIT_FIRMWARE "ene-ub6250/ms_init.bin"
> +#define MSP_RW_FIRMWARE "ene-ub6250/msp_rdwr.bin"
> +#define MS_RW_FIRMWARE "ene-ub6250/ms_rdwr.bin"
> +
> MODULE_DESCRIPTION("Driver for ENE UB6250 reader");
> MODULE_LICENSE("GPL");
> -
> +MODULE_FIRMWARE(SD_INIT1_FIRMWARE);
> +MODULE_FIRMWARE(SD_INIT2_FIRMWARE);
> +MODULE_FIRMWARE(SD_RW_FIRMWARE);
> +MODULE_FIRMWARE(MS_INIT_FIRMWARE);
> +MODULE_FIRMWARE(MSP_RW_FIRMWARE);
> +MODULE_FIRMWARE(MS_RW_FIRMWARE);

Why do you need the #defines here at all? What's wrong with just using
the file names in the MODULE_FIRMWARE() macro directly? That cuts the
size of the patch in half :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Tim Gardner

unread,
Jul 24, 2012, 5:00:02 PM7/24/12
to
/*
* The table of devices
@@ -1883,28 +1895,28 @@ static int ene_load_bincode(struct us_data *us, unsigned char flag)
/* For SD */
case SD_INIT1_PATTERN:
US_DEBUGP("SD_INIT1_PATTERN\n");
- fw_name = "ene-ub6250/sd_init1.bin";
+ fw_name = SD_INIT1_FIRMWARE;
break;
case SD_INIT2_PATTERN:
US_DEBUGP("SD_INIT2_PATTERN\n");
- fw_name = "ene-ub6250/sd_init2.bin";
+ fw_name = SD_INIT2_FIRMWARE;
break;
case SD_RW_PATTERN:
US_DEBUGP("SD_RDWR_PATTERN\n");
- fw_name = "ene-ub6250/sd_rdwr.bin";
+ fw_name = SD_RW_FIRMWARE;
break;
/* For MS */
case MS_INIT_PATTERN:
US_DEBUGP("MS_INIT_PATTERN\n");
- fw_name = "ene-ub6250/ms_init.bin";
+ fw_name = MS_INIT_FIRMWARE;
break;
case MSP_RW_PATTERN:
US_DEBUGP("MSP_RW_PATTERN\n");
- fw_name = "ene-ub6250/msp_rdwr.bin";
+ fw_name = MSP_RW_FIRMWARE;
break;
case MS_RW_PATTERN:
US_DEBUGP("MS_RW_PATTERN\n");
- fw_name = "ene-ub6250/ms_rdwr.bin";
+ fw_name = MS_RW_FIRMWARE;
break;
default:
US_DEBUGP("----------- Unknown PATTERN ----------\n");
--
1.7.9.5

Greg Kroah-Hartman

unread,
Jul 24, 2012, 5:30:01 PM7/24/12
to
On Tue, Jul 24, 2012 at 03:00:06PM -0600, Tim Gardner wrote:
> If the firmware file name ever changes, then you'll have to find and
> modify it in 2 places.

Oops, sorry, I missed the second place it was used in the file,
nevermind, time for more coffee...

Tim Gardner

unread,
Jul 24, 2012, 5:40:01 PM7/24/12
to
On 07/24/2012 02:34 PM, Greg Kroah-Hartman wrote:
If the firmware file name ever changes, then you'll have to find and
modify it in 2 places.

I don't really have a strong preference, but I would like to see
MODULE_FIRMWARE() used so I can cut down on the number of false
positives as I go through the kernel firmware directory and the
linux-firmware package to filter out unused files using modinfo.

rtg
--
Tim Gardner tim.g...@canonical.com

Betty Dall

unread,
Jul 24, 2012, 6:00:01 PM7/24/12
to
Hi Tim,

I reviewed this patch and it looks good. Once small suggestion you can
take or leave...
All the other rdwr patterns are RW not RDWR. So you could change this
one to be SD_RW_PATTERN to be consistent with MSP_RW_PATTERN and
MS_RW_PATTERN. This is a nit.

> - fw_name = "ene-ub6250/sd_rdwr.bin";
> + fw_name = SD_RW_FIRMWARE;
> break;
> /* For MS */
> case MS_INIT_PATTERN:
> US_DEBUGP("MS_INIT_PATTERN\n");
> - fw_name = "ene-ub6250/ms_init.bin";
> + fw_name = MS_INIT_FIRMWARE;
> break;
> case MSP_RW_PATTERN:
> US_DEBUGP("MSP_RW_PATTERN\n");
> - fw_name = "ene-ub6250/msp_rdwr.bin";
> + fw_name = MSP_RW_FIRMWARE;
> break;
> case MS_RW_PATTERN:
> US_DEBUGP("MS_RW_PATTERN\n");
> - fw_name = "ene-ub6250/ms_rdwr.bin";
> + fw_name = MS_RW_FIRMWARE;
> break;
> default:
> US_DEBUGP("----------- Unknown PATTERN ----------\n");

-Betty

Tim Gardner

unread,
Jul 27, 2012, 1:00:01 PM7/27/12
to
Advertise firmware files using MODULE_FIRMWARE macros.

Fix a debug string: SD_RDWR_PATTERN --> SD_RW_PATTERN

Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: linu...@vger.kernel.org
Cc: usb-s...@lists.one-eyed-alien.net
Signed-off-by: Tim Gardner <tim.g...@canonical.com>
---
drivers/usb/storage/ene_ub6250.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index b28f2ad..95edee5 100644
- US_DEBUGP("SD_RDWR_PATTERN\n");
- fw_name = "ene-ub6250/sd_rdwr.bin";
+ US_DEBUGP("SD_RW_PATTERN\n");
+ fw_name = SD_RW_FIRMWARE;
break;
/* For MS */
case MS_INIT_PATTERN:
US_DEBUGP("MS_INIT_PATTERN\n");
- fw_name = "ene-ub6250/ms_init.bin";
+ fw_name = MS_INIT_FIRMWARE;
break;
case MSP_RW_PATTERN:
US_DEBUGP("MSP_RW_PATTERN\n");
- fw_name = "ene-ub6250/msp_rdwr.bin";
+ fw_name = MSP_RW_FIRMWARE;
break;
case MS_RW_PATTERN:
US_DEBUGP("MS_RW_PATTERN\n");
- fw_name = "ene-ub6250/ms_rdwr.bin";
+ fw_name = MS_RW_FIRMWARE;
break;
default:
US_DEBUGP("----------- Unknown PATTERN ----------\n");
--
1.7.9.5
0 new messages