RE: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Vikrampal <vikram.pal@samsung.com>]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, August 22, 2014 1:18 PM
> To: Vikrampal Yadav
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> 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@samsung.com> 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