Re: [PATCH Take 3] MTD driver for alauda chips

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 11 July 2007, Jörn Engel wrote:
> A driver for the Olympus MAUSB-10 and Fujifilm DPC-R1 card readers.
> Unlike most stuff on the market the chip inside these two allows raw
> flash access and doesn't implement and FTL, leaving that functionality
> to the device driver.
>
> Raw flash access in a cheap USB cardreader!  An MTD test device one can
> attach to a PC!  What a deal!

Cool stuff! just a few details I noticed:

> +/* Address shifting */
> +#define PBA_LO(pba) ((pba & 0xF) << 5)
> +#define PBA_HI(pba) (pba >> 3)
> +#define PBA_ZONE(pba) (pba >> 11)

What is PBA?

> +static struct usb_driver alauda_driver;

The forward declaration is use nowhere, you can kill it.

> +	struct alauda *al = mtd->priv;
> +	u32 pba = from >> al->card->blockshift;
> +	u32 page = (from >> al->card->pageshift) & al->pagemask;
> +	u8 command[] = {
> +		ALAUDA_BULK_CMD, ALAUDA_BULK_READ_PAGE, PBA_HI(pba),
> +		PBA_ZONE(pba), 0, PBA_LO(pba) + page, 1, 0,
> +		ALAUDA_PORT_XD
> +	};

It would be nice to have support for both ports, not hardcoding xD.
At least I only have SmartMedia cards, so I can't test the code
the way it is ;-)

> +	err = usb_bulk_msg(al->dev, al->bulk_out, command, 9, NULL, HZ);
> +	if (err < 0)
> +		return -EIO;
> +
> +	err = usb_bulk_msg(al->dev, al->bulk_in, buf, mtd->writesize, NULL, HZ);
> +	if (err < 0)
> +		return -EIO;
> +
> +	err = usb_bulk_msg(al->dev, al->bulk_in, oob, 16, NULL, HZ);
> +	if (err < 0)
> +		return -EIO;
> +	return 0;
> +}

Not sure how much difference it makes, but it should be more efficient
to queue all three commands first, and then wait for their completion
once, instead of doing them all synchronously.

> +	err = usb_bulk_msg(al->dev, al->bulk_out, command, 9, NULL, HZ);
> +	if (err < 0)
> +		return -EIO;
> +
> +	err = usb_bulk_msg(al->dev, al->write_out, buf, mtd->writesize, NULL,
> +			HZ);
> +	if (err < 0)
> +		return -EIO;
> +
> +	err = usb_bulk_msg(al->dev, al->write_out, oob, 16, NULL, HZ);
> +	if (err < 0)
> +		return -EIO;
> +	return 0;
> +}

Same here, obviously.

> +static int alauda_read(struct mtd_info *mtd, loff_t from, size_t len,
> +		size_t *retlen, u_char *buf)
> +{
> +	struct alauda *al = mtd->priv;
> +	int err, corrected=0, uncorrected=0;
> +
> +	if ((from & al->bytemask) || (len & al->bytemask))
> +		return -EINVAL;
> +
> +	*retlen = len;
> +	while (len) {
> +		u8 oob[16];
> +
> +		err = alauda_read_page(mtd, from, buf, oob);
> +		if (err)
> +			return err;
> +

and you could gain more if you have multiple pages outstanding I/O.

> +	while (len) {
> +		u32 page = (to >> al->card->pageshift) & al->pagemask;
> +		u8 oob[16] = {	'h', 'e', 'l', 'l', 'o', 0xff, 0xff, 0xff,
> +				0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};

Are you planning to keep this string?

	Armd <><