|
Rockbox mail archiveSubject: Re: r21912: Storage API : remove undeeded target-specific functionsRe: r21912: Storage API : remove undeeded target-specific functions
From: Thomas Martitz <thomas.martitz_at_student.htw-berlin.de>
Date: Fri, 17 Jul 2009 16:17:56 +0200 Rafaël Carré schrieb: > 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. > I don't know the term, but I figure with voiding you mean making it a simple #define? The complexity isn't hidden, at least not for plugin 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. > Probably only because they're not in the plugin API. > 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. > > I don't agree with that. I think the opposite way would be proper, to enfore the API. >> 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? > See, here the "complexity is hidden for developers" argument becomes simply wrong. For core code it's hidden, for plugins not. That's even worse than #ifdeffing all the way IMO. > 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) > That's the downside of a generic API. Is that a problem? Why was it API'fied in the first place? Either we use an API, which may cost a tiny bit binsize due to stubs, or we go for plain disk type specific functions. What we have now is a mix. An API which pretends to be one, but which isn't one actually. This is just bad, IMO. > >> In any case, I would rather see this done as part of the existing >> storage rework patch. Is that anywhere near ready for commit? It's been quite about it since a long time. Best regards. Received on 2009-07-17 Page template was last modified "Tue Sep 7 00:00:02 2021" The Rockbox Crew -- Privacy Policy |