Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Mikael Pettersson]

On Fri, 28 Jul 2006 06:02:11 -0700 (PDT), Alex Dubov wrote:
>The exact condition is (irq_status!=0 &&
>irq_status!=0xffffffff). I think it is not any better
>that what I have.
>
>--- Andrey Panin <pazke@donpac.ru> wrote:
>
>> On 208, 07 27, 2006 at 08:34:06PM -0700, Alex Dubov
>> wrote:
>> 
>> What this strange line (in tifm_7xx1_isr function)
>> is supposed to do:
>> 
>>         if(irq_status && (~irq_status))

If you're chasing micro-optimisations, you could write

    /* if irq_status is not 0 or ~0, do <blah> */
    if (((unsigned)irq_status + 1) >= 2)

which should reduce the number of conditional branches
to a single one. (And drop the cast if irq_status is
declared as unsigned.)

But for long-term maintenance just spelling out the exact
condition (irq_status != 0 && irq_status != ~0) is preferable.

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

No optimization. I just thought both 0xffffffff and -1
are ugly.

--- Mikael Pettersson <mikpe@it.uu.se> wrote:

> On Fri, 28 Jul 2006 06:02:11 -0700 (PDT), Alex Dubov
> wrote:
> >The exact condition is (irq_status!=0 &&
> >irq_status!=0xffffffff). I think it is not any
> better
> >that what I have.
> >
> >--- Andrey Panin <pazke@donpac.ru> wrote:
> >
> >> On 208, 07 27, 2006 at 08:34:06PM -0700, Alex
> Dubov
> >> wrote:
> >> 
> >> What this strange line (in tifm_7xx1_isr
> function)
> >> is supposed to do:
> >> 
> >>         if(irq_status && (~irq_status))
> 
> If you're chasing micro-optimisations, you could
> write
> 
>     /* if irq_status is not 0 or ~0, do <blah> */
>     if (((unsigned)irq_status + 1) >= 2)
> 
> which should reduce the number of conditional
> branches
> to a single one. (And drop the cast if irq_status is
> declared as unsigned.)
> 
> But for long-term maintenance just spelling out the
> exact
> condition (irq_status != 0 && irq_status != ~0) is
> preferable.
> 


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
> I was looking at the way to put my driver into the kernel and currently have three ways of doing
> it (all of them came up in the thread, already):
> 1.
> Put everything in drivers/misc
>
> 2.
> Put tifm_core, tifm_7xx1 and tifm_ms (in progress) in drivers/misc, tifm_sd in drivers/mmc
>
> 3.
> Put everything in drivers/mmc
>
> I'm favoring everything in drivers/mmc, especially if it can be renamed into drivers/flashcards or
> something. This way, all flash card drivers will be nicely localized. In this respect, I also
> wonder where the MemoryStick driver for Winbond card readers is supposed to go when it enters the
> kernel? (Winbond driver is written by people with access to the MemoryStick spec and I'm using it
> as reference for my own work, with great utility).
>
>   

I prefer 2, where only tifm_sd (and tifm_ms) show up in Kconfig. The
other modules will be "Select":ed via Kconfig.

1 can be a bit confusing for users as they expect a MMC/SD driver to be
in drivers/mmc, but it could be acceptable if it's considered more
important to keep all files in a common place.

3 is out as drivers/mmc isn't for "any flashcard technology". It is
intended to grow with SDIO drivers, so we do not want to clutter it up
with other systems.

Until your (and Winbond's) driver has a generic MemoryStick layer, then
the fact that you use MemoryStick is just an implementation detail. From
the kernel's point of view you're just some odd-ball block driver. When
we have a generic MS layer, we should probably move these to a "ms"
directory, possibly creating a common tree structure for mmc and ms.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

I was looking at the way to put my driver into the kernel and currently have three ways of doing
it (all of them came up in the thread, already):
1.
Put everything in drivers/misc

2.
Put tifm_core, tifm_7xx1 and tifm_ms (in progress) in drivers/misc, tifm_sd in drivers/mmc

3.
Put everything in drivers/mmc

I'm favoring everything in drivers/mmc, especially if it can be renamed into drivers/flashcards or
something. This way, all flash card drivers will be nicely localized. In this respect, I also
wonder where the MemoryStick driver for Winbond card readers is supposed to go when it enters the
kernel? (Winbond driver is written by people with access to the MemoryStick spec and I'm using it
as reference for my own work, with great utility).


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
> I wonder, was there any progress with the inclusion of tifm driver into kernel? Are there any
> additional issues to examine with respect to this?
>
>   

Not from my perspective. Follow the discussion with Greg in this thread
and put together a patch against the current git. Make sure you follow
the format of kernel patches and things will go smoothly. :)

Note that Russell still has the final say when it comes to MMC though.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

I wonder, was there any progress with the inclusion of tifm driver into kernel? Are there any
additional issues to examine with respect to this?


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

As a temporal work-around to the timeout problem I've put the following in:
1. All data timeout values are multiplied by a fudge factor of 10 (this is still lower latency
than waiting for a software fall-back).
2. I've added a module option to disable hardware data timeout at all. This is how TI does it too
- command timeout is set to 64 clocks are data timeouts (if any) are captured by the slow software
handler. Card removal is signalled by its own interrupt, so the wait for data in this case will be
aborted anyway.
I haven't checked out your patch yet.

For a written blocks I'm now reporting the BUSY de-assert count rather than block counter value. I
hope this is a good idea.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Greg KH wrote:
> But there is no "subsystem" for a memory card reader, right?  That's one
> of the problems here :)
>   

There is for the MMC portion. And MemoryStick will probably get one next.

> I don't know, but misc/ is fine with me unless someone else has a good
> idea of where to put it.
>   

I was more interested in if the Kconfig select scheme seemed reasonable.
If so, then Alex can start transforming his code into a patch against
the current tree.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Greg KH]

