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

[Alistair Francis]

On Tue, Feb 2, 2016 at 7:17 AM, Peter Maydell <address@hidden> wrote:
> 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 :-)

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