Re: [Qemu-devel] [PATCH v3 20/20] arm: add Faraday FTSPI020 SPI flash controller support

[Peter Crosthwaite]

On Thu, Feb 7, 2013 at 10:13 PM, Andreas Färber <address@hidden> wrote:
> Am 07.02.2013 13:04, schrieb Peter Maydell:
>> On 7 February 2013 11:50, Andreas Färber <address@hidden> wrote:
>>> Am 07.02.2013 07:59, schrieb Kuo-Jung Su:
>>>>>> +/******************************************************************************
>>>>>> + * FTSPI020 registers
>>>>>> + 
>>>>>> *****************************************************************************/
>>>>>> +#define REG_CMD0            0x00    /* Flash address */
>>>>>> +#define REG_CMD1            0x04
>>>>>> +#define REG_CMD2            0x08
>>>>>> +#define REG_CMD3            0x0c
>>>>>> +#define REG_CTRL            0x10    /* Control Register */
>>>>>> +#define REG_ACT             0x14    /* AC Timing Register */
>>>>>> +#define REG_STR             0x18    /* Status Register */
>>>>>> +#define REG_ICR             0x20    /* Interrupt Control Register */
>>>>>> +#define REG_ISR             0x24    /* Interrupt Status Register */
>>>>>> +#define REG_RDST            0x28    /* Read Status Register */
>>>>>> +#define REG_REV             0x50    /* Revision Register */
>>>>>> +#define REG_FEA             0x54    /* Feature Register */
>>>>>> +#define REG_DATA            0x100    /* Data Register */
>>>>>> +
>>>>>
>>>>> These can just live in the C file - they are private to the 
>>>>> implementation.
>>>
>>> No. Having them in a header means they can be reused by qtests.
>>
>> Do we really want to change the public/private boundaries of
>> our source code just for the benefit of the test framework? Ugh.
>
> The truth is (and that applies to the hw/ refactoring series as well)
> that we don't have a clear distinction between "public" and "private"
> headers for devices.
>

I guess a logical conclusion here is that defines that relate to the
programmers model (eg #define REG_ISR 0x0C/4) and parameterization
(e.g. #define NUM_CHIP_SELECTS 4) need to be public while everything
else (implementation stuff) is private. In theory however, the only
client of a programmers model is unit testing. So perhaps there should
be some sort of poisoning system like with the cpu headers? Your only
allowed to include a header for a concrete class (such as this) if you
own it, or if your a test.

>>> To embed the device into a SoC object, the state struct and TYPE_
>>> constant should be in the header. That applies to each device part of
>>> the SoC rather than on the board.
>>

I really think this is a step backwards. Every little lightweight
device model needs its own header just for

#define TYPE_FOO "foo"

Regards,
Peter

>> I would like it if we could define what we consider to be best
>> practice for public/private header files for QOM devices
>> (in particular whether we need a public header for every device
>> with the TYPE_ constants &c).
>
> gtk-doc for instance needs to scan one directory, so having all headers
> in include/ would be beneficial long-term - today we do not get
> generated documentation for qdev-core.h.
>
> The separation between PCI internal helpers and
> structs/typedefs-to-be-used is made by including specific headers
> depending on what you need.
>
> In the refcatoring thread I suggested that if we want to have all SPI
> devices in hw/spi/ then each device that needs a public function
> declaration or TYPE_ or struct should export its own header. If we group
> them into a common directory such as hw/arm/ that could be shared across
> SoC devices. Therefore I was suggesting not to split up too much but
> seem to be overruled. ;) There's downsides to everything.
>
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>