Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] > Sent: Friday, August 22, 2014 1:18 PM > To: Vikrampal Yadav > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; cpgs@xxxxxxxxxxx > Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP > ListPlayerApplicationSettingValues support > > Hi Vikrampal, > > On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav > <vikram.pal@xxxxxxxxxxx> wrote: > > Support for decoding AVRCP ListPlayerApplicationSettingValues > > added in Bluetooth monitor. > > --- > > monitor/avctp.c | 79 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 79 insertions(+) > > > > diff --git a/monitor/avctp.c b/monitor/avctp.c index ec2adcd..35eca02 > > 100644 > > --- a/monitor/avctp.c > > +++ b/monitor/avctp.c > > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr) > > } > > } > > > > +static const char *value2str(uint8_t attr, uint8_t value) { > > + switch (attr) { > > + case AVRCP_ATTRIBUTE_ILEGAL: > > + return "Illegal"; > > + case AVRCP_ATTRIBUTE_EQUALIZER: > > + switch (value) { > > + case 0x01: > > + return "OFF"; > > + case 0x02: > > + return "ON"; > > + default: > > + return "Reserved"; > > + } > > + case AVRCP_ATTRIBUTE_REPEAT_MODE: > > + switch (value) { > > + case 0x01: > > + return "OFF"; > > + case 0x02: > > + return "Single Track Repeat"; > > + case 0x03: > > + return "All Track Repeat"; > > + case 0x04: > > + return "Group Repeat"; > > + default: > > + return "Reserved"; > > + } > > + case AVRCP_ATTRIBUTE_SHUFFLE: > > + switch (value) { > > + case 0x01: > > + return "OFF"; > > + case 0x02: > > + return "All Track Suffle"; > > + case 0x03: > > + return "Group Suffle"; > > + default: > > + return "Reserved"; > > + } > > + case AVRCP_ATTRIBUTE_SCAN: > > + switch (value) { > > + case 0x01: > > + return "OFF"; > > + case 0x02: > > + return "All Track Scan"; > > + case 0x03: > > + return "Group Scan"; > > + default: > > + return "Reserved"; > > + } > > + default: > > + return "Unknown"; > > + } > > +} > > + > > static void avrcp_passthrough_packet(const struct l2cap_frame *frame) > > { } @@ -504,6 +558,31 @@ static void avrcp_list_player_values(const > > struct l2cap_frame *frame, > > uint8_t ctype, uint8_t len, > > uint8_t indent) { > > + static uint8_t attr = 0; > > + uint8_t num, i; > > + > > + if (len < 1) { > > + print_text(COLOR_ERROR, "PDU malformed"); > > + packet_hexdump(frame->data, frame->size); > > + return; > > + } > > + > > + if (ctype > AVC_CTYPE_GENERAL_INQUIRY) { > > + num = *((uint8_t *) frame->data); > > + print_field("%*cValueCount: 0x%02x", (indent - 8), ' > > + ', num); > > + > > + for (i = 0; num > 0; num--, i++) { > > + uint8_t value; > > + > > + value = *((uint8_t *) (frame->data + 1 + i)); > > + print_field("%*cValueID: 0x%02x (%s)", (indent - 8), > > + ' ', value, value2str(attr, value)); > > + } > > + } else { > > + attr = *((uint8_t *) frame->data); > > + print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ', > > + attr, attr2str(attr)); > > + } > > } > > What I suggested regarding the goto is valid for all the patches, also you > seem to disregard my comment about advancing in the frame instead of an > offset for the very begging of the frame. > > -- > Luiz Augusto von Dentz I thought you suggested to use goto for deep indentations. So, wherever the nesting is manageable, I chose to let it be as it is. Otherwise at some places where indentations goes deep, I have made use of goto as suggested by you. And for 2nd suggestion, I believe in any loop the loop index is an important variable and we should use it wherever obvious pattern is encountered in a loop. If understandability is a concern then I can put proper comments explaining the intention. Regards, Vikram -- 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
