2008-06-22 13:00:55

by Jaswinder Singh Rajput

[permalink] [raw]
Subject: [PATCH] Remove fdump tool for av7110 firmware

There's no point in this, since the user can use the BUILTIN_FIRMWARE
option to include arbitrary firmware files directly in the kernel image.

Thanks to David Woodhouse for help.

Signed-off-by: Jaswinder Singh <[email protected]>
--
drivers/media/dvb/ttpci/Kconfig | 23 +++----------------
drivers/media/dvb/ttpci/Makefile | 9 -------
drivers/media/dvb/ttpci/av7110.c | 16 -------------
drivers/media/dvb/ttpci/fdump.c | 44 -------------------------------------
4 files changed, 4 insertions(+), 88 deletions(-)
diff --git a/drivers/media/dvb/ttpci/Kconfig b/drivers/media/dvb/ttpci/Kconfig
index e0bbcaf..6b7a586 100644
--- a/drivers/media/dvb/ttpci/Kconfig
+++ b/drivers/media/dvb/ttpci/Kconfig
@@ -6,7 +6,7 @@ config DVB_AV7110
tristate "AV7110 cards"
depends on DVB_CORE && PCI && I2C
depends on HOTPLUG
- select FW_LOADER if !DVB_AV7110_FIRMWARE
+ select FW_LOADER
select TTPCI_EEPROM
select VIDEO_SAA7146_VV
depends on VIDEO_DEV # dependencies of VIDEO_SAA7146_VV
@@ -30,6 +30,9 @@ config DVB_AV7110
download/extract it, and then copy it to /usr/lib/hotplug/firmware
or /lib/firmware (depending on configuration of firmware hotplug).

+ Alternatively, you can download the file and use the
+ BUILTIN_FIRMWARE option to build it into your kernel image.
+
Say Y if you own such a card and want to use it.

config DVB_AV7110_BOOTCODE
@@ -39,24 +42,6 @@ config DVB_AV7110_BOOTCODE
This includes firmware for AV7110 bootcode
Say 'N' and let it get loaded from userspace on demand

-config DVB_AV7110_FIRMWARE
- bool "Compile AV7110 firmware into the driver"
- depends on DVB_AV7110 && !STANDALONE
- default y if DVB_AV7110=y
- help
- The AV7110 firmware is normally loaded by the firmware hotplug manager.
- If you want to compile the firmware into the driver you need to say
- Y here and provide the correct path of the firmware. You need this
- option if you want to compile the whole driver statically into the
- kernel.
-
- All other people say N.
-
-config DVB_AV7110_FIRMWARE_FILE
- string "Full pathname of av7110 firmware file"
- depends on DVB_AV7110_FIRMWARE
- default "/usr/lib/hotplug/firmware/dvb-ttpci-01.fw"
-
config DVB_AV7110_OSD
bool "AV7110 OSD support"
depends on DVB_AV7110
diff --git a/drivers/media/dvb/ttpci/Makefile b/drivers/media/dvb/ttpci/Makefile
index d7483f1..a7ff3cd 100644
--- a/drivers/media/dvb/ttpci/Makefile
+++ b/drivers/media/dvb/ttpci/Makefile
@@ -14,12 +14,3 @@ obj-$(CONFIG_DVB_BUDGET_PATCH) += budget-patch.o
obj-$(CONFIG_DVB_AV7110) += dvb-ttpci.o

EXTRA_CFLAGS += -Idrivers/media/dvb/dvb-core/ -Idrivers/media/dvb/frontends/
-
-hostprogs-y := fdump
-
-ifeq ($(CONFIG_DVB_AV7110_FIRMWARE),y)
-$(obj)/av7110.o: $(obj)/av7110_firm.h
-
-$(obj)/av7110_firm.h: $(obj)/fdump
- $(obj)/fdump $(CONFIG_DVB_AV7110_FIRMWARE_FILE) dvb_ttpci_fw $@
-endif
diff --git a/drivers/media/dvb/ttpci/av7110.c b/drivers/media/dvb/ttpci/av7110.c
index 747e7f1..c11a13c 100644
--- a/drivers/media/dvb/ttpci/av7110.c
+++ b/drivers/media/dvb/ttpci/av7110.c
@@ -1497,20 +1497,6 @@ static int check_firmware(struct av7110* av7110)
return 0;
}

