Re: [Qemu-trivial] [Qemu-devel] [PATCH v2 13/14] sm501: Add reset function and vmstate descriptor

[BALATON Zoltan]

On Thu, 2 Mar 2017, BALATON Zoltan wrote:
> On Thu, 2 Mar 2017, Peter Maydell wrote:
> > On 2 March 2017 at 20:18, BALATON Zoltan <address@hidden> wrote:
> > > On Thu, 2 Mar 2017, Peter Maydell wrote:
> > > > Don't use qemu_register_reset(). Set the appropriate dc->reset
> > > > function pointers instead.
> > >
> > >
> > > Any reason for that? This way I could save two more boilerplate functions
> > > because I could define reset function once, otherwise I'd need two 
> > > versions
> > > taking sysbus and pci states just to extract the SM501State function and
> > > call this function. Do you still think I should do that instead?
> >
> > qemu_register_reset is a pre-QOM method for doing reset;
> > the standard QOM way of saying "my device has some reset
> > behaviour" is to set its reset method pointer.
> > Code calling qemu_register_reset() is generally either (a) doing
> > something kind of weird or (b) old device code that hasn't yet
> > been converted to QOM.
>
> Hmm, does having a device with both sysbus and pci versions qualify as weird? 
> Does adding a comment saying that this way we don't need two more reset 
> functions just to call the registered one justifies using 
> qemu_register_reset() or this function is deprecated and should not be used 
> and should go with the separate reset functions instead?

On second thought I may go with the separate reset functions because then 
I can also update the bus type in the misc_control (not that anything I 
know depends on it but we can get it right).

> > > So should it be 4 patches: reset for pci, reset for sysbus, vmstate for 
> > > pci,
> > > vmstate for sysbus or 2 patches: reset for both, vmstate for both?
> >
> > I think I'd go with 2 patches, since the two are going to
> > share the bulk of the implementation in each case.
>
> OK, thanks for the explanation in previous reply as well, I'll do this then.

So I may also add the reset function already in the QOMify and Add PCI 
version patches then only one more patch is needed for the vmstate.