Hi Luiz, > -----Original Message----- > From: linux-bluetooth-owner@xxxxxxxxxxxxxxx [mailto:linux-bluetooth- > owner@xxxxxxxxxxxxxxx] On Behalf Of Luiz Augusto von Dentz > Sent: Friday, September 12, 2014 3:37 PM > To: Vikrampal Yadav > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Dmitry Kasatkin; cpgs@xxxxxxxxxxx > Subject: Re: [PATCH 1/2] Monitor: Modify design of AVRCP callback functions > > Hi Vikram, > > On Thu, Sep 11, 2014 at 3:35 PM, Vikrampal Yadav > <vikram.pal@xxxxxxxxxxx> wrote: > > Modified the design of AVRCP callback functions. > > --- > > monitor/avctp.c | 94 > > +++++++++++++++++++++++++++++++++++++++------------------ > > 1 file changed, 65 insertions(+), 29 deletions(-) > > > > diff --git a/monitor/avctp.c b/monitor/avctp.c index c7e242b..41b6380 > > 100644 > > --- a/monitor/avctp.c > > +++ b/monitor/avctp.c > > @@ -158,6 +158,12 @@ > > #define AVRCP_ATTRIBUTE_SHUFFLE 0x03 > > #define AVRCP_ATTRIBUTE_SCAN 0x04 > > > > +struct avctp_frame { > > + uint8_t hdr; > > + uint8_t pt; > > + struct l2cap_frame *l2cap_frame; }; > > + > > static const char *ctype2str(uint8_t ctype) { > > switch (ctype & 0x0f) { > > @@ -517,15 +523,19 @@ static const char *charset2str(uint16_t charset) > > } > > } > > > > -static bool avrcp_passthrough_packet(struct l2cap_frame *frame) > > +static bool avrcp_passthrough_packet(struct avctp_frame *avctp_frame) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > + > > packet_hexdump(frame->data, frame->size); > > return true; > > } > > > > -static bool avrcp_get_capabilities(struct l2cap_frame *frame, uint8_t > ctype, > > - uint8_t len, uint8_t indent) > > +static bool avrcp_get_capabilities(struct avctp_frame *avctp_frame, > > + uint8_t ctype, uint8_t len, > > + uint8_t indent) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t cap, count; > > int i; > > > > @@ -576,10 +586,11 @@ static bool avrcp_get_capabilities(struct > l2cap_frame *frame, uint8_t ctype, > > return true; > > } > > > > -static bool avrcp_list_player_attributes(struct l2cap_frame *frame, > > +static bool avrcp_list_player_attributes(struct avctp_frame > > +*avctp_frame, > > uint8_t ctype, uint8_t len, > > uint8_t indent) { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t num; > > int i; > > > > @@ -604,9 +615,11 @@ static bool avrcp_list_player_attributes(struct > l2cap_frame *frame, > > return true; > > } > > > > -static bool avrcp_list_player_values(struct l2cap_frame *frame, uint8_t > ctype, > > - uint8_t len, uint8_t indent) > > +static bool avrcp_list_player_values(struct avctp_frame *avctp_frame, > > + uint8_t ctype, uint8_t len, > > + uint8_t indent) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > static uint8_t attr = 0; > > uint8_t num; > > > > @@ -640,10 +653,11 @@ response: > > return true; > > } > > > > -static bool avrcp_get_current_player_value(struct l2cap_frame *frame, > > +static bool avrcp_get_current_player_value(struct avctp_frame > > +*avctp_frame, > > uint8_t ctype, uint8_t len, > > uint8_t indent) { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t num; > > > > if (!l2cap_frame_get_u8(frame, &num)) @@ -688,9 +702,11 @@ > > response: > > return true; > > } > > > > -static bool avrcp_set_player_value(struct l2cap_frame *frame, uint8_t > ctype, > > - uint8_t len, uint8_t indent) > > +static bool avrcp_set_player_value(struct avctp_frame *avctp_frame, > > + uint8_t ctype, uint8_t len, > > + uint8_t indent) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t num; > > > > if (ctype > AVC_CTYPE_GENERAL_INQUIRY) @@ -720,10 +736,11 @@ > > static bool avrcp_set_player_value(struct l2cap_frame *frame, uint8_t > ctype, > > return true; > > } > > > > -static bool avrcp_get_player_attribute_text(struct l2cap_frame > > *frame, > > +static bool avrcp_get_player_attribute_text(struct avctp_frame > > +*avctp_frame, > > uint8_t ctype, uint8_t len, > > uint8_t indent) { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t num; > > > > if (!l2cap_frame_get_u8(frame, &num)) @@ -783,10 +800,11 @@ > > response: > > return true; > > } > > > > -static bool avrcp_get_player_value_text(struct l2cap_frame *frame, > > +static bool avrcp_get_player_value_text(struct avctp_frame > > +*avctp_frame, > > uint8_t ctype, uint8_t len, > > uint8_t indent) { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > static uint8_t attr = 0; > > uint8_t num; > > > > @@ -858,9 +876,11 @@ response: > > return true; > > } > > > > -static bool avrcp_displayable_charset(struct l2cap_frame *frame, uint8_t > ctype, > > - uint8_t len, uint8_t indent) > > +static bool avrcp_displayable_charset(struct avctp_frame *avctp_frame, > > + uint8_t ctype, uint8_t len, > > + uint8_t indent) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t num; > > > > if (ctype > AVC_CTYPE_GENERAL_INQUIRY) @@ -886,8 +906,8 @@ > > static bool avrcp_displayable_charset(struct l2cap_frame *frame, > > uint8_t ctype, > > > > struct avrcp_ctrl_pdu_data { > > uint8_t pduid; > > - bool (*func) (struct l2cap_frame *frame, uint8_t ctype, uint8_t len, > > - uint8_t indent); > > + bool (*func) (struct avctp_frame *avctp_frame, uint8_t ctype, > > + uint8_t len, uint8_t > > + indent); > > }; > > > > static const struct avrcp_ctrl_pdu_data avrcp_ctrl_pdu_table[] = { @@ > > -915,9 +935,10 @@ static bool avrcp_rejected_packet(struct l2cap_frame > *frame, uint8_t indent) > > return true; > > } > > > > -static bool avrcp_pdu_packet(struct l2cap_frame *frame, uint8_t > > ctype, > > +static bool avrcp_pdu_packet(struct avctp_frame *avctp_frame, uint8_t > > +ctype, > > > > uint8_t indent) { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > uint8_t pduid, pt; > > uint16_t len; > > int i; > > @@ -929,6 +950,9 @@ static bool avrcp_pdu_packet(struct l2cap_frame > *frame, uint8_t ctype, > > if (!l2cap_frame_get_u8(frame, &pt)) > > return false; > > > > + /* For further use */ > > + avctp_frame->pt = pt; > > + > > if (!l2cap_frame_get_be16(frame, &len)) > > return false; > > > > @@ -953,11 +977,13 @@ static bool avrcp_pdu_packet(struct l2cap_frame > *frame, uint8_t ctype, > > return true; > > } > > > > - return ctrl_pdu_data->func(frame, ctype, len, indent + 2); > > + return ctrl_pdu_data->func(avctp_frame, ctype, len, indent + > > + 2); > > } > > > > -static bool avrcp_control_packet(struct l2cap_frame *frame) > > +static bool avrcp_control_packet(struct avctp_frame *avctp_frame) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > + > > uint8_t ctype, address, subunit, opcode, company[3], indent = > > 2; > > > > if (!l2cap_frame_get_u8(frame, &ctype) || @@ -988,7 +1014,7 @@ > > static bool avrcp_control_packet(struct l2cap_frame *frame) > > > > switch (opcode) { > > case 0x7c: > > - return avrcp_passthrough_packet(frame); > > + return avrcp_passthrough_packet(avctp_frame); > > case 0x00: > > if (!l2cap_frame_get_u8(frame, &company[0]) || > > !l2cap_frame_get_u8(frame, > > &company[1]) || @@ -998,29 +1024,32 @@ static bool > avrcp_control_packet(struct l2cap_frame *frame) > > print_field("%*cCompany ID: 0x%02x%02x%02x", indent, ' ', > > company[0], company[1], > > company[2]); > > > > - return avrcp_pdu_packet(frame, ctype, 10); > > + return avrcp_pdu_packet(avctp_frame, ctype, 10); > > default: > > packet_hexdump(frame->data, frame->size); > > return true; > > } > > } > > > > -static bool avrcp_browsing_packet(struct l2cap_frame *frame, uint8_t > > hdr) > > +static bool avrcp_browsing_packet(struct avctp_frame *avctp_frame) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > + > > packet_hexdump(frame->data, frame->size); > > return true; > > } > > > > -static void avrcp_packet(struct l2cap_frame *frame, uint8_t hdr) > > +static void avrcp_packet(struct avctp_frame *avctp_frame) > > { > > + struct l2cap_frame *frame = avctp_frame->l2cap_frame; > > bool ret; > > > > switch (frame->psm) { > > case 0x17: > > - ret = avrcp_control_packet(frame); > > + ret = avrcp_control_packet(avctp_frame); > > break; > > case 0x1B: > > - ret = avrcp_browsing_packet(frame, hdr); > > + ret = avrcp_browsing_packet(avctp_frame); > > break; > > default: > > packet_hexdump(frame->data, frame->size); @@ -1037,18 > > +1066,25 @@ void avctp_packet(const struct l2cap_frame *frame) { > > uint8_t hdr; > > uint16_t pid; > > - struct l2cap_frame avctp_frame; > > + struct l2cap_frame l2cap_frame; > > + struct avctp_frame avctp_frame; > > const char *pdu_color; > > > > - l2cap_frame_pull(&avctp_frame, frame, 0); > > + l2cap_frame_pull(&l2cap_frame, frame, 0); > > I think it better to use only one variable, avctp_frame, and change the struct > to actually have the data inline instead of a pointer: > I couldn't understand as to how having data inline instead of a pointer is better here. Having pointer is sufficient. Isn't it? Moreover, in function avctp_packet(), we are receiving a pointer only. > struct avctp_frame { > uint8_t hdr; > uint8_t pt; > struct l2cap_frame l2cap_frame; /* note that is not a pointer */ } > > > > > - if (!l2cap_frame_get_u8(&avctp_frame, &hdr) || > > - !l2cap_frame_get_be16(&avctp_frame, &pid)) { > > + /* For further use */ > > + avctp_frame.l2cap_frame = &l2cap_frame; > > + > > + if (!l2cap_frame_get_u8(&l2cap_frame, &hdr) || > > + !l2cap_frame_get_be16(&l2cap_frame, &pid)) { > > print_text(COLOR_ERROR, "frame too short"); > > packet_hexdump(frame->data, frame->size); > > return; > > } > > You should be able to use the fields from the struct directly and eliminate the > user of extra variable here. > > > + /* For further use */ > > + avctp_frame.hdr = hdr; > > + > > if (frame->in) > > pdu_color = COLOR_MAGENTA; > > else > > @@ -1061,7 +1097,7 @@ void avctp_packet(const struct l2cap_frame > *frame) > > hdr & 0x0c, hdr >> 4, pid); > > > > if (pid == 0x110e || pid == 0x110c) > > - avrcp_packet(&avctp_frame, hdr); > > + avrcp_packet(&avctp_frame); > > else > > packet_hexdump(frame->data, frame->size); > > pid should probably be part of the avctp_frame as well. > > > } > > -- > > 1.9.1 > > Btw, this patch does not apply when checking with check patch: > > Applying: Monitor: Modify design of AVRCP callback functions > bluez/.git/rebase-apply/patch:132: trailing whitespace. > uint8_t ctype, uint8_t len, > fatal: 1 line adds whitespace errors. > > Please add a hook to check patch, for example what I use is: > > exec git diff --cached | checkpatch.pl -q --no-signoff --ignore > CAMELCASE,NEW_TYPEDEFS,INITIALISED_STATIC,GLOBAL_INITIALISERS,PREF > ER_PACKED,SPACING,FSF_MAILING_ADDRESS,TRAILING_STATEMENTS,RETUR > N_VOID,FILE_PATH_CHANGES > --show-types - > > -- > Luiz Augusto von Dentz > -- > 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 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
