lkml.org 
[lkml]   [2012]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2012-02-07 20:43    [W:2.480 / U:0.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site