On Tue, Sep 05, 2006 at 10:08:46PM +0200, Pierre Ossman wrote:
> Greg KH wrote:
> > On Sun, Sep 03, 2006 at 11:53:35AM +0200, Pierre Ossman wrote:
> >   
> >> Is there no driver in the kernel that already has this design?
> >>     
> >
> > Not directly, no.  USB-storage handles a wide range of devices like this
> > by virtue of them following the usb storage spec (which is really just
> > scsi).
> >   
> 
> How about this... We put the main driver in drivers/misc, add a Kconfig
> for it that isn't visible, put the submodules in their respective
> subsystems and set their Kconfigs to select the main module. Does that
> sound like a good solution?

But there is no "subsystem" for a memory card reader, right?  That's one
of the problems here :)

I don't know, but misc/ is fine with me unless someone else has a good
idea of where to put it.

thanks,

greg k-h

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Greg KH wrote:
> On Sun, Sep 03, 2006 at 11:53:35AM +0200, Pierre Ossman wrote:
>   
>> Is there no driver in the kernel that already has this design?
>>     
>
> Not directly, no.  USB-storage handles a wide range of devices like this
> by virtue of them following the usb storage spec (which is really just
> scsi).
>   

How about this... We put the main driver in drivers/misc, add a Kconfig
for it that isn't visible, put the submodules in their respective
subsystems and set their Kconfigs to select the main module. Does that
sound like a good solution?

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Greg KH]

On Sun, Sep 03, 2006 at 11:53:35AM +0200, Pierre Ossman wrote:
> Greg KH wrote:
> > On Sat, Sep 02, 2006 at 10:50:14PM +0200, Pierre Ossman wrote:
> >   
> >>
> >> This is a PCI device yes. Which has a number of card readers as
> >> separate, hot-pluggable functions. Currently this means it interacts
> >> with the block device and MMC subsystems of the kernel. As more drivers
> >> pop up, the other card formats will probably get their own subsystems
> >> the way MMC has. So there are three issues here:
> >>
> >>  * Where to put the central module that handles the generic parts of the
> >> chip and pulls in the other modules as needed.
> >>     
> >
> > Right now, the drivers/mmc directory has such a driver, the sdhci.c
> > file, right?
> >
> >   
> 
> Not quite. sdhci is a vendor-neutral MMC controller driver. What I'm
> talking about here is the interface-neutral portion for the Texas
> Instruments multi-format card reader.
> 
> >>  * If the subfunction modules should be put with the subsystems they
> >> connect to or with the main, generic module.
> >>     
> >
> > It all depends on how bit it grows over time.  It is always easy to move
> > files around at a later time if you so wish.
> >
> > For now, is the drivers/mmc/ directory acceptable?  If other card
> > formats show up, we can reconsider it at that time.  Is that ok?
> >   
> 
> Support for MemoryStick isn't that far off in the future, so it would be
> preferable to get this right from the start.
> 
> Is there no driver in the kernel that already has this design?

Not directly, no.  USB-storage handles a wide range of devices like this
by virtue of them following the usb storage spec (which is really just
scsi).

thanks,

greg k-h

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

Alex Dubov wrote:
> I also got a bug report from one of my users. Formerly, I used fixed timeout for data transfer,
> but now I'm setting a timeout from data->timeout_clks. This causes recurrent timeouts with some
> cards. Have you encountered this problem before? If yes, what will be the preferred solution?
>
>   

Yup. The timeout calculation we currently have is broken with regard to
SD cards. Only some cards are affected though. Patch included.

Rgds
Pierre


