On Tue, Feb 2, 2016 at 7:17 AM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 19 January 2016 at 07:23, Alistair Francis <alistai...@gmail.com> wrote: >> Add the STM32F2xx ADC device. This device randomly >> generates values on each read. >> >> This also includes creating a hw/adc directory. >> >> Signed-off-by: Alistair Francis <alist...@alistair23.me> > >> +static uint32_t stm32f2xx_adc_generate_value(STM32F2XXADCState *s) >> +{ >> + /* Attempts to fake some ADC values */ >> +#ifdef RAND_AVALIABLE >> + s->adc_dr = s->adc_dr + rand(); >> +#else >> + s->adc_dr = s->adc_dr + 7; >> +#endif > > We shouldn't be using rand() in devices I think. (Among other things > it means we won't be deterministic, which will break record-replay.) > > In any case you've typoed your #ifdef constant name, which means > that code is never used :-)
Woops, I didn't realise that. I'll take the rand() function out then. I have made all of the other changes as well. Thanks, Alistair > >> +static uint64_t stm32f2xx_adc_read(void *opaque, hwaddr addr, >> + unsigned int size) >> +{ >> + STM32F2XXADCState *s = opaque; >> + >> + DB_PRINT("Address: 0x%"HWADDR_PRIx"\n", addr); > > Spaces around the HWADDR_PRIx would be nice. > >> + >> + if (addr >= ADC_COMMON_ADDRESS) { >> + qemu_log_mask(LOG_UNIMP, >> + "%s: ADC Common Register Unsupported\n", __func__); >> + } >> + >> + switch (addr) { >> + case ADC_SR: >> + return s->adc_sr; >> + case ADC_CR1: >> + return s->adc_cr1; >> + case ADC_CR2: >> + return s->adc_cr2 & 0xFFFFFFF; >> + case ADC_SMPR1: >> + return s->adc_smpr1; >> + case ADC_SMPR2: >> + return s->adc_smpr2; >> + case ADC_JOFR1: >> + case ADC_JOFR2: >> + case ADC_JOFR3: >> + case ADC_JOFR4: >> + qemu_log_mask(LOG_UNIMP, "%s: " \ >> + "Injection ADC is not implemented, the registers are >> " \ >> + "included for compatability\n", __func__); > > "compatibility" > >> + return s->adc_jofr[(addr - ADC_JOFR1) / 4]; >> + case ADC_HTR: >> + return s->adc_htr; >> + case ADC_LTR: >> + return s->adc_ltr; >> + case ADC_SQR1: >> + return s->adc_sqr1; >> + case ADC_SQR2: >> + return s->adc_sqr2; >> + case ADC_SQR3: >> + return s->adc_sqr3; >> + case ADC_JSQR: >> + qemu_log_mask(LOG_UNIMP, "%s: " \ >> + "Injection ADC is not implemented, the registers are >> " \ >> + "included for compatability\n", __func__); >> + return s->adc_jsqr; >> + case ADC_JDR1: >> + case ADC_JDR2: >> + case ADC_JDR3: >> + case ADC_JDR4: >> + qemu_log_mask(LOG_UNIMP, "%s: " \ >> + "Injection ADC is not implemented, the registers are >> " \ >> + "included for compatability\n", __func__); >> + return s->adc_jdr[(addr - ADC_JDR1) / 4] - >> + s->adc_jofr[(addr - ADC_JDR1) / 4]; >> + case ADC_DR: >> + if ((s->adc_cr2 & ADC_CR2_ADON) && (s->adc_cr2 & ADC_CR2_SWSTART)) { >> + s->adc_cr2 ^= ADC_CR2_SWSTART; >> + return stm32f2xx_adc_generate_value(s); >> + } else { >> + return 0x00000000; > > Just "0" seems more readable to me. > >> +#ifdef RAND_MAX >> +/* The rand() function is avaliable */ >> +#define RAND_AVAILABLE >> +#undef RAND_MAX >> +#define RAND_MAX 0xFF >> +#endif /* RAND_MAX */ > > What platforms don't have rand()? > If we need an "exists everywhere" random number function > then there is one in glib. > > (but as noted earlier I don't think we should be using rand() here) > >> + >> +typedef struct { >> + /* <private> */ >> + SysBusDevice parent_obj; >> + >> + /* <public> */ >> + MemoryRegion mmio; >> + >> + uint32_t adc_sr; >> + uint32_t adc_cr1; >> + uint32_t adc_cr2; >> + uint32_t adc_smpr1; >> + uint32_t adc_smpr2; >> + uint32_t adc_jofr[4]; >> + uint32_t adc_htr; >> + uint32_t adc_ltr; >> + uint32_t adc_sqr1; >> + uint32_t adc_sqr2; >> + uint32_t adc_sqr3; >> + uint32_t adc_jsqr; >> + uint32_t adc_jdr[4]; >> + uint32_t adc_dr; >> + >> + qemu_irq irq; >> +} STM32F2XXADCState; >> + >> +#endif /* HW_STM32F2XX_ADC_H */ > > You need to implement the VMState structure for migration. > > thanks > -- PMM