On 08.12.2015 02:49, Oded Gabbay wrote:
On Mon, Dec 7, 2015 at 9:51 AM, Michel Dänzer michel@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.