[-- Attachment #2: sd-timeout.patch --]
[-- Type: text/x-patch, Size: 3566 bytes --]

[MMC] Fix SD timeout calculation

Secure Digital cards use a different algorithm to calculate the timeout
for data transfers. Using the MMC one works often, but not always.

Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>
---

 drivers/mmc/mmc.c       |   15 +++++++++++++--
 drivers/mmc/mmc_block.c |   44 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 33525bd..c0c7ef2 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -912,6 +912,7 @@ static void mmc_read_scrs(struct mmc_hos
 	struct mmc_request mrq;
 	struct mmc_command cmd;
 	struct mmc_data data;
+	unsigned int timeout_us;
 
 	struct scatterlist sg;
 
@@ -947,8 +948,18 @@ static void mmc_read_scrs(struct mmc_hos
 
 		memset(&data, 0, sizeof(struct mmc_data));
 
-		data.timeout_ns = card->csd.tacc_ns * 10;
-		data.timeout_clks = card->csd.tacc_clks * 10;
+		data.timeout_ns = card->csd.tacc_ns * 100;
+		data.timeout_clks = card->csd.tacc_clks * 100;
+
+		timeout_us = data.timeout_ns / 1000;
+		timeout_us += data.timeout_clks * 1000 /
+			(host->ios.clock / 1000);
+
+		if (timeout_us > 100000) {
+			data.timeout_ns = 100000000;
+			data.timeout_clks = 0;
+		}
+
 		data.blksz_bits = 3;
 		data.blksz = 1 << 3;
 		data.blocks = 1;
diff --git a/drivers/mmc/mmc_block.c b/drivers/mmc/mmc_block.c
index e033424..cb7fc8d 100644
--- a/drivers/mmc/mmc_block.c
+++ b/drivers/mmc/mmc_block.c
@@ -30,6 +30,7 @@ #include <linux/blkdev.h>
 #include <linux/mutex.h>
 
 #include <linux/mmc/card.h>
+#include <linux/mmc/host.h>
 #include <linux/mmc/protocol.h>
 
 #include <asm/system.h>
@@ -171,8 +172,6 @@ static int mmc_blk_issue_rq(struct mmc_q
 
 		brq.cmd.arg = req->sector << 9;
 		brq.cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
-		brq.data.timeout_ns = card->csd.tacc_ns * 10;
-		brq.data.timeout_clks = card->csd.tacc_clks * 10;
 		brq.data.blksz_bits = md->block_bits;
 		brq.data.blksz = 1 << md->block_bits;
 		brq.data.blocks = req->nr_sectors >> (md->block_bits - 9);
@@ -180,6 +179,41 @@ static int mmc_blk_issue_rq(struct mmc_q
 		brq.stop.arg = 0;
 		brq.stop.flags = MMC_RSP_R1B | MMC_CMD_AC;
 
+		brq.data.timeout_ns = card->csd.tacc_ns * 10;
+		brq.data.timeout_clks = card->csd.tacc_clks * 10;
+
+		/*
+		 * Scale up the timeout by the r2w factor
+		 */
+		if (rq_data_dir(req) == WRITE) {
+			brq.data.timeout_ns <<= card->csd.r2w_factor;
+			brq.data.timeout_clks <<= card->csd.r2w_factor;
+		}
+
+		/*
+		 * SD cards use a 100 multiplier and has a upper limit
+		 */
+		if (mmc_card_sd(card)) {
+			unsigned int limit_us, timeout_us;
+
+			brq.data.timeout_ns *= 10;
+			brq.data.timeout_clks *= 10;
+
+			if (rq_data_dir(req) == READ)
+				limit_us = 100000;
+			else
+				limit_us = 250000;
+
+			timeout_us = brq.data.timeout_ns / 1000;
+			timeout_us += brq.data.timeout_clks * 1000 /
+				(card->host->ios.clock / 1000);
+
+			if (timeout_us > limit_us) {
+				brq.data.timeout_ns = limit_us * 1000;
+				brq.data.timeout_clks = 0;
+			}
+		}
+
 		if (rq_data_dir(req) == READ) {
 			brq.cmd.opcode = brq.data.blocks > 1 ? MMC_READ_MULTIPLE_BLOCK : MMC_READ_SINGLE_BLOCK;
 			brq.data.flags |= MMC_DATA_READ;
@@ -187,12 +221,6 @@ static int mmc_blk_issue_rq(struct mmc_q
 			brq.cmd.opcode = MMC_WRITE_BLOCK;
 			brq.data.flags |= MMC_DATA_WRITE;
 			brq.data.blocks = 1;
-
-			/*
-			 * Scale up the timeout by the r2w factor
-			 */
-			brq.data.timeout_ns <<= card->csd.r2w_factor;
-			brq.data.timeout_clks <<= card->csd.r2w_factor;
 		}
 
 		if (brq.data.blocks > 1) {

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

--- Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> > 2. for write operation, error-less BUSY assert/de-assert pairs shall be counted instead
> > Currently I only look at the last BUSY de-assert to verify that command is completed
> successfully.
> > As mmc_block always issues single block writes it is sufficient. If this will ever change,
> more
> > sophisticated scheme can be devised.
> >
> >   
> 
> This is about to change. Hence the need to check that all drivers are
> reporting what they're supposed to.
> 
I'll put in the code for this today.

I also got a bug report from one of my users. Formerly, I used fixed timeout for data transfer,
but now I'm setting a timeout from data->timeout_clks. This causes recurrent timeouts with some
cards. Have you encountered this problem before? If yes, what will be the preferred solution?

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
> --- Pierre Ossman <drzeus-list@drzeus.cx> wrote:
>
>   
>> I suppose it's a matter of taste, but personally I think the mere
>> mentioning of 'for' allows you to directly see that there is some kind
>> of looping involved. And it shouldn't be terribly complex:
>>
>> for (i = 0;i < 8;i++) {
>>     resp[i] = readw(addr + RESPONSE + (7 - i)*4) << 16;
>>     resp[i] |= readw(addr + RESPONSE + (6 - i)*4);
>> }
>>
>>     
>
> The actual loop is slightly different (there are 4 elements in cmd->resp):
>   

My bad. I got confused with your eight registers. ;)

> for (i=0; i < 4; i++) {
>       resp[i] = readl(addr + RESP + (7 - 2 * i) * 4) << 16;
>       resp[i] |= readl(addr + RESP + (6 - 2 * i) * 4);
> }
> As there are only 4 iterations it's not a lot of work to spare the compiler from address
> calculation. readl also seems more appropriate than readw, as resp is array of u32.
>
>   

I smell premature optimisation. Besides, the compiler is probably better
than you at unraveling that loop in an efficient manner anyway. You
should generally start with readable and obviously correct code and
optimise only when bottle necks are found. It keeps the code
maintainable in the long run.

As for the readw(), it was because you said only 16 of the 32 bits
contained anything of value.

> I changed the variable and function names to *_timeout, but left the macros as *_TO. This way,
> the macro name corresponds to the datasheet and the meaning is evident from context:
>
> writel(data_timeout, sock->addr + SOCK_MMCSD_DATA_TO);
>
>   

Great. That should allow even the most inexperienced reader to
understand the code.

> Additionally, I added defines for response and command types.
>
>   

Should be ready for merge then. We just need to sort out exactly where
to put the files. And Russell probably wants his say in this as well. ;)

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
>
> If I understand correctly, there should be two different ways to report bytes_xfered:
> 1. for read operations, the current block/byte counter reporting is sufficient
>   

It should be the number of successfully received bytes all the way to
the kernel. But this is difficult to get wrong. ;)

> 2. for write operation, error-less BUSY assert/de-assert pairs shall be counted instead
> Currently I only look at the last BUSY de-assert to verify that command is completed successfully.
> As mmc_block always issues single block writes it is sufficient. If this will ever change, more
> sophisticated scheme can be devised.
>
>   

This is about to change. Hence the need to check that all drivers are
reporting what they're supposed to.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

--- Pierre Ossman <drzeus-list@drzeus.cx> wrote:

> Russell King wrote:
> > It's really the bus we care about at this stage, since the errors we
> > receive are along the lines of "the card reported that the last data
> > block had a CRC error", "we encountered an underrun condition during
> > the last data block", or "the card didn't request data before we
> > timed out", etc.
> >
> > Basically, the transfer of the next block confirms that the previous
> > block was successfully received by the card.
> >
> >   
> 
> Ehm... Now I'm a bit confused. At the point of a bus error, there
> difference between the data sent to the bus and the data successfully
> received by the card should amount to one block (as the last block got
> NACK:ed for whatever reason). If we expect host drivers to report the
> bytes sent to the bus, we need to subtract one block from the value
> reported to the block layer.
> 
> Rgds
> Pierre
> 
If I understand correctly, there should be two different ways to report bytes_xfered:
1. for read operations, the current block/byte counter reporting is sufficient
2. for write operation, error-less BUSY assert/de-assert pairs shall be counted instead
Currently I only look at the last BUSY de-assert to verify that command is completed successfully.
As mmc_block always issues single block writes it is sufficient. If this will ever change, more
sophisticated scheme can be devised.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

--- Pierre Ossman <drzeus-list@drzeus.cx> wrote:

> 
> I suppose it's a matter of taste, but personally I think the mere
> mentioning of 'for' allows you to directly see that there is some kind
> of looping involved. And it shouldn't be terribly complex:
> 
> for (i = 0;i < 8;i++) {
>     resp[i] = readw(addr + RESPONSE + (7 - i)*4) << 16;
>     resp[i] |= readw(addr + RESPONSE + (6 - i)*4);
> }
> 

The actual loop is slightly different (there are 4 elements in cmd->resp):
for (i=0; i < 4; i++) {
      resp[i] = readl(addr + RESP + (7 - 2 * i) * 4) << 16;
      resp[i] |= readl(addr + RESP + (6 - 2 * i) * 4);
}
As there are only 4 iterations it's not a lot of work to spare the compiler from address
calculation. readl also seems more appropriate than readw, as resp is array of u32.


> The problem is that it's a big difference between
> seeing "data TO" and
> seeing "data to" in the code. How about using the
> three letter
> abbreviations in those places? I.e. "cto" and "dto"?
I changed the variable and function names to *_timeout, but left the macros as *_TO. This way,
the macro name corresponds to the datasheet and the meaning is evident from context:

writel(data_timeout, sock->addr + SOCK_MMCSD_DATA_TO);


Additionally, I added defines for response and command types.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Russell King wrote:
> It's really the bus we care about at this stage, since the errors we
> receive are along the lines of "the card reported that the last data
> block had a CRC error", "we encountered an underrun condition during
> the last data block", or "the card didn't request data before we
> timed out", etc.
>
> Basically, the transfer of the next block confirms that the previous
> block was successfully received by the card.
>
>   

Ehm... Now I'm a bit confused. At the point of a bus error, there
difference between the data sent to the bus and the data successfully
received by the card should amount to one block (as the last block got
NACK:ed for whatever reason). If we expect host drivers to report the
bytes sent to the bus, we need to subtract one block from the value
reported to the block layer.

Rgds
Pierre


-- 
VGER BF report: H 1.64317e-09

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Russell King]

On Sun, Sep 03, 2006 at 12:41:01AM -0700, Alex Dubov wrote:
> > What I'd like to see from you is to double check
> > that bytes_xfered is
> > set to the number of bytes successfully sent to the
> > _card_, not the
> > controller. This is critical for correct handling of
> > bus errors.
> The OMAP datasheet is somewhat unclear, but I think
> that block and byte counters truly represent the
> amount of data shifted out to the mmc bus. Whether
> this data really reaches the flash memory I don't know
> to tell.

It's really the bus we care about at this stage, since the errors we
receive are along the lines of "the card reported that the last data
block had a CRC error", "we encountered an underrun condition during
the last data block", or "the card didn't request data before we
timed out", etc.

Basically, the transfer of the next block confirms that the previous
block was successfully received by the card.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

-- 
VGER BF report: H 0.00934292

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
>> tifm_sd_fetch_resp() could be redone as a for loop
>> to make it more
>> obvious what's going on.
>>     
> I'm not sure it's a good idea. The response value is
> returned in 8 16-bit registers, which are mapped over
> 8 32-bit registers (so that only LS part of each
> register is valid). Additionally, the fetch order is
> reversed, so cmd->resp[0] is fetched from offsets 24
> and 28, while cmd->resp[3] is fetched from offsets 0
> and 4. To write this as a loop requires moderately
> complex address calculation that may look even less
> obvious.
>
>   

I suppose it's a matter of taste, but personally I think the mere
mentioning of 'for' allows you to directly see that there is some kind
of looping involved. And it shouldn't be terribly complex:

for (i = 0;i < 8;i++) {
    resp[i] = readw(addr + RESPONSE + (7 - i)*4) << 16;
    resp[i] |= readw(addr + RESPONSE + (6 - i)*4);
}

>> You should probably rename tifm_sd_set_data_to(). It
>> isn't obvious that
>> 'to' stands for 'timeout'. Same thing with other
>> instances of 'to'.
>>     
> I agree, yet I wanted to retain the names of the
> registers as defined in datasheet (so it's easier to
> search for them; for some reason it always abbreviates
> timeout as TO). Apparently TI does the same in their
> drivers.
>
>   

The problem is that it's a big difference between seeing "data TO" and
seeing "data to" in the code. How about using the three letter
abbreviations in those places? I.e. "cto" and "dto"?

>> What I'd like to see from you is to double check
>> that bytes_xfered is
>> set to the number of bytes successfully sent to the
>> _card_, not the
>> controller. This is critical for correct handling of
>> bus errors.
>>     
> The OMAP datasheet is somewhat unclear, but I think
> that block and byte counters truly represent the
> amount of data shifted out to the mmc bus. Whether
> this data really reaches the flash memory I don't know
> to tell.
>
>   

Hmm.... I'm planning on putting together a test module to determine this
(as my specs aren't crystal clear on the subject either). I'll try to
remember to send it to you. :)

Rgds
Pierre


-- 
VGER BF report: U 0.5

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Greg KH wrote:
> On Sat, Sep 02, 2006 at 10:50:14PM +0200, Pierre Ossman wrote:
>   
>>
>> This is a PCI device yes. Which has a number of card readers as
>> separate, hot-pluggable functions. Currently this means it interacts
>> with the block device and MMC subsystems of the kernel. As more drivers
>> pop up, the other card formats will probably get their own subsystems
>> the way MMC has. So there are three issues here:
>>
>>  * Where to put the central module that handles the generic parts of the
>> chip and pulls in the other modules as needed.
>>     
>
> Right now, the drivers/mmc directory has such a driver, the sdhci.c
> file, right?
>
>   

Not quite. sdhci is a vendor-neutral MMC controller driver. What I'm
talking about here is the interface-neutral portion for the Texas
Instruments multi-format card reader.

>>  * If the subfunction modules should be put with the subsystems they
>> connect to or with the main, generic module.
>>     
>
> It all depends on how bit it grows over time.  It is always easy to move
> files around at a later time if you so wish.
>
> For now, is the drivers/mmc/ directory acceptable?  If other card
> formats show up, we can reconsider it at that time.  Is that ok?
>   

Support for MemoryStick isn't that far off in the future, so it would be
preferable to get this right from the start.

Is there no driver in the kernel that already has this design?

Rgds
Pierre


-- 
VGER BF report: U 0.499916

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

> The constants you've borrowed from OMAP, have you
> confirmed all of them?
All the constants defined in tifm_sd.c are used in
accordance to OMAP datasheet with expected effect.
Constants that I've guessed mostly reside in the
tifm.h file and marked accordingly.

> tifm_sd_fetch_resp() could be redone as a for loop
> to make it more
> obvious what's going on.
I'm not sure it's a good idea. The response value is
returned in 8 16-bit registers, which are mapped over
8 32-bit registers (so that only LS part of each
register is valid). Additionally, the fetch order is
reversed, so cmd->resp[0] is fetched from offsets 24
and 28, while cmd->resp[3] is fetched from offsets 0
and 4. To write this as a loop requires moderately
complex address calculation that may look even less
obvious.

> 
> You should probably rename tifm_sd_set_data_to(). It
> isn't obvious that
> 'to' stands for 'timeout'. Same thing with other
> instances of 'to'.
I agree, yet I wanted to retain the names of the
registers as defined in datasheet (so it's easier to
search for them; for some reason it always abbreviates
timeout as TO). Apparently TI does the same in their
drivers.

> What I'd like to see from you is to double check
> that bytes_xfered is
> set to the number of bytes successfully sent to the
> _card_, not the
> controller. This is critical for correct handling of
> bus errors.
The OMAP datasheet is somewhat unclear, but I think
that block and byte counters truly represent the
amount of data shifted out to the mmc bus. Whether
this data really reaches the flash memory I don't know
to tell.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

-- 
VGER BF report: U 0.499996

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Greg KH]

On Sat, Sep 02, 2006 at 10:50:14PM +0200, Pierre Ossman wrote:
> Andrew Morton wrote:
> > On Sat, 02 Sep 2006 13:15:52 +0200
> > Pierre Ossman <drzeus-list@drzeus.cx> wrote:
> >   
> >> Andrew, we could use some help with how this driver should fit into the
> >> kernel tree. The hardware is multi-function, so there will be a couple
> >> of drivers, one for every function, and a common part. How has this been
> >> organised in the past?
> >>
> >>     
> >
> > Greg would be a far better person that I for this.   Is it a PCI device?
> >   
> 
> It's always difficult to know who to contact when there's an issue that
> isn't specific to one single area. And since you are the 2.6 maintainer
> I figured it wouldn't be too off base to throw this in your lap. ;)
> 
> This is a PCI device yes. Which has a number of card readers as
> separate, hot-pluggable functions. Currently this means it interacts
> with the block device and MMC subsystems of the kernel. As more drivers
> pop up, the other card formats will probably get their own subsystems
> the way MMC has. So there are three issues here:
> 
>  * Where to put the central module that handles the generic parts of the
> chip and pulls in the other modules as needed.

Right now, the drivers/mmc directory has such a driver, the sdhci.c
file, right?

>  * If the subfunction modules should be put with the subsystems they
> connect to or with the main, generic module.

It all depends on how bit it grows over time.  It is always easy to move
files around at a later time if you so wish.

For now, is the drivers/mmc/ directory acceptable?  If other card
formats show up, we can reconsider it at that time.  Is that ok?

thanks,

greg k-h

-- 
VGER BF report: U 0.476704

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Andrew Morton wrote:
> On Sat, 02 Sep 2006 13:15:52 +0200
> Pierre Ossman <drzeus-list@drzeus.cx> wrote:
>   
>> Andrew, we could use some help with how this driver should fit into the
>> kernel tree. The hardware is multi-function, so there will be a couple
>> of drivers, one for every function, and a common part. How has this been
>> organised in the past?
>>
>>     
>
> Greg would be a far better person that I for this.   Is it a PCI device?
>   

It's always difficult to know who to contact when there's an issue that
isn't specific to one single area. And since you are the 2.6 maintainer
I figured it wouldn't be too off base to throw this in your lap. ;)

This is a PCI device yes. Which has a number of card readers as
separate, hot-pluggable functions. Currently this means it interacts
with the block device and MMC subsystems of the kernel. As more drivers
pop up, the other card formats will probably get their own subsystems
the way MMC has. So there are three issues here:

 * Where to put the central module that handles the generic parts of the
chip and pulls in the other modules as needed.

 * If the subfunction modules should be put with the subsystems they
connect to or with the main, generic module.

Rgds
Pierre


-- 
VGER BF report: H 0.055417

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Andrew Morton]

On Sat, 02 Sep 2006 13:15:52 +0200
Pierre Ossman <drzeus-list@drzeus.cx> wrote:

> Andrew, the stuff meant for you is at the bottom.

<panics>

> Alex Dubov wrote:
> > Hi there.
> > I've made a couple of fixes to my flashmedia driver
> > (http://developer.berlios.de/projects/tifmxx/) to the
> > effect of much improved R/W speed in PIO mode and
> > writing speed in DMA mode.
> >   
> 
> The users will be pleased :)
> 
> > I also tried to clean-up reverse engineering mess out
> > of the code - it should be more readable now.
> >   
> 
> Wonderful. Things are looking a lot better. I have a few questions though.
> 
> The constants you've borrowed from OMAP, have you confirmed all of them?
> If not, you should add a comment to those that are pure speculation so far.
> 
> tifm_sd_op_flags() still use literals. Could you fix up some defines
> there as well?
> 
> tifm_sd_fetch_resp() could be redone as a for loop to make it more
> obvious what's going on. Also, please don't put several statements on
> one line.
> 
> You should probably rename tifm_sd_set_data_to(). It isn't obvious that
> 'to' stands for 'timeout'. Same thing with other instances of 'to'.
> 
> We're also in the process of fixing this dreadfully slow write mess.
> What I'd like to see from you is to double check that bytes_xfered is
> set to the number of bytes successfully sent to the _card_, not the
> controller. This is critical for correct handling of bus errors.
> 
> > Next on my list is MemoryStick functionality.
> >
> >   
> 
> I would suggest finishing this and getting it merged to be your top
> priority. There are quite a few people who would like to have this
> hardware supported. Which brings me to...
> 
> Andrew, we could use some help with how this driver should fit into the
> kernel tree. The hardware is multi-function, so there will be a couple
> of drivers, one for every function, and a common part. How has this been
> organised in the past?
> 

Greg would be a far better person that I for this.   Is it a PCI device?

-- 
VGER BF report: H 0

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Andrew, the stuff meant for you is at the bottom.

Alex Dubov wrote:
> Hi there.
> I've made a couple of fixes to my flashmedia driver
> (http://developer.berlios.de/projects/tifmxx/) to the
> effect of much improved R/W speed in PIO mode and
> writing speed in DMA mode.
>   

The users will be pleased :)

> I also tried to clean-up reverse engineering mess out
> of the code - it should be more readable now.
>   

Wonderful. Things are looking a lot better. I have a few questions though.

The constants you've borrowed from OMAP, have you confirmed all of them?
If not, you should add a comment to those that are pure speculation so far.

tifm_sd_op_flags() still use literals. Could you fix up some defines
there as well?

tifm_sd_fetch_resp() could be redone as a for loop to make it more
obvious what's going on. Also, please don't put several statements on
one line.

You should probably rename tifm_sd_set_data_to(). It isn't obvious that
'to' stands for 'timeout'. Same thing with other instances of 'to'.

We're also in the process of fixing this dreadfully slow write mess.
What I'd like to see from you is to double check that bytes_xfered is
set to the number of bytes successfully sent to the _card_, not the
controller. This is critical for correct handling of bus errors.

> Next on my list is MemoryStick functionality.
>
>   

I would suggest finishing this and getting it merged to be your top
priority. There are quite a few people who would like to have this
hardware supported. Which brings me to...

Andrew, we could use some help with how this driver should fit into the
kernel tree. The hardware is multi-function, so there will be a couple
of drivers, one for every function, and a common part. How has this been
organised in the past?

Rgds
Pierre


-- 
VGER BF report: H 0.0963121

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

Hi there.
I've made a couple of fixes to my flashmedia driver
(http://developer.berlios.de/projects/tifmxx/) to the
effect of much improved R/W speed in PIO mode and
writing speed in DMA mode.
I also tried to clean-up reverse engineering mess out
of the code - it should be more readable now.

Next on my list is MemoryStick functionality.



__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

-- 
VGER BF report: U 0.5

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
> I was working on making some sense from all the
> constants and discovered that many flashmedia mmc
> registers are very similar in bit assignment to OMAP
> mmc ones (which are documented). Pity I haven't
> noticed it before. I'll take some time now to review
> the driver given this new information.
>
>   

Nice. Hope you find something.

> But what with mmc_host structure that also hangs
> around? I think it deserves the name "host" even more.
>
>   

Perhaps. But the other drivers have chosen to use "mmc" for the mmc_host
structure and "host" for their internal state structure. It's really a
matter of taste, but it's easier to do changes that cover all/many
drivers when they all use the same naming conventions.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

I was working on making some sense from all the
constants and discovered that many flashmedia mmc
registers are very similar in bit assignment to OMAP
mmc ones (which are documented). Pity I haven't
noticed it before. I'll take some time now to review
the driver given this new information.


> Ah, ok, I see. The socket structure would then be
> similar to a device
> structure in the kernel. Perhaps you should use the
> name "host" instead
> of "card" then as that is widely used in the other
> mmc host drivers.

But what with mmc_host structure that also hangs
around? I think it deserves the name "host" even more.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
> It appears that any socket can hold any card type. So
> the logic goes as following:
> 1. Get insertion interrupt on socket
> 2. Detect card type
> 3. Stick proper handler to the socket
> ...do work
> 4. Get removal interrupt on socket
> 5. Ditch type-specific handler, mark socket as empty
> A quick example: on 8033 sd cards are most often wired
> to socket 3, while on 803b they are wired to socket 1.
>
>   

Ah, ok, I see. The socket structure would then be similar to a device
structure in the kernel. Perhaps you should use the name "host" instead
of "card" then as that is widely used in the other mmc host drivers. It
makes it easier when reviewing your future patches.

> I'm afraid you'll have to clarify this issue further.
> Consider the following: TI uses look up table to set
> command type register. The decoding of this table is
> my tifm_sd_op_flags. Its output directly sent to
> hardware. Can you notice any bit patterns here:
> Response type (4b):
> R1 - 1     R1b - 9 (so highest 1 is MMC_RSP_BUSY)
> R2 - 2     R3 - 3      R6 - 6
> (if bit 1 is MMC_RSP_136 why it is set for R3 and R6;
> and if it's MMC_RSP_CRC why it is not set for R1?)
>   

Some controllers are brain dead like this, which makes forward
compatibility a bit difficult. I just wanted to make sure as some
previous drivers have added this artificial restriction in just the
software.

I would still like it to bail out on an unknown type though as anything
else can generate really unpredictable and annoying bugs.

> Command type (2b):
> BC - 0 (implied)   BCR  - 1
> AC - 2             ADTC - 3 
> (even if higher bit stands for "addressed vs
> broadcast" it still doesn't make sense for lower bit).
>   

These are more functional in nature, and therefore exactly how things
should work.

> By the way, if I recall correctly, SD spec does not
> splits command types and responses into components. It
> always speaks of R1s and R6s and so on.
>
>   

The SD spec is focused on cards, not controllers. If you check their
controller spec (sdhci), you'll see that they've split it up.

> It's not hard to figure out bits that are used
> systematically. The problem is that there are 4 or 5
> registers that are set to some constant at
> initialization. I made all kinds of trials with them
> and got no conclusive idea on their meaning.
>
>   

That's all one can expect. :)

You could put some comments about the registers somewhere so that other
people working on the driver will now what you've already tested.
Reverse engineered drivers are usually difficult for others to get into,
so everything you document will increase the changes of more people
helping you out.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

> Then try to make qualified guesses. Even if the
> constants are
> TIFM_INTFLAG_2, that still makes the code more
> readable as you see which
> values are the same constant.
It is not necessarily good idea for constants that are
used only once on initialization. May be a side
comment will be better. I'll check this out.

> Is it possible for a "socket" to have
> multiple "card":s
> associated with it? Otherwise I see little need for
> the dynamic behaviour.

It appears that any socket can hold any card type. So
the logic goes as following:
1. Get insertion interrupt on socket
2. Detect card type
3. Stick proper handler to the socket
...do work
4. Get removal interrupt on socket
5. Ditch type-specific handler, mark socket as empty
A quick example: on 8033 sd cards are most often wired
to socket 3, while on 803b they are wired to socket 1.

> Sorry, I was a bit unclear. Of course hardware cares
> about what kind of
> response it will get. What I meant was that hardware
> shouldn't care if
> it's R1, R2 or R666. 

I'm afraid you'll have to clarify this issue further.
Consider the following: TI uses look up table to set
command type register. The decoding of this table is
my tifm_sd_op_flags. Its output directly sent to
hardware. Can you notice any bit patterns here:
Response type (4b):
R1 - 1     R1b - 9 (so highest 1 is MMC_RSP_BUSY)
R2 - 2     R3 - 3      R6 - 6
(if bit 1 is MMC_RSP_136 why it is set for R3 and R6;
and if it's MMC_RSP_CRC why it is not set for R1?)

Command type (2b):
BC - 0 (implied)   BCR  - 1
AC - 2             ADTC - 3 
(even if higher bit stands for "addressed vs
broadcast" it still doesn't make sense for lower bit).

By the way, if I recall correctly, SD spec does not
splits command types and responses into components. It
always speaks of R1s and R6s and so on.

> Baby steps. If you test carefully enough (and do
> some qualified
> guessing), you usually can figure out what most bits
> of a register are for.

It's not hard to figure out bits that are used
systematically. The problem is that there are 4 or 5
registers that are set to some constant at
initialization. I made all kinds of trials with them
and got no conclusive idea on their meaning.



__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Alex Dubov wrote:
> They are magic numbers. I have only the vague idea on
> what most of these numbers mean. I digged them out
> from TI's/everest's binary driver.
>   

Then try to make qualified guesses. Even if the constants are
TIFM_INTFLAG_2, that still makes the code more readable as you see which
values are the same constant.

By "magic", in the valid sense, I do not mean "unknown". What I mean is
"purposely random", as in values used to identify file systems and file
formats.

>
> I already have "socket". "card" is something that is
> plugged into the "socket". It's hard to think about
> short name that fits here nicely.
>
>   

I assume "socket" is a structure for one of the card type subfunctions
of the controller and that "card" is something you create once that
subfunction is activated by a card insertion. Why can't you have the
"card" portions allocated at all times as part of the "socket"
structure? Is it possible for a "socket" to have multiple "card":s
associated with it? Otherwise I see little need for the dynamic behaviour.

> Unfortunately, hardware does care. Output of
> tifm_sd_op_flag is set into upper half of command
> register. 
>   

Sorry, I was a bit unclear. Of course hardware cares about what kind of
response it will get. What I meant was that hardware shouldn't care if
it's R1, R2 or R666. What it should care about is if it's "Short
response with CRC and embedded opcode" or something similar. If you look
at the meaning of the response types and try to compare the different
bit combinations, you can usually see patterns. Just remember that most
controllers do not usually support checking every little aspect.

> The fall-back is 0 (implied default for both
> switches).
>   

And you are sure this will be valid for response type RFooBar,
consisting of only a new combination of the existing response
attributes, that I will come up with tomorrow?

> The TI binary drivers tests for the command type
> before setting the appropriate bit in command register
> (similar to tifm_sd_op_flags).
>   

Please do a BUG_ON() or fail the request or something similar. If you
would happen to get a request where cmd->data is set, but type isn't
ADTC, then I assume all hell will break loose.

> In general: while I'm using code flow different from
> this used in TI's binary drivers, I tried very hard to
> preserve register access semantics. 
>   

Careful. People get a bit edgy with such reasoning around reverse
engineering. ;)

> When I failed to
> do so, very bad things happened - sporadic and brutal
> kernel crashes and run-aways. I think they were caused
> by device writing random junk to some memory address
> at arbitrary times.
>
>   

Baby steps. If you test carefully enough (and do some qualified
guessing), you usually can figure out what most bits of a register are for.

Great work figuring out the hardware as much as you have though. This
driver will be a nice addition.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

--- Pierre Ossman <drzeus-list@drzeus.cx> wrote:

> Some comments from a MMC/SD perspective.
> 
> On a general note, please replace all your constants
> with defines. Magic
> values are no fun (unless they are in fact a magic
> number ;)).
They are magic numbers. I have only the vague idea on
what most of these numbers mean. I digged them out
from TI's/everest's binary driver.

Luckily, Vayo's linux binary had every register
accessed from separate function, so, at least,
register names are known with certain confidence.

> 
> Also, calling the struct "card" might be a bit
> misleading as it might be
> a bus in the MMC case.
I already have "socket". "card" is something that is
plugged into the "socket". It's hard to think about
short name that fits here nicely.

> 
> In tifm_sd_o_flags(), try not to case on response
> types as the hardware
> shouldn't have to care about this. If you really,
> really, _really_ must
> do this, then make sure you have a default that
> prints something nasty
> and fails the request with an error.

Unfortunately, hardware does care. Output of
tifm_sd_op_flag is set into upper half of command
register. The fall-back is 0 (implied default for both
switches).

> 
> A default is also needed for cmd_flags in the same
> function.
> 
> In tifm_sd_exec(), you should only need to test for
> the presence of
> cmd->data, not that the command is of ADTC type.

The TI binary drivers tests for the command type
before setting the appropriate bit in command register
(similar to tifm_sd_op_flags).

In general: while I'm using code flow different from
this used in TI's binary drivers, I tried very hard to
preserve register access semantics. When I failed to
do so, very bad things happened - sporadic and brutal
kernel crashes and run-aways. I think they were caused
by device writing random junk to some memory address
at arbitrary times.



__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Pierre Ossman]

Some comments from a MMC/SD perspective.

On a general note, please replace all your constants with defines. Magic
values are no fun (unless they are in fact a magic number ;)).

Also, calling the struct "card" might be a bit misleading as it might be
a bus in the MMC case.

In tifm_sd_o_flags(), try not to case on response types as the hardware
shouldn't have to care about this. If you really, really, _really_ must
do this, then make sure you have a default that prints something nasty
and fails the request with an error.

A default is also needed for cmd_flags in the same function.

In tifm_sd_exec(), you should only need to test for the presence of
cmd->data, not that the command is of ADTC type.

Rgds
Pierre

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

Ok - fixed those.

--- Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Thu, Jul 27, 2006 at 08:34:06PM -0700, Alex Dubov
> wrote:
> > The driver is called tifmxx and available from:
> > http://developer.berlios.de/projects/tifmxx/
> 
> 1. Usual name for spinlocking flags is
> 
> 	unsigned long flags;
> 
> 2. Preferred CS for if statements is
> 
> 	if (foo)
> 		bar; /* two lines no matter how short */
> 
> 3. Check for NULL at the start of tifm_7xx1_remove
> is unnecessary.
>    You've saved valid fm at probe time, right?
> 
> 4. If ->suspend and ->resume are not implemented why
> add dummy ones?
> 
> 5. 
> 
> 


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

The exact condition is (irq_status!=0 &&
irq_status!=0xffffffff). I think it is not any better
that what I have.

--- Andrey Panin <pazke@donpac.ru> wrote:

> On 208, 07 27, 2006 at 08:34:06PM -0700, Alex Dubov
> wrote:
> 
> What this strange line (in tifm_7xx1_isr function)
> is supposed to do:
> 
>         if(irq_status && (~irq_status))
> 
> check for nonzero irq_status in most obfuscated way
> ?
> Please replace it with something readable.
> 
> > I would like to announce the availability of the
> > driver for TI FlashMedia flash card readers.
> Currently
> > supported pci ids:
> > 1. 104c:8033.3
> > 2. 104c:803b.2
> > 
> > Device with id 8033 also features sdhci interface
> (as
> > subfunction 4). However, sdhci is disabled on many
> > laptops (notably Acer's), while FlashMedia
> interface
> > is available.
> > 
> > The driver is called tifmxx and available from:
> > http://developer.berlios.de/projects/tifmxx/
> > 
> > Only mmc/sd cards are supported at present, via
> mmc
> > subsystem. Provisions for other card types (Sony
> MS,
> > xD and such) are in place, but no support is
> available
> > due to lack of hardware and interest.
> > 
> > 
> > __________________________________________________
> > Do You Yahoo!?
> > Tired of spam?  Yahoo! Mail has the best spam
> protection around 
> > http://mail.yahoo.com 
> > -
> > 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/
> > 
> 
> -- 
> Andrey Panin		| Linux and UNIX system administrator
> pazke@donpac.ru		| PGP key: wwwkeys.pgp.net
> 


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Andrey Panin]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

On 208, 07 27, 2006 at 08:34:06PM -0700, Alex Dubov wrote:

What this strange line (in tifm_7xx1_isr function) is supposed to do:

        if(irq_status && (~irq_status))

check for nonzero irq_status in most obfuscated way ?
Please replace it with something readable.

> I would like to announce the availability of the
> driver for TI FlashMedia flash card readers. Currently
> supported pci ids:
> 1. 104c:8033.3
> 2. 104c:803b.2
> 
> Device with id 8033 also features sdhci interface (as
> subfunction 4). However, sdhci is disabled on many
> laptops (notably Acer's), while FlashMedia interface
> is available.
> 
> The driver is called tifmxx and available from:
> http://developer.berlios.de/projects/tifmxx/
> 
> Only mmc/sd cards are supported at present, via mmc
> subsystem. Provisions for other card types (Sony MS,
> xD and such) are in place, but no support is available
> due to lack of hardware and interest.
> 
> 
> __________________________________________________
> Do You Yahoo!?
> Tired of spam?  Yahoo! Mail has the best spam protection around 
> http://mail.yahoo.com 
> -
> 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/
> 

-- 
Andrey Panin		| Linux and UNIX system administrator
pazke@donpac.ru		| PGP key: wwwkeys.pgp.net

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alexey Dobriyan]

On Thu, Jul 27, 2006 at 08:34:06PM -0700, Alex Dubov wrote:
> The driver is called tifmxx and available from:
> http://developer.berlios.de/projects/tifmxx/

1. Usual name for spinlocking flags is

	unsigned long flags;

2. Preferred CS for if statements is

	if (foo)
		bar; /* two lines no matter how short */

3. Check for NULL at the start of tifm_7xx1_remove is unnecessary.
   You've saved valid fm at probe time, right?

4. If ->suspend and ->resume are not implemented why add dummy ones?

5. 

Support for TI FlashMedia (pci id 104c:8033, 104c:803b) flash card readers

[Alex Dubov]

I would like to announce the availability of the
driver for TI FlashMedia flash card readers. Currently
supported pci ids:
1. 104c:8033.3
2. 104c:803b.2

Device with id 8033 also features sdhci interface (as
subfunction 4). However, sdhci is disabled on many
laptops (notably Acer's), while FlashMedia interface
is available.

The driver is called tifmxx and available from:
http://developer.berlios.de/projects/tifmxx/

Only mmc/sd cards are supported at present, via mmc
subsystem. Provisions for other card types (Sony MS,
xD and such) are in place, but no support is available
due to lack of hardware and interest.


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com