Re: [Qemu-devel] [PATCH V4 0/7] CAN bus support for QEMU (SJA1000 PCI so far)

[Deniz Eren]

Hi Pavel, Philippe,

I’m happy with whatever way is best for the project.

However I would personally think merging the different drivers into one C file 
would not be a very modular way of handling the problem. As you can see from 
the Advantech drivers for example, the card supplier end can pose difficult Bar 
allocation logic, which I would think is best isolated within the particular 
driver model C file. Trying to come up with mechanisms to handle these could 
prove difficult.

Also keep in mind there should be more driver support added in future, which 
could pose other driver model specific difficulties that are best isolated to 
their own C files.

Having said all that, there might be a nice way of merging them all generically 
which I fail to see currently.



Best regards,
Deniz

Sent from my iPhone

Deniz Eren
+61 400 307 762

> On 25 Jan 2018, at 7:24 pm, Pavel Pisa <address@hidden> wrote:
> 
> Hello Philippe,
> 
>> On Wednesday 24 of January 2018 22:41:16 Philippe Mathieu-Daudé wrote:
>> Hi Pavel,
>>> I have seen that a few other type_init-s do more
>>> than simple sequence of type_register_static().
>>> Is it acceptable to use type_init for registration
>>> to CAN core by function call for now? Conversion simplifies
>>> makefiles and unnecessary stub file is removed.
>>> 
>>> But I would use attribute if that solution is preferred because
>>> it is allways present on Linux where SocketCAN is used anyway
>>> and it is used in other Qemu subsystems as well.
>> 
>> using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't
>> work?
> 
> If that is preferred then I implement the stub this way.
> As for the location, can I add
> 
>  stub-obj-y += can_host_stub.o
> 
> into hw/can/Makefile.objs same as it is in crypto/Makefile.objs
> to keep CAN stuff together at least for now or it should go
> to stubs directory?
> 
>  stub-obj-$(CONFIG_CAN_BUS) += can_host_variant.o
> 
> As for connection to host, again I have weak preference
> to keep it in hw/can to keep CAN related code together
> but if you think that it should go t chardev, I would prepare
> patch that way
> 
>  chardev/can-socketcan.c
> 
> As for the rest of the remarks
> 
> You are right that there is some code duplication
> in the SJA1000 CAN PCI cards support but problem
> is that KVASER single uses S5920 PCI local bus bridge
> which requires additional BARs and additional bridge
> specific interrupt enable support. There are more KVASER
> variants which combine multiple SJA1000 in a single BAR.
> pcm3680 and mioe3680 have different BAR structure,
> each SJA1000 uses one BAR. The first uses I/O mapped
> SJA1000 and another memory mapped with stride 4.
> Yes, it all can be combined into one C file.
> But it would require to to add more more fields
> into CardX_PCIState structure and some mechanisms
> and code to select right combination of the BARs,
> handlers etc for each card. It all can be done,
> but I am not sure if I find time for such changes now.
> I expect to have time again in summer when my teaching
> semester ends.
> Another disadvantage is that if somebody else wants
> to implement other card emulation then actual simple
> can_kvaser_pci.c is easily readable. Code gets much
> more complex with all variants selection logic and
> we have already abandoned that simple PCI example
> (can_pci.c) with dummy PCI ID which as been included
> in the past.
> 
> If the code duplication is a problem for now then
> I vote to include only can_kvaser_pci.c for now.
> But Deniz Eren would be sad because he uses the
> cards (which he has contributed) in his test environment.
> 
> Anyway, I would follow what is proposed.
> 
> Thanks for your review and time,
> 
> Pavel