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

[Richard Henderson <richard.henderson@linaro.org>]

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.


r~