-#ifdef CONFIG_DVB_AV7110_FIRMWARE_FILE
-#include "av7110_firm.h"
-static void put_firmware(struct av7110* av7110)
-{
- av7110->bin_fw = NULL;
-}
-
-static inline int get_firmware(struct av7110* av7110)
-{
- av7110->bin_fw = dvb_ttpci_fw;
- av7110->size_fw = sizeof(dvb_ttpci_fw);
- return check_firmware(av7110);
-}
-#else
static void put_firmware(struct av7110* av7110)
{
vfree(av7110->bin_fw);
@@ -1559,8 +1545,6 @@ static int get_firmware(struct av7110* av7110)
release_firmware(fw);
return ret;
}
-#endif
-

static int alps_bsrv2_tuner_set_params(struct dvb_frontend* fe, struct dvb_frontend_parameters *params)
{
diff --git a/drivers/media/dvb/ttpci/fdump.c b/drivers/media/dvb/ttpci/fdump.c
deleted file mode 100644
index c90001d..0000000
--- a/drivers/media/dvb/ttpci/fdump.c
+++ /dev/null
@@ -1,44 +0,0 @@
-#include <stdio.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <unistd.h>
-
-int main(int argc, char **argv)
-{
- unsigned char buf[8];
- unsigned int i, count, bytes = 0;
- FILE *fd_in, *fd_out;
-
- if (argc != 4) {
- fprintf(stderr, "\n\tusage: %s <ucode.bin> <array_name> <output_name>\n\n", argv[0]);
- return -1;
- }
-
- fd_in = fopen(argv[1], "rb");
- if (fd_in == NULL) {
- fprintf(stderr, "firmware file '%s' not found\n", argv[1]);
- return -1;
- }
-
- fd_out = fopen(argv[3], "w+");
- if (fd_out == NULL) {
- fprintf(stderr, "cannot create output file '%s'\n", argv[3]);
- return -1;
- }
-
- fprintf(fd_out, "\n#include <asm/types.h>\n\nu8 %s [] = {", argv[2]);
-
- while ((count = fread(buf, 1, 8, fd_in)) > 0) {
- fprintf(fd_out, "\n\t");
- for (i = 0; i < count; i++, bytes++)
- fprintf(fd_out, "0x%02x, ", buf[i]);
- }
-
- fprintf(fd_out, "\n};\n\n");
-
- fclose(fd_in);
- fclose(fd_out);
-
- return 0;
-}


2008-07-06 01:16:49

by Oliver Endriss

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

Jaswinder Singh wrote:
> There's no point in this, since the user can use the BUILTIN_FIRMWARE
> option to include arbitrary firmware files directly in the kernel image.

NAK! This option allows to compile the firmware into the _driver_,
which is very useful if you want to test various driver/firmware
combinations. Having the firmware in the _kernel_ does not help!

Well, I am tired to fight for this option in the kernel every other
month. :-(

@Mauro:
Is there a way to strip this stuff from Kconfig/Makefile/av7110*.[ch]
for submission to the kernel? Basically I don't care whether and how
they cripple the driver in the kernel. But I would like to keep the code
'as is' in the linuxtv repositories.

CU
Oliver

>
> Thanks to David Woodhouse for help.
>
> Signed-off-by: Jaswinder Singh <[email protected]>
> --
> drivers/media/dvb/ttpci/Kconfig | 23 +++----------------
> drivers/media/dvb/ttpci/Makefile | 9 -------
> drivers/media/dvb/ttpci/av7110.c | 16 -------------
> drivers/media/dvb/ttpci/fdump.c | 44 -------------------------------------
> 4 files changed, 4 insertions(+), 88 deletions(-)
> diff --git a/drivers/media/dvb/ttpci/Kconfig b/drivers/media/dvb/ttpci/Kconfig
> index e0bbcaf..6b7a586 100644
> --- a/drivers/media/dvb/ttpci/Kconfig
> +++ b/drivers/media/dvb/ttpci/Kconfig
> @@ -6,7 +6,7 @@ config DVB_AV7110
> tristate "AV7110 cards"
> depends on DVB_CORE && PCI && I2C
> depends on HOTPLUG
> - select FW_LOADER if !DVB_AV7110_FIRMWARE
> + select FW_LOADER
> select TTPCI_EEPROM
> select VIDEO_SAA7146_VV
> depends on VIDEO_DEV # dependencies of VIDEO_SAA7146_VV
> @@ -30,6 +30,9 @@ config DVB_AV7110
> download/extract it, and then copy it to /usr/lib/hotplug/firmware
> or /lib/firmware (depending on configuration of firmware hotplug).
>
> + Alternatively, you can download the file and use the
> + BUILTIN_FIRMWARE option to build it into your kernel image.
> +
> Say Y if you own such a card and want to use it.
>
> config DVB_AV7110_BOOTCODE
> @@ -39,24 +42,6 @@ config DVB_AV7110_BOOTCODE
> This includes firmware for AV7110 bootcode
> Say 'N' and let it get loaded from userspace on demand
>
> -config DVB_AV7110_FIRMWARE
> - bool "Compile AV7110 firmware into the driver"
> - depends on DVB_AV7110 && !STANDALONE
> - default y if DVB_AV7110=y
> - help
> - The AV7110 firmware is normally loaded by the firmware hotplug manager.
> - If you want to compile the firmware into the driver you need to say
> - Y here and provide the correct path of the firmware. You need this
> - option if you want to compile the whole driver statically into the
> - kernel.
> -
> - All other people say N.
> -
> -config DVB_AV7110_FIRMWARE_FILE
> - string "Full pathname of av7110 firmware file"
> - depends on DVB_AV7110_FIRMWARE
> - default "/usr/lib/hotplug/firmware/dvb-ttpci-01.fw"
> -
> config DVB_AV7110_OSD
> bool "AV7110 OSD support"
> depends on DVB_AV7110
> diff --git a/drivers/media/dvb/ttpci/Makefile b/drivers/media/dvb/ttpci/Makefile
> index d7483f1..a7ff3cd 100644
> --- a/drivers/media/dvb/ttpci/Makefile
> +++ b/drivers/media/dvb/ttpci/Makefile
> @@ -14,12 +14,3 @@ obj-$(CONFIG_DVB_BUDGET_PATCH) += budget-patch.o
> obj-$(CONFIG_DVB_AV7110) += dvb-ttpci.o
>
> EXTRA_CFLAGS += -Idrivers/media/dvb/dvb-core/ -Idrivers/media/dvb/frontends/
> -
> -hostprogs-y := fdump
> -
> -ifeq ($(CONFIG_DVB_AV7110_FIRMWARE),y)
> -$(obj)/av7110.o: $(obj)/av7110_firm.h
> -
> -$(obj)/av7110_firm.h: $(obj)/fdump
> - $(obj)/fdump $(CONFIG_DVB_AV7110_FIRMWARE_FILE) dvb_ttpci_fw $@
> -endif
> diff --git a/drivers/media/dvb/ttpci/av7110.c b/drivers/media/dvb/ttpci/av7110.c
> index 747e7f1..c11a13c 100644
> --- a/drivers/media/dvb/ttpci/av7110.c
> +++ b/drivers/media/dvb/ttpci/av7110.c
> ...

--
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

2008-07-06 05:11:42

by Jaswinder Singh Rajput

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

Hello Oliver,

On Sun, 2008-07-06 at 03:15 +0200, Oliver Endriss wrote:
> Jaswinder Singh wrote:
> > There's no point in this, since the user can use the BUILTIN_FIRMWARE
> > option to include arbitrary firmware files directly in the kernel image.
>
> NAK!

I think you are watching two weeks old patch.

Please check updated patch from :-

http://git.infradead.org/users/dwmw2/firmware-2.6.git?a=commitdiff;h=cf852ae7e54eaf8944e1e734321fa6d5285379f1

And give us your valuable comments.

> This option allows to compile the firmware into the _driver_,

We also compile firmware without fdump.

> which is very useful if you want to test various driver/firmware
> combinations. Having the firmware in the _kernel_ does not help!
>

For testing purpose kernel/drivers source tree is not suitable.
Please find some other suitable place.

> Well, I am tired to fight for this option in the kernel every other
> month. :-(
>

yeah I understand, fighting with Truth is not easy ;)

Thank you,

Jaswinder Singh.

2008-07-06 10:02:52

by Klaus Schmidinger

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

On 07/06/08 03:15, Oliver Endriss wrote:
> Jaswinder Singh wrote:
>> There's no point in this, since the user can use the BUILTIN_FIRMWARE
>> option to include arbitrary firmware files directly in the kernel image.
>
> NAK! This option allows to compile the firmware into the _driver_,
> which is very useful if you want to test various driver/firmware
> combinations. Having the firmware in the _kernel_ does not help!

I strongly support Oliver's request!
Working with various driver versions is much easier with the
firmware compiled into the driver!

Klaus

> Well, I am tired to fight for this option in the kernel every other
> month. :-(
>
> @Mauro:
> Is there a way to strip this stuff from Kconfig/Makefile/av7110*.[ch]
> for submission to the kernel? Basically I don't care whether and how
> they cripple the driver in the kernel. But I would like to keep the code
> 'as is' in the linuxtv repositories.
>
> CU
> Oliver

2008-07-06 11:18:30

by David Woodhouse

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

On Sun, 2008-07-06 at 11:09 +0200, Klaus Schmidinger wrote:
> On 07/06/08 03:15, Oliver Endriss wrote:
> > Jaswinder Singh wrote:
> >> There's no point in this, since the user can use the BUILTIN_FIRMWARE
> >> option to include arbitrary firmware files directly in the kernel image.
> >
> > NAK! This option allows to compile the firmware into the _driver_,
> > which is very useful if you want to test various driver/firmware
> > combinations. Having the firmware in the _kernel_ does not help!
>
> I strongly support Oliver's request!
> Working with various driver versions is much easier with the
> firmware compiled into the driver!

That's strange; I've found exactly the opposite to be the case.

If I want to test permutations of driver and firmware, as I've done for
the libertas driver a number of times, I find it _much_ better to
preserve the modularity. I can build each version of the driver and can
test that against various firmware versions without having to rebuild
it, and with much less chance of something going wrong so that I'm not
actually testing what I think I'm testing.

Perhaps I'm missing something that would help me work better? Please
could you help me understand how you currently work, and I'll attempt to
make it easier for you. Can you talk me through an example of a session
where you had to do this testing of 'various driver/firmware
combinations'?


--
dwmw2

2008-07-06 14:30:00

by Devin Heitmueller

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

On Sun, Jul 6, 2008 at 7:17 AM, David Woodhouse <[email protected]> wrote:
> On Sun, 2008-07-06 at 11:09 +0200, Klaus Schmidinger wrote:
>> On 07/06/08 03:15, Oliver Endriss wrote:
>> > Jaswinder Singh wrote:
>> >> There's no point in this, since the user can use the BUILTIN_FIRMWARE
>> >> option to include arbitrary firmware files directly in the kernel image.
>> >
>> > NAK! This option allows to compile the firmware into the _driver_,
>> > which is very useful if you want to test various driver/firmware
>> > combinations. Having the firmware in the _kernel_ does not help!
>>
>> I strongly support Oliver's request!
>> Working with various driver versions is much easier with the
>> firmware compiled into the driver!
>
> That's strange; I've found exactly the opposite to be the case.
>
> If I want to test permutations of driver and firmware, as I've done for
> the libertas driver a number of times, I find it _much_ better to
> preserve the modularity. I can build each version of the driver and can
> test that against various firmware versions without having to rebuild
> it, and with much less chance of something going wrong so that I'm not
> actually testing what I think I'm testing.
>
> Perhaps I'm missing something that would help me work better? Please
> could you help me understand how you currently work, and I'll attempt to
> make it easier for you. Can you talk me through an example of a session
> where you had to do this testing of 'various driver/firmware
> combinations'?
>
>
> --
> dwmw2
>
>
> _______________________________________________
> linux-dvb mailing list
> [email protected]
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
>

Correct me if I'm wrong, but doesn't this also affect those
distributions that consider kernels with binary firmware blobs to not
be free software? Those distributions take the stance that the
firmware must be loadable by userland, in which case the proposed
patch removes this capability.

Is there some downside to leaving this functionality in there? Are
there known bugs or maintainability issues with the code as-is?

Devin

--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

2008-07-06 14:51:20

by David Woodhouse

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

On Sun, 2008-07-06 at 10:29 -0400, Devin Heitmueller wrote:
> Correct me if I'm wrong, but doesn't this also affect those
> distributions that consider kernels with binary firmware blobs to not
> be free software? Those distributions take the stance that the
> firmware must be loadable by userland, in which case the proposed
> patch removes this capability.

No, it doesn't remove that capability.

We're just observing that the trick which fdump.c uses to turn firmware
into a hex array in C source is obsoleted by the generic ability to
include firmware blobs into the kernel using CONFIG_EXTRA_FIRMWARE. If
we just use the generic method, then the conditional code in the driver
can go away, as can the fdump tool.

Yes, that does mean that it's either in the _kernel_ or in userspace,
rather than being in the .ko file -- but if you want modularity and are
already depending on having a functional userspace, it doesn't seem to
make a lot of sense to have this reimplementation of generic
functionality, just so that you can keep the two parts in _one_ file.

--
dwmw2

2008-07-07 02:53:15

by Oliver Endriss

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

David Woodhouse wrote:
> On Sun, 2008-07-06 at 11:09 +0200, Klaus Schmidinger wrote:
> > On 07/06/08 03:15, Oliver Endriss wrote:
> > > Jaswinder Singh wrote:
> > >> There's no point in this, since the user can use the BUILTIN_FIRMWARE
> > >> option to include arbitrary firmware files directly in the kernel image.
> > >
> > > NAK! This option allows to compile the firmware into the _driver_,
> > > which is very useful if you want to test various driver/firmware
> > > combinations. Having the firmware in the _kernel_ does not help!
> >
> > I strongly support Oliver's request!
> > Working with various driver versions is much easier with the
> > firmware compiled into the driver!
>
> That's strange; I've found exactly the opposite to be the case.
>
> If I want to test permutations of driver and firmware, as I've done for
> the libertas driver a number of times, I find it _much_ better to
> preserve the modularity. I can build each version of the driver and can
> test that against various firmware versions without having to rebuild
> it, and with much less chance of something going wrong so that I'm not
> actually testing what I think I'm testing.
>
> Perhaps I'm missing something that would help me work better? Please
> could you help me understand how you currently work, and I'll attempt to
> make it easier for you. Can you talk me through an example of a session
> where you had to do this testing of 'various driver/firmware
> combinations'?

Note that the v4ldvb drivers can be configured and compiled outside the
kernel tree. Basically I have several directory trees, for example
- v4l-dvb_head
- v4l-dvb_31dec2007_fw2622
- v4l-dvb_31dec2007_fwf12623
- ...

Within these directories you can simply use "make menuconfig" to
configure the driver. If you compile the firmware into the driver,
you can easily freeze a 'known good' or 'known bad' configuration.

Trying a different driver is as easy as changing directories.
This simplifies testing.

CU
Oliver

--
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
----------------------------------------------------------------

2008-07-07 21:02:50

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

Hi Oliver,

> @Mauro:
> Is there a way to strip this stuff from Kconfig/Makefile/av7110*.[ch]
> for submission to the kernel? Basically I don't care whether and how
> they cripple the driver in the kernel. But I would like to keep the code
> 'as is' in the linuxtv repositories.

Having a binary firmware inside a GPL'd file seems to be (at least for some
people) a GPL violation, since the firmware source code should also be released
with GPL (please read [1] for a good article about this).

This seems to be one of the reasons for moving all firmwares that are still in
kernel into a separate dir. A latter step seems to move those files to a cousin
tree, hosted together with kernel tree, and clearly stating that those
firmwares are not GPL, but are allowed to be used with Linux.

IANAL, but, if the firmware is violating GPL at Kernel, the same violation also
affects our tree. So, from legal POV, I think we ought to do the same thing at
V4L/DVB.

Probably, it is safe to keep the firmware files into a separate dir,
if we state clearly that the firmwares are not GPL.

Anyway, let's see how we can handle it after having those patches merged at
mainstream.

[1] http://lwn.net/Articles/284932/

Cheers,
Mauro

2008-07-07 21:20:36

by Alan

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH] Remove fdump tool for av7110 firmware

> Having a binary firmware inside a GPL'd file seems to be (at least for some
> people) a GPL violation, since the firmware source code should also be released
> with GPL (please read [1] for a good article about this).

The firmware is usually a separate program, running on a separate
processor communicating by a standard API and is used by Linux, Windows
and other OS's. Thats not quite the same as say the Nvidia driver.

If it happens to keep the GPL zealots happier then fine, but there are
good sound technical reasons for doing it

- compiled in firmware isn't pageable
- compiled in firmware can't be updated separately
- it makes it really hard for end users told to "try the new firmware" -
especially in the enterprise space.

Clearly there are some cases that is less relevant - drivers where the
firmware/OS pair must match such as aic7xxx where you don't really want
firmware floating around loose.

Alan