Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: Re: r21912: Storage API : remove undeeded target-specific functions

Re: 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 was last modified "Jan 10 2012" The Rockbox Crew
aaa