Re: [PATCH] Add support for Xilinx SystemACE CompactFlash interface.

["Grant Likely" <grant.likely@secretlab.ca>]

On 5/2/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  2 May 2007 00:43:16 -0600 Grant Likely <grant.likely@secretlab.ca> 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.
grant.likely@secretlab.ca
(403) 399-0195
-
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/