Re: [Qemu-devel] [PATCH 2/8] [MIPS] qdev: convert ds1225y nvram to sysbus device

[Markus Armbruster]

Hervé Poussineau <address@hidden> writes:

> Use it in Jazz emulation
> Remove protection stuff, which doesn't belong to this device
> Remove ds1225y_init() and ds1225y_set_protection() functions, which are not 
> used anymore
>
> Signed-off-by: Hervé Poussineau <address@hidden>

I think this would be easier to review if you split it up some, perhaps
cleanup such as DPRINTF(), removal of protection stuff, and qdev
conversion.

> ---
>  hw/ds1225y.c   |  151 
> ++++++++++++++++++++++++++++++--------------------------
>  hw/mips.h      |    4 --
>  hw/mips_jazz.c |    4 +-
>  3 files changed, 83 insertions(+), 76 deletions(-)
>
> diff --git a/hw/ds1225y.c b/hw/ds1225y.c
> index 009d127..046d1ec 100644
> --- a/hw/ds1225y.c
> +++ b/hw/ds1225y.c
> @@ -22,31 +22,34 @@
>   * THE SOFTWARE.
>   */
>  
> -#include "hw.h"
> -#include "mips.h"
> -#include "nvram.h"
> +#include "sysbus.h"
>  
>  //#define DEBUG_NVRAM
>  
> -typedef struct ds1225y_t
> -{
> -    uint32_t chip_size;
> +#ifdef DEBUG_NVRAM
> +#define DPRINTF(fmt, ...)                                       \
> +    do { printf("nvram: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +typedef struct {
> +    DeviceState qdev;
> +    int32_t chip_size;

You change chip_size from unsigned to signed.  Why?

> +    char* filename;
>      QEMUFile *file;
>      uint8_t *contents;
> -    uint8_t protection;
> -} ds1225y_t;
> -
> +} NvRamState;
[...]