[PATCH 1/3] staging: sm750fb: merge calcPLL and getPllValue into getChipClock

Mike Rapoport mike.rapoport at gmail.com
Thu Oct 8 20:22:01 UTC 2015


On Thu, Oct 08, 2015 at 12:58:04PM +0530, Sudip Mukherjee wrote:
> On Tue, Oct 06, 2015 at 09:42:15AM +0100, Mike Rapoport wrote:
> > The getChipClock function is used only to get MXCLK frequency, which
> > makes most of getPllValue function unused and thus. The detection of
> > MXCLK frequency may be implemented directly in getChipClock rendering
> > getPllValue and calcPLL unused.
> > 
> > Signed-off-by: Mike Rapoport <mike.rapoport at gmail.com>
> > ---
> 
> You have also missed saving the values in pPLL->inputFreq and
> pPLL->clockType = clockType. Both have been used later.

the pPLL pointer that is passed to getPllValue points to a local
variable in getChipClock. If we merge these funcions, the variable and
the pointer are not needed. 
 
> Is it ok to remove the PLL calculation of all the different type of
> display devices? Currently only PANEL_PLL_CTRL is being used for
> calculation and ofcourse we also donot keep in kernel that we donot use.

I think we can remove the unused code and then generalize the clock
calculations if anybody will ever need it.
 
> Maybe we can do some thing like:
> 
> diff --git a/drivers/staging/sm750fb/ddk750_chip.c b/drivers/staging/sm750fb/ddk750_chip.c
> index ad2f1c0..4b60894 100644
> --- a/drivers/staging/sm750fb/ddk750_chip.c
> +++ b/drivers/staging/sm750fb/ddk750_chip.c
> @@ -39,11 +39,17 @@ static unsigned int getChipClock(void)
>  {
>  	unsigned int pll_reg;
>  	unsigned int M, N, OD, POD;
> +	clock_type_t clk_type = PANEL_PLL_CTRL;
> +	/*
> +	 * Different register location based on display device type:
> +	 * MXCLK_PLL_CTRL, PANEL_PLL_CTRL, CRT_PLL_CTRL, VGA_PLL0_CTRL,
> +	 * VGA_PLL1_CTRL
> +	 */
>  
>  	if (getChipType() == SM750LE)
>  		return MHz(130);
>  
> -	pll_reg = PEEK32(MXCLK_PLL_CTRL);
> +	pll_reg = PEEK32(clk_type);
>  	M = FIELD_GET(pll_reg, PANEL_PLL_CTRL, M);
>  	N = FIELD_GET(pll_reg, PANEL_PLL_CTRL, N);
>  	OD = FIELD_GET(pll_reg, PANEL_PLL_CTRL, OD);
> 
> ---
> 
> This is on top of your patch. So now we have the different possible
> values in the comments, so whenever, if anyone is working on a different
> device atleast they will not have to search and find out what the other
> values can be.
> 
> regards
> sudip


More information about the devel mailing list