Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts

[BALATON Zoltan <balaton@eik.bme.hu>]

On Thu, 14 Oct 2021, BALATON Zoltan wrote:
> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>> On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
>>> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>>>> On Wed, Oct 13, 2021 at 02:13:09PM +0200, BALATON Zoltan wrote:
>>>>> This device is part of a superio/ISA bridge chip and IRQs from it are
>>>>> routed to an ISA interrupt set by the Interrupt Line PCI config
>>>>> register. Change uhci_update_irq() to allow this and implement it in
>>>>> vt82c686-uhci-pci.
>>>> 
>>>> Looks good.  There are some unrelated changes in though (whitespace,
>>>> comments, ...), and the vt82c686-uhci-pci.c changes should be a
>>>> separate patch.
>>> 
>>> So you mean split it into a series of three small patches? Should I do a 
>>> w4
>>> with that?
>> 
>> I was thinking about two patches: drop the unrelated stuff, one patch
>> for the irq signaling change, and one for the vt82c686 changes.
>> 
>> But of course you can add more patches for the other changes, i.e.
>> dropping the else branch for level = 0 and other small tweaks.
>
> The tewak for the comment is needed for checkpach, dropping the else is just 
> to make the function shorter and IMO more readable. I can do this in a 
> previous patch so checkpatch won't complain on the pci irq change or need at 
> least the comment change in that patch. I'm thinking about an alternative way 
> for the vt82c686 part to avoid using isa_get_irq which has a comment saying 
> it should be dropped but not sure about that yet, I'll try it and submit a v4 
> with at least breaking it up to smaller patches.

I've tried it and it's just not worth it. By moving isa reference to 
vt82c686.c we end up doing the same, just with a lot more code changes 
(which is harder to understand) and less efficiently (at every interrupt 
need to jump through hoops and do some QOM casts and additional calls to 
reach the same effect). These are basically modelling functions of the 
same device just split up in parts to categorise and reuse parts of 
similar models so accesing isa parts of the chip from usb part does not 
seem to be that bad and it's simple this way. So I ended up just splitting 
the patch for v4.

Thanks,
BALATON Zoltan