lkml.org 
[lkml]   [2011]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [Device-drivers-devel] [PATCH] sigma-firmware: loader for Analog Devices' SigmaStudio
On Wed, Jan 19, 2011 at 19:37, Andrew Morton wrote:
> On Wed, 12 Jan 2011 00:41:31 -0500 Mike Frysinger wrote:
> The newly-added code has no callsites.  What are we to make of this?

the ASoC drivers have already been submitted and are going through the process

>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index e8b6a13..8f4f7b2 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -134,4 +134,16 @@ config ISCSI_IBFT
>>         detect iSCSI boot parameters dynamically during system boot, say Y.
>>         Otherwise, say N.
>>
>> +config SIGMA
>> +     tristate "SigmaStudio firmware loader"
>> +     depends on I2C
>
> The dependence on just I2C looks odd.  What's the rationale here?

the code clearly calls i2c_master_send

>> +int process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw)
>
> Global scope appears to be unnecessary.

idea originally was to let people process things more directly, but
there hasnt been a need so far. so localizing them until someone
needs them is probably fine.

>> +     pr_debug("%s: crc=%x\n", __func__, crc32(0, fw->data, fw->size));
>
> The code calculates and prints the crc but doesn't actually check it
> against anything.  What's up with that?

it's supposed to abort. guess we forgot to.

>> +static inline u32 sigma_action_len(struct sigma_action *sa)
>> +{
>> +     return (sa->len_hi << 16) | sa->len;
>> +}
>> +
>> +static inline size_t sigma_action_size(struct sigma_action *sa, u32 payload_len)
>> +{
>> +     return sizeof(*sa) + payload_len + (payload_len % 2);
>> +}
>
> Did these two functions need to be in a header?  If they're only ever
> used in sigma.c then they should be located in that file.

part of the original idea of allowing the callers to process things
more directly if necessary. should be harmless to leave it here.
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-01-20 03:39    [W:0.026 / U:0.884 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site