Messages in this thread | | | Date | Tue, 07 Feb 2012 19:41:08 +0000 | From | Chris Boot <> | Subject | Re: [RFC][PATCH] firewire: Add function to get speed from opaque struct fw_request |
| |
On 07/02/2012 18:15, Stefan Richter wrote: > On Feb 06 Chris Boot wrote: >> [ Would something like the following be acceptable as an addition to the >> FireWire stack? This would be enough for me to get the speed of the >> request for my work on the SBP-2 target code. ] > [...] >> --- a/drivers/firewire/core-transaction.c >> +++ b/drivers/firewire/core-transaction.c >> @@ -820,6 +820,12 @@ void fw_send_response(struct fw_card *card, >> } >> EXPORT_SYMBOL(fw_send_response); >> >> +int fw_request_speed(struct fw_request *request) >> +{ >> + return request->response.speed; >> +} >> +EXPORT_SYMBOL(fw_request_speed); >> + >> static void handle_exclusive_region_request(struct fw_card *card, >> struct fw_packet *p, >> struct fw_request *request, >> diff --git a/include/linux/firewire.h b/include/linux/firewire.h >> index 84ccf8e..eded4e4 100644 >> --- a/include/linux/firewire.h >> +++ b/include/linux/firewire.h >> @@ -340,6 +340,7 @@ int fw_core_add_address_handler(struct fw_address_handler *handler, >> void fw_core_remove_address_handler(struct fw_address_handler *handler); >> void fw_send_response(struct fw_card *card, >> struct fw_request *request, int rcode); >> +int fw_request_speed(struct fw_request *request); >> void fw_send_request(struct fw_card *card, struct fw_transaction *t, >> int tcode, int destination_id, int generation, int speed, >> unsigned long long offset, void *payload, size_t length, > > Whenever you add a new exported function, please add a brief kerneldoc > comment right above its definition. Can be a one-liner in this case. > Apart from this, > > Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de> > > You can queue this via your SBP-2 target patch series (at which I haven't > looked yet), followed right away by a change to your driver which uses the > new symbol. Or even better, fold these two changes into one.
I'll add some kerneldoc to it before I make it part of my series, it might as well be part of the same thing. Thanks for the ack!
> Not sure if four more characters should be spent on the symbol's name > (fw_get_request_speed). If you already thought about it and preferred > fw_request_speed, keep it that way.
I did briefly consider it but I came up with 'fw_request_get_speed()' and didn't like it. fw_get_request_speed() makes sense to me though so I'll make the change before I send it out again.
> There are at least two alternatives to your proposal: > > - We could move the definition of struct fw_request from > drivers/firewire/core-transaction.c to include/linux/firewire.h. > That would make for a rather bad driver API though. > > - We could expand the function type fw_address_callback_t() by a speed > argument. The other users of the API (firewire-core itself, > firewire-sbp2, firewire-net, firedtv, snd-firewire-lib, and other > as yet unmerged firewire sound drivers) and any other perceivable > future user of the API would not care of that speed though. > > I.e. the proposed "get speed of request" helper seems better.
Great!
Thanks, Chris
-- Chris Boot bootc@bootc.net
| |