Re: [PATCH v2 03/11] goldfish_rtc: Add endianness property

[Laurent Vivier <lvivier@redhat.com>]

On 04/07/2022 12:21, Richard Henderson wrote:
> On 7/4/22 15:46, Laurent Vivier wrote:
>> On 04/07/2022 11:59, Richard Henderson wrote:
>>> On 7/4/22 02:58, Stafford Horne wrote:
>>>> -static const MemoryRegionOps goldfish_rtc_ops = {
>>>> -    .read = goldfish_rtc_read,
>>>> -    .write = goldfish_rtc_write,
>>>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>>>> -    .valid = {
>>>> -        .min_access_size = 4,
>>>> -        .max_access_size = 4
>>>> -    }
>>>> +static const MemoryRegionOps goldfish_rtc_ops[3] = {
>>>> +    [DEVICE_NATIVE_ENDIAN] = {
>>>> +        .read = goldfish_rtc_read,
>>>> +        .write = goldfish_rtc_write,
>>>> +        .endianness = DEVICE_NATIVE_ENDIAN,
>>>> +        .valid = {
>>>> +            .min_access_size = 4,
>>>> +            .max_access_size = 4
>>>> +        }
>>>> +    },
>>>> +    [DEVICE_LITTLE_ENDIAN] = {
>>>> +        .read = goldfish_rtc_read,
>>>> +        .write = goldfish_rtc_write,
>>>> +        .endianness = DEVICE_LITTLE_ENDIAN,
>>>> +        .valid = {
>>>> +            .min_access_size = 4,
>>>> +            .max_access_size = 4
>>>> +        }
>>>> +    },
>>>> +    [DEVICE_BIG_ENDIAN] = {
>>>> +        .read = goldfish_rtc_read,
>>>> +        .write = goldfish_rtc_write,
>>>> +        .endianness = DEVICE_BIG_ENDIAN,
>>>> +        .valid = {
>>>> +            .min_access_size = 4,
>>>> +            .max_access_size = 4
>>>> +        }
>>>> +    },
>>>>   };
>>>
>>> You don't need 3 copies, only big and little.
>>>
>>>> +static Property goldfish_rtc_properties[] = {
>>>> +    DEFINE_PROP_UINT8("endianness", GoldfishRTCState, endianness,
>>>> +                      DEVICE_NATIVE_ENDIAN),
>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>
>>> ... and I think the clear desire for default is little-endian.  I would make the 
>>> property be bool, and add a comment that this is only for m68k compatibility, so don't 
>>> use it in new code.
>>
>> m68k doesn't really need this.
>>
>> kernel with the m68k virt machine and goldfish device supports "native" mode so I think 
>> it's not needed to add another layer of complexity for it.
> 
> "Another level"?  I'm talking about removing "native", and only having "big" and "little", 
> which is less complexity.

"Less complexity" is to keep only native. I'm not against the change, I'm just saying it's 
not needed by m68k.

Thanks,
Laurent