On Wed, Jun 06, 2018 at 07:35:28PM +0200, BALATON Zoltan wrote:
> On Wed, 6 Jun 2018, Philippe Mathieu-Daudé wrote:
> > On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > > Basic emulation of the M41T80 serial (I2C) RTC chip. Only getting time
> > > of day is implemented. Setting time and RTC alarm are not supported.
> [...]
> > > diff --git a/hw/timer/m41t80.c b/hw/timer/m41t80.c
> > > new file mode 100644
> > > index 0000000..9dbdb1b
> > > --- /dev/null
> > > +++ b/hw/timer/m41t80.c
> > > @@ -0,0 +1,117 @@
> > > +/*
> > > + * M41T80 serial rtc emulation
> > > + *
> > > + * Copyright (c) 2018 BALATON Zoltan
> > > + *
> > > + * This work is licensed under the GNU GPL license version 2 or later.
> > > + *
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qemu/log.h"
> > > +#include "qemu/timer.h"
> > > +#include "qemu/bcd.h"
> > > +#include "hw/i2c/i2c.h"
> > > +
> > > +#define TYPE_M41T80 "m41t80"
> > > +#define M41T80(obj) OBJECT_CHECK(M41t80State, (obj), TYPE_M41T80)
> > > +
> > > +typedef struct M41t80State {
> > > + I2CSlave parent_obj;
> > > + int8_t addr;
> > > +} M41t80State;
> > > +
> > > +static void m41t80_realize(DeviceState *dev, Error **errp)
> > > +{
> > > + M41t80State *s = M41T80(dev);
> > > +
> > > + s->addr = -1;
> > > +}
> > > +
> > > +static int m41t80_send(I2CSlave *i2c, uint8_t data)
> > > +{
> > > + M41t80State *s = M41T80(i2c);
> > > +
> > > + if (s->addr < 0) {
> > > + s->addr = data;
> > > + } else {
> > > + s->addr++;
> > > + }
> >
> > What about adding enum i2c_event in M41t80State and use the enum here
> > rather than the addr < 0? Also this wrap at INT8_MAX = 127, is this
> > expected?
>
> Thanks for the review. I guess we could add enum for device bytes and the
> special case -1 meaning no register address selected yet but this is a very
> simple device with only 20 bytes and the datasheet also lists them by number
> without naming them so I think we can also refer to them by number. Since
> the device has only this 20 bytes the case with 127 should also not be a
> problem as that's invalid address anyway. Or did you mean something else?
So, I'm not particularly in favour of adding extra state variables.
But is using addr < 0 safe here? You're assigning the uint8_t data to
addr - could that result in a negative value?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
