Re: [PATCH v3] Firmware download for Qualcomm Bluetooth devices

[Matt Wilson <mtwilson@codeaurora.org>]

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