On 19 February 2018 at 04:03, Michael Davidsaver <mdavidsa...@gmail.com> wrote: > Support for: ds1307, ds1337, ds1338, ds1339, > ds1340, ds1375, ds1388, and ds3231. > > Tested with ds1338 and ds1375. > > The existing ds1338 model has two bugs > with almost no practical impact. > > 1. Trying to set time in 12-hour mode works, > but the 12 hour mode bit isn't stored. > So time always reads in 24 hour mode. > > 2. wday_offset is always stored for the > local time zone. When the RTC is set > and rtc_utc=1 and the local timezone > has a different day than UTC, then > wday_offset will be off by one. > > Signed-off-by: Michael Davidsaver <mdavidsa...@gmail.com> > --- > default-configs/arm-softmmu.mak | 2 +- > hw/timer/Makefile.objs | 2 +- > hw/timer/ds-rtc-i2c.c | 466 > ++++++++++++++++++++++++++++++++++++++++ > hw/timer/ds1338.c | 248 ---------------------
This patch is really hard for me to read because it's basically "throw away the old file and implement a new one". Can you split it up a bit please? I'd recommend starting with "just a plain file rename" and then piecemeal changes of behaviour. Then it's easy to see what has been changed. > --- /dev/null > +++ b/hw/timer/ds-rtc-i2c.c > @@ -0,0 +1,466 @@ > +/* Emulation of various Dallas/Maxim RTCs accessed via I2C bus > + * > + * Copyright (c) 2017 Michael Davidsaver > + * Copyright (c) 2009 CodeSourcery > + * > + * Authors: Michael Davidsaver > + * Paul Brook > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the LICENSE file in the top-level directory. This has lost the "contributions after 2012 are GPL-2-or-later" part of the licensing information. > +#ifdef DEBUG_DSRTC > +#define DPRINTK(FMT, ...) info_report(TYPE_DSRTC " : " FMT, ## __VA_ARGS__) > +#else > +#define DPRINTK(FMT, ...) do {} while (0) > +#endif If we're overhauling the device it might be nice to convert it to tracepoints, but I don't insist on that. > +#define TYPE_DSRTC "ds-rtc-i2c" > +#define DSRTC(obj) OBJECT_CHECK(DSRTCState, (obj), TYPE_DSRTC) > +#define DSRTC_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(DSRTCClass, obj, TYPE_DSRTC) > +#define DSRTC_CLASS(klass) \ > + OBJECT_CLASS_CHECK(DSRTCClass, klass, TYPE_DSRTC) > + > +static const VMStateDescription vmstate_dsrtc = { > + .name = TYPE_DSRTC, > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_I2C_SLAVE(parent_obj, DSRTCState), > + VMSTATE_INT64(time_offset, DSRTCState), > + VMSTATE_INT8_V(wday_offset, DSRTCState, 2), > + VMSTATE_UINT8_ARRAY(regs, DSRTCState, DSRTC_REGSIZE), > + VMSTATE_UINT8(addr, DSRTCState), > + VMSTATE_BOOL(addrd, DSRTCState), > + VMSTATE_END_OF_LIST() > + } > +}; Changing the VMState name and fields will break cross-version migration from old QEMU versions of all the boards which use a DS1338. I'd strongly prefer to avoid that. There are ways to do that, like subsections and pre/post-load functions, but a big part of it is not changing fields if you don't really need to. > + > +static void dsrtc_reset(DeviceState *device); > + > +/* update current time registers */ > +static Can you not put newlines after the 'static' keyword, please? > +void dsrtc_latch(DSRTCState *ds) > +{ thanks -- PMM