Re: [PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/10/20 10:50 AM, Igor Mammedov wrote:
> On Sat,  8 Feb 2020 17:56:40 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> We want to have a common class_init(). The only value that
>> matters (and changes) is the board revision.
>> Pass the board_rev as class_data to class_init().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/arm/raspi.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 62b8df3c2e..fbfcd29732 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
>>       /*< private >*/
>>       MachineClass parent_obj;
>>       /*< public >*/
>> +    uint32_t board_rev;
>>   } RaspiMachineClass;
>>   
>>   #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
>> @@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>>       arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
>>   }
>>   
>> -static void raspi_init(MachineState *machine, uint32_t board_rev)
>> +static void raspi_init(MachineState *machine)
>>   {
>> +    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
>>       RaspiMachineState *s = RASPI_MACHINE(machine);
>> +    uint32_t board_rev = mc->board_rev;
>>       int version = board_version(board_rev);
>>       uint64_t ram_size = board_ram_size(board_rev);
>>       uint32_t vcram_size;
>> @@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
>>   
>>   static void raspi2_init(MachineState *machine)
>>   {
>> -    raspi_init(machine, 0xa21041);
>> +    raspi_init(machine);
>>   }
>>   
>>   static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
>> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>>   
>> +    rmc->board_rev = board_rev;
> 
> instead of doing a bit obscure ".class_data     = (void *)0xa21041," and
> using it here, I'd just do
> 
>         rmc->board_rev = 0xa21041;
> 
> using value specific for each leaf class

Leaf classes are removed in patch #12 "Use a unique 
raspi_machine_class_init() method", see more uses of .class_data from v2:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg677164.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg677166.html

Are you disagreeing with them? Then we should document .class_data as 
deprecated and show example of good code.

> with this change
>   Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
>>       mc->desc = "Raspberry Pi 2B";
>>       mc->init = raspi2_init;
>>       mc->block_default_type = IF_SD;
>> @@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>>   #ifdef TARGET_AARCH64
>>   static void raspi3_init(MachineState *machine)
>>   {
>> -    raspi_init(machine, 0xa02082);
>> +    raspi_init(machine);
>>   }
>>   
>>   static void raspi3_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
>> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>>   
>> +    rmc->board_rev = board_rev;
>>       mc->desc = "Raspberry Pi 3B";
>>       mc->init = raspi3_init;
>>       mc->block_default_type = IF_SD;
>> @@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
>>           .name           = MACHINE_TYPE_NAME("raspi2"),
>>           .parent         = TYPE_RASPI_MACHINE,
>>           .class_init     = raspi2_machine_class_init,
>> +        .class_data     = (void *)0xa21041,
>>   #ifdef TARGET_AARCH64
>>       }, {
>>           .name           = MACHINE_TYPE_NAME("raspi3"),
>>           .parent         = TYPE_RASPI_MACHINE,
>>           .class_init     = raspi3_machine_class_init,
>> +        .class_data     = (void *)0xa02082,
>>   #endif
>>       }, {
>>           .name           = TYPE_RASPI_MACHINE,
>