On Wed, 2 May 2007 00:43:16 -0600 Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> + * To Do:
> + * - Add FPGA configuration control interface.
> + * - Request major number from lanana
> + * - Add legacy device geometry ioctl
> + */
Swoon. Want a job writing kernel comments?
>
> +static LIST_HEAD(ace_instances);
> +static int ace_major = 0;
Pleas eremove the "= 0;': it takes up space in vmlinux, but .bss is zeroed
anyway.
> + void (*out)(struct ace_device *ace, ulong reg, uint16_t val);
> + void (*identin)(struct ace_device *ace);
> + void (*datain)(struct ace_device *ace);
> + void (*dataout)(struct ace_device *ace);
> +};
> +
> +/* 8 Bit bus width */
> +static uint16_t ace_in_8(struct ace_device *ace, ulong reg)
Here we have the correct `foo *bar';
> +{
> + void* r = ace->baseaddr + reg;
And here we have the kernelly-incorrect `foo* bar;'
Please do s/* / */ everywhere.
> +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.
> +{
> +#ifndef __LITTLE_ENDIAN
> +# ifdef __BIG_ENDIAN
> + /* The ace_reg_read16 macro handles 16 bit reads correctly, but
> + * 32bit values are partially little endian; swap the words
> + */
> + id->lba_capacity = ((id->lba_capacity >> 16) & 0x0000FFFF) |
> + ((id->lba_capacity << 16) & 0xFFFF0000);
> + id->spg = ((id->spg >> 16) & 0x0000FFFF) |
> + ((id->spg << 16) & 0xFFFF0000);
> +# else
> +# error "Please fix <asm/byteorder.h>"
Don't you trust us? :)
> +#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?
> +static int ace_release(struct inode *inode, struct file *filp)
> +{
> + struct ace_device *ace = inode->i_bdev->bd_disk->private_data;
> + unsigned long flags;
> + uint16_t val;
> +
> + ace_dbg(ace, "ace_release() users=%i\n", ace->users-1);
> +
> + spin_lock_irqsave(&ace->lock, flags);
> + ace->users--;
> + if (ace->users == 0) {
> + val = ace_reg_read16(ace, ACE_CTRL);
> + ace_reg_write16(ace, ACE_CTRL, val & ~ACE_CTRL_LOCKREQ);
> + }
> + spin_unlock_irqrestore(&ace->lock, flags);
> + return 0;
> +}
Who owns gendisk.private_data? Is it the device driver? I've never
looked...
> +static int ace_ioctl(struct inode *inode, struct file *filp,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct ace_device *ace = inode->i_bdev->bd_disk->private_data;
> + ace_dbg(ace, "ace_ioctl()\n");
> +
> + return -ENOTTY;
> +}
I suspect you can just leave this unimplemented.
> + /*
> + * Initialize the request queue
> + */
> + ace->queue = blk_init_queue(ace_request, &ace->lock);
> + if (ace->queue == NULL)
> + goto err_blk_initq;
> + blk_queue_hardsect_size(ace->queue, 512);
> +
> + /*
> + * Allocate and initialize GD structure
> + */
> + ace->gd = alloc_disk(ACE_NUM_MINORS);
> + if (!ace->gd)
> + goto err_alloc_disk;
> +
> + ace->gd->major = ace_major;
> + ace->gd->first_minor = ace->id * ACE_NUM_MINORS;
> + ace->gd->fops = &ace_fops;
> + ace->gd->queue = ace->queue;
> + ace->gd->private_data = ace;
> + snprintf(ace->gd->disk_name, 32, "xs%c", ace->id + 'a');
> + device_rename(ace->dev, ace->gd->disk_name);
Now why are we doing a device_rename() here? The only other caller in the
tree is the netdev renaming code. I suspect something is awry here.
Looks nice though.