Rockbox

I’d also like to get rid of direct access to storage_sleep() and storage_spindown() in the plugin api, and replace them with something that gives hints to the storage layer based on path (or file descriptor)

This one gets rid of *_get_identify() for non-ata, and introduces *_get_info() instead. None of the mentioned problems have been solved yet.

   storage_v2.diff (128.9 KiB)

Hi Frank,

My few comments, based on skimming through the diff over lunch (I haven’t tested on any target yet):

1. What you have here is a candidate for “a good first step” already, assuming the sim can be made to compile without too much hassle. It’s probably a good idea to split big changes like this into smaller parts, and maybe it’s a good idea to keep things that might intentionally change behaviour (eg. disk/partition handling, fixing HAVE_HOTSWAP/HAVE_MULTIVOLUME confusion) to a separate, later patch.

2. HAVE_DISK_STORAGE should probably be HAVE_STORAGE_DISK, to be consistent with the other HAVE_STORAGE_* macros

3. Is it necessary to have macros for storage_* on single-driver targets? Would inlining those functions be sufficient?

4. Isn’t flash_* redundant, since the iFP flash driver uses NAND anyway?

5. Does the storage_* layer really need to pass *_spin (etc) down to the sd/nand drivers? Could the underlying null implementations be removed and handled like storage_sleepnow()?

6. If we need to make up “could be better” vendor/model/firmware strings on non-ata, wouldn’t it be better not to have them at all?

7. There’s a leftover USING_ATA_CALLBACK in plugin.c (ok, it’s only in a comment..)

8. firmware/export/flash.h has #define MMC_H

9. Are the ‘extern’ function modifiers in the new headers necessary? The fact that they’re in a header at all says they are “external”.

As an aside, I would have thought this is a prime candiate for discussion on the dev mailing list.

Thanks for looking at it!

1. I won’t commit this in one step. Once I’m reasonably happy with it, my plan is to split it into a set of logically consecutive patches and commit those. I also won’t do the disk/partition deconfusion right away, as it is reasonably separated from this.
2. Actually, it means something else. HAVE_DISK_STORAGE and HAVE_FLASH_STORAGE indicate certain features, like spin up times and things like that. HAVE_STORAGE_* basically is a namespace indicator. Maybe this needs to be thought through a bit more.
3. I’d like inline functions as well, but some of these functions get used as callbacks (I think mainly the spinup related ones). Needs more thought.
4. probably. iFP is annoying actually, as it doesn’t compile at all.
5. They should probably go, yes.
6. USB storage needs names. I don’t think anything else does though.
7. I’ll fix that
8. Oops. I’ll fix that too (unless I drop the entire file).

As soon as I’ve tested it a bit, I’ll bring it up on the ML. I think it’s a bit too invasive to commit without a proper warning.

Changes since v2:
- now has inline functions
- no flash_* anymore ata_flash.c now also uses nand_*
- no passing down of unneeded functions any more.
- now also has changes to the bootloaders
- the simulators compile and run again

Still totally untested (except for compilation)
I think that this is feature-complete enough for a first phase, but more review (and possibly splitting) is needed.
Most targets need between 100 and 500 bytes more (I think because the new *_get_info() function), but the tcc ones actually need about 2kb less.

   storage_v3.diff (140.8 KiB)

Sync to r18950

   storage_v4.diff (106.5 KiB)

Now tested on sansa c200, ipod mini and video (with and without rockbox usb), iriver h10, h120 and h320, gigabeat f,gigabeat s mrobe 100, iaudio x5l, cowon d2. Test planned on Archos Player (waiting for batteries to charge).

Current deltas (RAM usage) are : cowond2 -2064, gigabeatf +352, gigabeats +400, h10_5gb +464, h120 +264, h300 +272, iaudiox5 +288, ipodmini2g +448, ipodvideo +480, mrobe100 +448, player +264, sansac200 -128

*_get_info() is only used by usb_storage.c and debug_menu.c (as a fallback, currently only for tcc players until they have a proper disk_callback() function). Not including that reduces the delta a bit for hardware USB targets. The deltas then go down by 140 bytes for sh, 190 for coldfire and 220 for arm. Is this worth the extra #ifdef in all storage drivers, and what should that #ifdef actually depend on?

Other than possible delta reductions, and assuming that the player test won’t give any surprises, I think this is ready for commit.

   storage_v5.diff (106.3 KiB)