Re: [Qemu-devel] [PATCH v3 3/7] STM32F2xx: Add the ADC device

[Peter Maydell]

On 19 January 2016 at 07:23, Alistair Francis <address@hidden> 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 <address@hidden>

> +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 :-)

> +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