On 8 December 2015 at 04:00, Michel Dänzer <michel@xxxxxxxxxxx> wrote:
> 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.
I agree, having cpu_to_le32 repeated over and over makes code messy,
harder to read, harder to fit 80 chars (which is already often
violated in radeon code). radeon_ring_write_le sounds self
explanatory.
--
Rafał
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
