|
Rockbox mail archiveSubject: Re: r21912: Storage API : remove undeeded target-specific functionsRe: 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 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é
Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |