On 5/2/07, Andrew Morton <[email protected]> wrote:
On Wed, 2 May 2007 00:43:16 -0600 Grant Likely <[email protected]> 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?
Heh; I don't know, how much does that pain pay? :-)
>
> +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.
Oops; yes. I'm waiting on a major number assignment from lanana.
It's supposed to go here.
> + 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.
/me runs entire driver through scripts/Lindent for good measure.
> +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.
It's actually two different things that look very similar, and there
are only 4 tests for __BIG_ENDIAN in the whole driver, and they're all
different. But the point is moot; I messed up the 8 bit accessors.
It will be reimplemented in the next patch.
> +{
> +#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? :)
That's what happens when I copy a snippit from another parts of the kernel. :-P
> +#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?
Yeah, but I'll just remove them instead
> +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...
gd.private_data points to the struct ace_device; which is freed in ace_remove().
> +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.
But keeping it in reminds me that I need to get it done. :-) It will
be implemented in the next patch.
> + /*
> + * 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.
Mostly so that the device shows up as '/sys/block/xsa' instead of
'/sys/block/xsysace.0'. Plus it shows 'xsa' in the log when using
dev_dbg() macros.
I can remove this if this is a bad idea.
Looks nice though.
Thanks for the comments; I'll clean up and resubmit.
Cheers,
g.
--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
