Messages in this thread | | | Subject | Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option | From | David Woodhouse <> | Date | Fri, 23 May 2008 18:13:57 +0100 |
| |
On Fri, 2008-05-23 at 18:41 +0200, Sam Ravnborg wrote: > > +FIRMWARE_BINS := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE)) > > +FIRMWARE_OBJS := $(patsubst %,%.o, $(FIRMWARE_BINS)) > > +FIRMWARE_SRCS := $(patsubst %,$(obj)/%.c, $(FIRMWARE_BINS)) > > My personal rule-of-thumb is to use lower case for > all local variables and UPPER case for global variables. > > I know that outside the kernel everyone use UPPER case > for Makefile variables but that just not readable. > So in this code snippet I would have used lower case.
OK, I'll do that.
> > + > > + > > +quiet_cmd_fwbin = MK_FW $@ > > + cmd_fwbin = echo '/* File automatically generated */' > $@ ; \ > > + echo '\#include <linux/firmware.h>' >> $@ ; \ > > + echo 'static const unsigned char fw[] = {' >> $@ ; \ > > + od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \ > > + sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \ > > + echo '};' >> $@ ; \ > > + echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@
> A small comment is justified here. > Do not consider everyone to know od.
Isn't the context enough? Some comments are just superfluous :)
> If you choose a deciaml output you do not need to add 0x
Hm, that's true -- force of habit, I suppose. But it doesn't really matter, and if anyone _does_ have cause to look at the generated file, it's probably nicer in hex.
Actually, it would be nicer to do it in assembly and use .incbin -- but then we'd have to deal with potential struct alignment issues for the 'struct builtin_fw'. Again, that's an improvement for later. But it's why I used 'unsigned long' for the size, instead of 'size_t'.
> > + > > +$(FIRMWARE_SRCS): $(obj)/%.c: $(srctree)/$(obj)/% > > + $(call cmd,fwbin) > > + > > +obj-y := $(FIRMWARE_OBJS) > > Assignment to targets i smiisng so generated files are not cleaned > by "make clean". > > targets := $(FIRMWARE_OBJS)
OK, thanks.
-- dwmw2
| |