Re: [MFD] Patchset *UPDATED*

[ian <spyro@xxxxxxx>]

On Mon, 2008-07-21 at 01:35 +0200, Pierre Ossman wrote:

Hi!

> From: Ian Molton <spyro@xxxxxxx>
>
> > +	depends on MMC && MFD_CORE
> 
> MMC is redundant. The entire things is surrounded by if MMC. (Yeah, I know there
> are some other violators. They'll be taken out back and shot eventually...)

Taken out back and shot.

> > +#include <linux/mmc/mmc.h>
> 
> Generally a buggy behaviour when this is needed. I assume you only need it for
> MMC_STOP_TRANSMISSION, but that should be matched using the same logic the hw
> uses.

what do you mean by 'matched by the same loic the hardware uses' ?

> Using that define implies some higher level awareness (which isn't there).
> The driver should make the exact same check the hardware does to properly
> use this offloading feature. I.e. you check the value of the opcode, _and_
> the value of the parameter. IOW:
> 
> /* The hardware can do this command by itself */
> if ((cmd.opcode == 12) && (cmd.argument == 0))

What do you mean by 'higher level awareness that isnt there'? The driver
knows the hardware processes this command internally. You will never see
a response from the hardware to CMD12 so we need to know this.

Why use '12' rather than the opcode macro? that seems wrong.

> It would probably be better if you probe the hardware and figure out which
> aspects of the response the hardware actually checks, and then do a mapping
> based on that. It should allow you to write a future proof driver.

How would I do that? the hardware isnt going to just turn round and say
'ow!' when I prod it.

Part of the problem is that this driver was reverse engineered from near
no documentation and a couple of windows CE drivers, so there are a
number of things we will never know if the driver requires or are just
oiddities in the winCE drivers and docs.

> > +/* FIXME - this seems to be ok comented out but the spec suggest this bit should
> > + *         be set when issuing app commands.
> > + *	if(cmd->flags & MMC_FLAG_ACMD)
> > + *		c |= APP_CMD;
> > + */
> 
> I've yet to see a reasonable explanation as to what hardware might to
> differently for ACMDs, so just kill off this dead code completely. It's
> just noise.

Who knows? I was hoping to leave that there until I can get round to
implementing SDIO on this chipset, just in case.

> > +/* This chip always returns (at least?) as much data as you ask for.
> > + * Im unsure what happens if you ask for less than a block. This should be
> > + * looked into to ensure that a funny length read doesnt hose the controller.
> > + *
> > + * FIXME - this chip cannot do 1 and 2 byte data requests in 4 bit mode
> > + */
> 
> You should check for such requests and fail them with -EINVAL.

true. Will fix.

> > +
> > +	/* FIXME - return correct transfer count on errors */
> > +	if (!data->error)
> > +		data->bytes_xfered = data->blocks * data->blksz;
> > +	else
> > +		data->bytes_xfered = 0;
> > +
> 
> Are you confident you can figure out the correct bytes_xfered value without
> vendor cooperation? This is not some value you should make guestimates about.

How do you propose I query the hardware?

The comment says everything I know. AFAICT the hardware transfers a
minimum of the transfer size you ask it to.

> > +	/*FIXME - other drivers allow an optional stop command of any given type
> > +	 *        which we dont do, as the chip can auto generate them.
> > +	 *        Perhaps we can be smarter about when to use auto CMD12 and
> > +	 *        only issue the auto request when we know this is the desired
> > +	 *        stop command, allowing fallback to the stop command the
> > +	 *        upper layers expect. For now, we do what works.
> > +	 */
> 
> You need to check this assumption and fail the requests you cannot handle
> with -EINVAL.

I'll see what I can do.

> > +
> > +	status = tmio_ioread32(ctl + CTL_STATUS);
> > +	irq_mask   = tmio_ioread32(ctl + CTL_IRQ_MASK);
> > +	ireg   = status & TMIO_MASK_IRQ & ~irq_mask;
> 
> Some stray whitespace here.

Killed.

> > +
> > +	mmc->ops = &tmio_mmc_ops;
> > +	mmc->caps = MMC_CAP_4_BIT_DATA;
> > +	mmc->f_min = 46875;
> 
> mmc->f_max / 512 is a lot clearer.

Not sure I agree. Not sure the compiler is smart enough to use a
constant in that case either. Dont want to change it. If you'd like
clarity, how about #define MMC_CLK_46K8 or something? For now, its
staying as-is.

> > +static inline int tmio_mmc_next_sg(struct tmio_mmc_host *host)
> > +{
> > +	host->sg_ptr++;
> 
> sg_next()

Fixed. probably didnt exist when this was written.

> Two more things:
> 
> 1. Replace those constants you know with proper defines so that someone
> else has a reasonable chance of hacking the code.

Will see what I can do. ISTR not very many of the definitions are known.

> 2. Run the entire thing through mmc_test. It tortures the driver with a lot
> of corner cases.

I'll see what I can do, although my only machines with this device use
MMC for their root filesystem.


-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php