On Fri, Apr 16, 2010 at 09:50:42AM +0400, Yury G. Kudryashov wrote: > Hi! > > Review the attached patches, please. > From 5cc42c6f87f690fdf66e29de2a816dab49a119f4 Mon Sep 17 00:00:00 2001 > From: Yury G. Kudryashov <urkud.urkud@xxxxxxxxx> > Date: Fri, 16 Apr 2010 00:21:02 +0400 > Subject: [PATCH 1/2] Include linux/types.h 1. Prefix the subject with "hid2hci: " so it's clear from the shortlog what this affects. 2. Please include some rational in the commit message. What prototype or datatype is missing that requires including <linux/types.h>? > > --- > extras/hid2hci/hid2hci.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/extras/hid2hci/hid2hci.c b/extras/hid2hci/hid2hci.c > index 0d0a022..839c4fb 100644 > --- a/extras/hid2hci/hid2hci.c > +++ b/extras/hid2hci/hid2hci.c > @@ -28,6 +28,7 @@ > #include <string.h> > #include <getopt.h> > #include <sys/ioctl.h> > +#include <linux/types.h> > #include <linux/hiddev.h> > #include <usb.h> > > -- > 1.7.0.5 > > > From f9881e554deafad1301d00e4539a02fe63078c00 Mon Sep 17 00:00:00 2001 > From: Yury G. Kudryashov <urkud.urkud@xxxxxxxxx> > Date: Fri, 16 Apr 2010 09:38:32 +0400 > Subject: [PATCH 2/2] Add --with-firmware-path configure option Again, please include some rational in the commit message. What problem is this solving? > > --- > Makefile.am | 3 ++- > configure.ac | 22 ++++++++++++++++++++++ > extras/firmware/firmware.c | 10 ++++++---- > 3 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/Makefile.am b/Makefile.am > index 68a68d9..f094746 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -156,7 +156,7 @@ EXTRA_DIST += \ > udev/udevd.xml > > %.7 %.8 : %.xml > - $(XSLTPROC) -o $@ -nonet http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl $< > + $(XSLTPROC) -o $@ http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl $< This is an unrelated change. Please remove it from this patch. > > # ------------------------------------------------------------------------------ > # udev tests > @@ -194,6 +194,7 @@ dist_udevrules_DATA += \ > # ------------------------------------------------------------------------------ > extras_firmware_firmware_SOURCES = extras/firmware/firmware.c > extras_firmware_firmware_LDADD = libudev/libudev-private.la > +extras_firmware_firmware_CPPFLAGS = $(AM_CPPFLAGS) -DFIRMWARE_PATH="$(FIRMWARE_PATH)" > dist_udevrules_DATA += extras/firmware/50-firmware.rules > libexec_PROGRAMS = extras/firmware/firmware > > diff --git a/configure.ac b/configure.ac > index 492fa02..5defd5c 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -82,6 +82,27 @@ if test "x$enable_extras" = xyes; then > fi]) > AC_SUBST(PCI_DATABASE) > > + AC_ARG_WITH(firmware-path, > + AS_HELP_STRING([--with-firmware-path=DIR[[[:DIR[...]]]]], > + [Firmware search path (default=/lib/firmware/updates:/lib/firmware)]), > + [], > + [with_firmware_path="/lib/fimware/updates:/lib/fimware"] > + ) > + [ > + OLD_IFS=$IFS > + IFS=: > + for i in $with_firmware_path > + do > + if [ "x${FIRMWARE_PATH}" == "x" ]; then > + FIRMWARE_PATH="\\\"${i}/\\\"" > + else > + FIRMWARE_PATH="${FIRMWARE_PATH}, \\\"${i}/\\\"" > + fi > + done > + IFS=$OLD_IFS > + ] I'm not sure what wrapping this whole section in [] quoting does. It looks like a good handling of the option, though. > + AC_SUBST([FIRMWARE_PATH], [$FIRMWARE_PATH]) > + > AC_CHECK_HEADER([linux/input.h], [:], AC_MSG_ERROR([kernel headers not found])) > AC_SUBST([INCLUDE_PREFIX], [$(echo '#include <linux/input.h>' | eval $ac_cpp -E - | sed -n '/linux\/input.h/ {s:.*"\(.*\)/linux/input.h".*:\1:; p; q}')]) > fi > @@ -144,6 +165,7 @@ AC_MSG_RESULT([ > > usb.ids: ${USB_DATABASE} > pci.ids: ${PCI_DATABASE} > + firmware path: ${FIRMWARE_PATH} > > xsltproc: ${XSLTPROC} > gperf: ${GPERF} > diff --git a/extras/firmware/firmware.c b/extras/firmware/firmware.c > index 92f0918..76071db 100644 > --- a/extras/firmware/firmware.c > +++ b/extras/firmware/firmware.c > @@ -79,10 +79,7 @@ int main(int argc, char **argv) > { "help", no_argument, NULL, 'h' }, > {} > }; > - static const char *searchpath[] = { > - "/lib/firmware/updates/", > - "/lib/firmware/" > - }; > + static const char *searchpath[] = { FIRMWARE_PATH }; > char fwencpath[UTIL_PATH_SIZE]; > char misspath[UTIL_PATH_SIZE]; > char loadpath[UTIL_PATH_SIZE]; > @@ -97,6 +94,11 @@ int main(int argc, char **argv) > unsigned int i; > int rc = 0; > > + for (i = 0; i < ARRAY_SIZE(searchpath); i++) { > + printf("Path %s\n", searchpath[i]); > + } > + return 0; > + I'm pretty sure the program stops doing anything useful with this hunk. :) > udev_log_init("firmware"); > > for (;;) { > -- > 1.7.0.5 Besides those small issues, both patches seem pretty good to me. -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-hotplug" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
