Rockbox mail archiveSubject: Re: r21912: Storage API : remove undeeded target-specific functions
Re: r21912: Storage API : remove undeeded target-specific functions
From: Rafaël Carré <rafael.carre_at_gmail.com>
Date: Fri, 17 Jul 2009 15:47:24 +0200
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
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,
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.
-- Rafaël Carré