On Fri, 17 Jul 2009 08:49:14 +0100
Rob Purchase <rob.purchase_at_googlemail.com> wrote:

> On 17/07/2009 05:55, Thomas Martitz wrote:
> >> storage_sleep, storage_spin, storage_spindown are only defined if
> >> #defiend (HAVE_DISK_STORAGE), not for MMC/ATA/SD
> >> remove already unneeded nand_disk_is_active, nand_soft_reset
> > Isn't the point of an API to forget about underlying differences so
> > that a developer doesn't need to care/know about this differences?
> > This commit introduces more (imo ugly) #ifdefs with destroying that
> > point.
> I agree, the idea of the storage_* layer was to hide the underlying
> differences and avoid the need for those #ifdefs.

Since the functions are voided in storage.h , the complexity is hidden
from developers.

That is different for storage_spin,storage_sleep, and storage_spindown
since they are used in the plugin API.

Some calls to these functions were already undef #ifdef
HAVE_DISK_STORAGE, so I only continued this way.

In storage.h , storage_soft_reset and storage_disk_is_active were
already voided for some storage types so I thought it would be right to
continue in this direction.

> A better solution might be for these functions to still exist at
> storage_* level, either by #defining to the relevant driver on
> single-storage-driver targets, or as an actual function on
> multi-driver targets. The storage rework patch (FS#9545) already goes
> some way towards this, but it does not yet remove the unncessary
> driver-specific functions. The actual driver-level functions could
> be voided, but not storage_*.

storage_* functions could be voided (just like this commit does) in the
appropriate configuration, perhaps I missed something.

> The problem with this approach is how to expose those functions to
> the plugin API on single-driver, non-ATA targets. A solution for this
> might simply be to always use a concrete storage_* function. I can't
> really see that being much of an overhead.

They only really need to be exported when HAVE_DISK_STORAGE is defined,
right?

It is true that the overhead is very minimal:

If you look at the build table for this commit, there was a binsize
decrease between 50 and 100 bytes for some targets (onda / SD, ondio /
MMC even if i forgot to remove the actual functions from ata_mmc.c) and
an increase between 38 & 58 bytes for Clip/Fuze/e200v2 (SD).

There is other ways to remove unused code (-ffunction-sections /
-gc-sections, or defining the functions as static inline in
<storage-type>.h but that still leaves quite a lot of disk-specific
functions into all storage drivers (them being implemented at a generic
level or a target-specific level)

> In any case, I would rather see this done as part of the existing
> storage rework patch.

Ok

-- 
Rafaël Carré