Return-Path: Subject: Re: [PATCH v3] Firmware download for Qualcomm Bluetooth devices From: Matt Wilson To: Johan Hedberg Cc: linux-bluetooth@vger.kernel.org, marcel@holtmann.org, rshaffer@codeaurora.org In-Reply-To: <20100820223757.GA23924@jh-x301> References: <4C6BFF05.5040506@codeaurora.org> <1282340251-10676-1-git-send-email-mtwilson@codeaurora.org> <20100820223757.GA23924@jh-x301> Content-Type: text/plain; charset="ISO-8859-1" Date: Mon, 23 Aug 2010 07:36:05 -0700 Message-ID: <1282574165.5377.11.camel@linux-champ-06.qualcomm.com> Mime-Version: 1.0 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Sat, 2010-08-21 at 01:37 +0300, Johan Hedberg wrote: > Hi Matt, > > On Fri, Aug 20, 2010, Matthew Wilson wrote: > > Configures device address from hciattach parameter. > > UART speed limited to 115200. > > Requires separate device specific firmware. > > --- > > Makefile.tools | 3 +- > > tools/hciattach.c | 10 ++ > > tools/hciattach.h | 1 + > > tools/hciattach_qualcomm.c | 279 ++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 292 insertions(+), 1 deletions(-) > > create mode 100644 tools/hciattach_qualcomm.c > > Thanks, the patch applies cleanly now. However, I spotted a couple of > whitespace/coding style issues that would be good to get fixed before > pushing this upstream: > > > +#define FAILIF(x, args...) do { \ > > + if (x) { \ > > + fprintf(stderr, ##args); \ > > + return -1; \ > > + } \ > > +} while(0) > > Before each \ at the end of the line you use a mix of tabs and spaces. > Please just use tabs. > See below for origin of style. > > +typedef struct { > > + uint8_t uart_prefix; > > + hci_event_hdr hci_hdr; > > + evt_cmd_complete cmd_complete; > > + uint8_t status; > > + uint8_t data[16]; > > +} __attribute__((packed)) command_complete_t; > > + > > + > > Why the two consecutive empty lines? Please remove one. No reason. Will remove. > > > +static int read_command_complete(int fd, unsigned short opcode, unsigned char len) { > > This one looks like it goes beyond 80 columns. Please split it. Also, > the coding style is to put the opening brace of a function on its own > line. See below for origin of style. > > + FAILIF(resp.hci_hdr.evt != EVT_CMD_COMPLETE, /* event must be event-complete */ > > + "Error in response: not a cmd-complete event, " > > + "but 0x%02x!\n", resp.hci_hdr.evt); > > Mixed tabs and spaces for indentation. Please just use tabs. > See below for origin of style. > > + FAILIF(resp.hci_hdr.plen < 4, /* plen >= 4 for EVT_CMD_COMPLETE */ > > + "Error in response: plen is not >= 4, but 0x%02x!\n", > > + resp.hci_hdr.plen); > > Same here. > See below for origin of style. > > + > > + /* cmd-complete event: opcode */ > > + FAILIF(resp.cmd_complete.opcode != 0, > > + "Error in response: opcode is 0x%04x, not 0!", > > + resp.cmd_complete.opcode); > > And here. > See below for origin of style. > > +static int qualcomm_load_firmware(int fd, const char *firmware, const char *bdaddr_s) { > > This one goes beyond 80 columns too and the opening brace should be on > its own line. > > > + FAILIF(fw < 0, > > + "Could not open firmware file %s: %s (%d).\n", > > + firmware, strerror(errno), errno); > See below for origin of style. > Mixed tabs and spaces for indentation. > > > + FAILIF(read(fw, data, cmd->plen) != cmd->plen, > > + "Could not read %d bytes of data for command with opcode %04x!\n", > > + cmd->plen, > > + cmd->opcode); > > Same here. > See below for origin of style. > > + FAILIF(nw != (int) sizeof(cmdp) + cmd->plen, > > + "Could not send entire command (sent only %d bytes)!\n", > > + nw); > > And here. > See below for origin of style. > > + if (read_command_complete(fd, > > + cmd->opcode, > > + cmd->plen) < 0) { > > And here. Why do you split it into three lines when it all fits within > 80 columns? > The style is from prior commit a8de99bdd963f0980877e066c7802c4247c1000c but I will fix anyway (just in this file; not in hciattach_tialt.c) > Johan > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Many thanks for the comprehensive review. v4 coming shortly. -Matt