Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()

From: "Michel Dänzer" <michel@daenzer.net>
To: Alex Deucher <alexdeucher@gmail.com>
Cc: dri-devel@lists.freedesktop.org,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH] drm/radeon: fix endian bugs in radeon_atom_get_clock_dividers()
Date: Mon, 22 Apr 2013 17:29:48 +0200	[thread overview]
Message-ID: <1366644588.4530.182.camel@thor.local> (raw)
In-Reply-To: <CADnq5_PZCPtCk48L1BGqrAY-0vGVoG-jkZN-MgTw5Z6LhCd1Ag@mail.gmail.com>

On Mon, 2013-04-22 at 10:18 -0400, Alex Deucher wrote: 
> On Mon, Apr 22, 2013 at 10:08 AM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
> > On Mon, Apr 22, 2013 at 10:03:13AM -0400, alexdeucher@gmail.com wrote:
> >> From: Alex Deucher <alexander.deucher@amd.com>
> >>
> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >> ---
> >>  drivers/gpu/drm/radeon/atombios.h        |    2 ++
> >>  drivers/gpu/drm/radeon/radeon_atombios.c |    6 ++----
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h
> >> index 4b04ba3..de678dd 100644
> >> --- a/drivers/gpu/drm/radeon/atombios.h
> >> +++ b/drivers/gpu/drm/radeon/atombios.h
[...] 
> >> @@ -491,6 +492,7 @@ typedef struct _COMPUTE_MEMORY_ENGINE_PLL_PARAMETERS_V5
> >>    {
> >>      ATOM_COMPUTE_CLOCK_FREQ  ulClock;         //Input Parameter
> >>      ATOM_S_MPLL_FB_DIVIDER   ulFbDiv;         //Output Parameter
> >> +    ULONG ulClockFbDiv;
> >>    };
> >>    UCHAR   ucRefDiv;                           //Output Parameter
> >>    UCHAR   ucPostDiv;                          //Output Parameter

This may just be a nitpick, but the location and name of the new union
member is slightly confusing: It suggests that it's some kind of
combination of the ulClock and ulFbDiv members, when it's just an
alternative representation of ulClock. I'd suggest moving up the new
member, clarifying its name and/or adding a comment explaining what it
is for. 
Looks good to me other than that.

-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel