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.
> +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.
> +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.
> + 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.
> + 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.
> +
> + /* 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.
> +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);
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.
> + FAILIF(nw != (int) sizeof(cmdp) + cmd->plen,
> + "Could not send entire command (sent only %d bytes)!\n",
> + nw);
And here.
> + 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?
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
