Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

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

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