Rafaël Carré schrieb:
On Fri, 17 Jul 2009 08:49:14 +0100
Rob Purchase <[email protected]> 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.
