[PATCH 2/3] radeon: Fix VCE ring test for Big-Endian systems

[Michel Dänzer]

On 08.12.2015 02:49, Oded Gabbay wrote:
> On Mon, Dec 7, 2015 at 9:51 AM, Michel Dänzer <michel at daenzer.net> wrote:
>> On 05.12.2015 06:09, Oded Gabbay wrote:
>>>
>>> @@ -765,7 +765,7 @@ int radeon_vce_ring_test(struct radeon_device *rdev, struct radeon_ring *ring)
>>>                         ring->idx, r);
>>>               return r;
>>>       }
>>> -     radeon_ring_write(ring, VCE_CMD_END);
>>> +     radeon_ring_write(ring, cpu_to_le32(VCE_CMD_END));
>>>       radeon_ring_unlock_commit(rdev, ring, false);
>>>
>>>       for (i = 0; i < rdev->usec_timeout; i++) {
>>>
>>
>> A new helper function such as
>>
>> static inline void radeon_ring_write_le(struct radeon_ring *ring, uint32_t v)
>> {
>>         radeon_ring_write(ring, cpu_to_le32(v));
>> }
>>
>> might be nice for this.
> 
> IMHO, I don't think this gives any benefit.
> You would just need to replace every:
> 
> radeon_ring_write(ring, cpu_to_le32(SOME_DEFINE));
> 
> with
> 
> radeon_ring_write_le(ring, SOME_DEFINE);
> 
> So no reduce in code size. Also, if you change it in my code, I think
> you need to change it in the entire driver for consistency.
> 
> What's even more important, is that when I look at the above, it seems
> to me this change even makes the code *less* clear as you now need to
> go into radeon_ring_write_le  to actually understand how the value is
> written.

Sprinkling cpu_to_le32 all over the place just seems a bit ugly to me,
but I don't feel strongly about it. I.e. I'm fine with the patch as is,
it was just a suggestion.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer