On 08.12.2015 02:49, Oded Gabbay wrote:
> On Mon, Dec 7, 2015 at 9:51 AM, Michel Dänzer <michel@xxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
