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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Many thanks for the comprehensive review. v4 coming shortly.
-Matt
--
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
