On May 2 2007 00:45, Andrew Morton wrote:
>> +static void ace_identin_8(struct ace_device *ace)
>> +{
>> + void* r = ace->baseaddr + 0x40;
>> + int i = ACE_FIFO_SIZE/2;
>> + while (i--)
>> +#if defined(__BIG_ENDIAN)
>> + *ace->data_ptr++ = (in_8(r)) | (in_8(r+1)<<8);
>> +#else
>> + *ace->data_ptr++ = (in_8(r)<<8) | (in_8(r+1));
>> +#endif
>> +}
>
>This ifdeffery appears in several places. SUggest the addition of a helper
>function which does it in a single place.
*ace->data_ptr++ = le16_to_cpu((in_8(r)<<8) | (in_8(r+1)));
could help.
>> +#define ace_identin(ace) ace->reg_ops->identin(ace)
>> +#define ace_datain(ace) ace->reg_ops->datain(ace)
>> +#define ace_dataout(ace) ace->reg_ops->dataout(ace)
>
>inline functions are preferred. The above is a strange mixture of inlines
>and macros. Can they all be made inlines?
First, a () pair is lacking, it should - for safety - be
(ace)->reg_ops->dataout(ace)
and since it may be evaluated twice too, inlines are a definite yes.
>> +/* FSM tasks; used to direct state transitions */
>> +#define ACE_TASK_IDLE 0
>> +#define ACE_TASK_IDENTIFY 1
>> +#define ACE_TASK_READ 2
>> +#define ACE_TASK_WRITE 3
>> +#define ACE_FSM_NUM_TASKS 4
>> +
>> +/* FSM state definitions */
>> +#define ACE_FSM_STATE_IDLE 0
>> +#define ACE_FSM_STATE_REQ_LOCK 1
>> +#define ACE_FSM_STATE_WAIT_LOCK 2
>> +#define ACE_FSM_STATE_WAIT_CFREADY 3
>> +#define ACE_FSM_STATE_IDENTIFY_PREPARE 4
>> +#define ACE_FSM_STATE_IDENTIFY_TRANSFER 5
>> +#define ACE_FSM_STATE_IDENTIFY_COMPLETE 6
>> +#define ACE_FSM_STATE_REQ_PREPARE 7
>> +#define ACE_FSM_STATE_REQ_TRANSFER 8
>> +#define ACE_FSM_STATE_REQ_COMPLETE 9
>> +#define ACE_FSM_STATE_ERROR 10
>> +#define ACE_FSM_NUM_STATES 11
enums for good!
>> +#if defined(DEBUG)
>> +const char* ace_statenames[ACE_FSM_NUM_STATES] = {
>> + "idle",
>> + "req lock",
>> + "wait lock",
>> + "wait cf ready",
>> + "identify prepare",
>> + "identify transfer",
>> + "identify complete",
>> + "request prepare",
>> + "request transfer",
>> + "request complete",
>> +};
>> +#endif
>
>Can these have static storage class?
And const please :) IOW,
static const char *const ace_statenames
Btw, ACE_FSM_NUM_STATES is defines as 11 above, but I only see 10
elements in that array. Intentional?
Also, should this be unintentional, and the number state names
matches ACE_FSM_NUM_STATES, an array of unspecified size may do
the trick.
>Who owns gendisk.private_data? Is it the device driver? I've never
>looked...
Check out loop.c, it gives some hints who owns that. :)
Jan
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
