Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Blue Swirl]

On Mon, Sep 14, 2009 at 9:59 PM, Markus Armbruster <address@hidden> wrote:
> Blue Swirl <address@hidden> writes:
>
>> On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias
>> <address@hidden> wrote:
>>> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote:
>>>> On 09/12/09 12:10, Edgar E. Iglesias wrote:
>>>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
>>>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<address@hidden>
>>>>>> wrote:
>>>>>>> xilinx.h defines a couple of static inline functions for creating
>>>>>>> devices.  While that's a fair technique for hot functions, device
>>>>>>> initialization is about as cold as it gets.  Define them in the device
>>>>>>> source files instead, and keep only declarations in the header.
>>>>>>
>>>>>> If I understood the qdev plan correctly, this is going to wrong
>>>>>> direction. These functions should reside near the instantiation, not
>>>>>> in the device code. The current approach looks OK if there are going
>>>>>> to be more users of the devices.
>>>>
>>>> The functions should go away ;)
>>>>
>>>> Some day the information carried by those code snippeds should come from a
>>>> machine description file, then we'll don't need them any more.
>>>
>>> I agree.
>>>
>>>>
>>>>> I agree that they shouldn't be in the device source.
>>>>> The reason they ended up in a header and not with the petalogix board
>>>>> was that in my tree there are multiple boards using these functions
>>>>> to easy instantiate devices.
>>>>
>>>> They have to be somewhere.  Having them in a header file is unclean. Having
>>>> them in the board-specific code isn't practical when multiple boards share
>>>> the code.  I'd stick them to the device source code as well.  Also note
>>>> that this is common practice elsewhere in the tree.
>>>
>>> I disagree.
>>>
>>> But if ppl feel very strongly about this, I can remove them and deal with
>>> the code duplication in my tree. Afterall, it's unlikely that upsteam
>>> qemu gets more xilinx boards before some kind of device tree driven
>>> board support is there.
>>
>> I also disagree. Why would the functions in a header file be unclean?
>> The common practice is wrong.
>
> The common practice is to put code in .c files, and stick to
> declarations needed by several .c files in .h files.  It's okay to
> deviate from that common practice if there's a good reason --- we do
> that in places.  However, in this case, I couldn't see a reason, so I
> moved the code from the header to the place where such code lives for
> pretty much every other qdevified device: the device code.

Understandable, since we don't see the source code of the other xilinx boards.

> Doing it the same way everywhere means that maintainers have an easier
> time to predict where stuff is, how it's named, and so forth.  Don't
> disappoint reasonable maintainer expectations without need.

It's still wrong, however disappointing that may be. But with enough
cleanups and guidance, the expectations will be directed to right
path.

> I don't want to make a big deal out of this.  If the code must remain in
> the header, expectations be damned, I'll respin the series and move on.

Pushing the device instantiation code higher makes the device code a
bit simpler. Also the internal state can be kept private, provided
that the interface is designed properly. For example, slavio_timer.c
does not export internal state and all functions are static. The
former init function is now in sun4m.c.

Actually, while I'm quite happy with the device level design (and I
think other targets should also be worked to this direction, not the
opposite), the next level design is not so clear since we don't have
the machine config or something.