Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] > Sent: Friday, August 22, 2014 3:32 PM > To: Vikrampal > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; cpgs@xxxxxxxxxxx > Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP > ListPlayerApplicationSettingValues support > > Hi Vikrampal, > > On Fri, Aug 22, 2014 at 11:27 AM, Vikrampal <vikram.pal@xxxxxxxxxxx> > wrote: > > 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. > > The suggestion was to avoid nesting as much as possible and once you do for > one function it probably makes sense to do for all functions to be consistent. > > > 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. > > Well this is not really relevant, what Im suggesting is to move ahead in the > frame using l2cap_frame_pull then the index can be used just to limit the > amount of times you gonna pull, also note that usually you should check if > there is still data remaining in the frame otherwise this code can access > invalid data if the counter is invalid. > > -- > Luiz Augusto von Dentz I'll do the changes accordingly and submit it again. 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
