From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Thu, 20 Jan 2011 12:52:26 +0100
Subject: [U-Boot] [PATCH V2 01/11] Add support for MX35 processor
In-Reply-To: <4D380C25.20901@denx.de>
References: <1295513194-16158-1-git-send-email-sbabic@denx.de>
	<1295513194-16158-2-git-send-email-sbabic@denx.de>
	<20110120092540.376C3D301D7@gemini.denx.de>
	<4D380C25.20901@denx.de>
Message-ID: <20110120115226.CAF72D301D7@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Stefano Babic,

In message <4D380C25.20901@denx.de> you wrote:
>
> >> +	if (readl(&ccm->pdr3) & MXC_CCM_PDR3_UART_M_U)
> >> +		freq = get_mcu_main_clk();
> >> +	else
> >> +		freq = decode_pll(readl(&ccm->ppctl),
> >> +			CONFIG_MX35_HCLK_FREQ);
> > 
> > Braces needed
> 
> checkpatch (and generally accepted in linux, as I can see) requires that
> single statements must not be surrounded by braces. checkpatch returns a
> warning, explaining that braces are not needed.


> I see in the past some comments requiring to remove braces, but it you
> prefer to add them. IMHO it is better to follow the same codestyle as
> linux, using the same tools as checkpatch. I do not know why we have two
> different results from checkpatch, I try to investigate. I had prefer to

I was running an older version of checkpatch...

> >> +	case USB_CLK:
> >> +		usb_prdf = (reg4 >> 25) & 0x7;
> >> +		usb_podf = (reg4 >> 22) & 0x7;
> >> +		if (reg4 & 0x200)
> >> +			pll = get_mcu_main_clk();
> >> +		else
> >> +			pll = decode_pll(readl(&ccm->ppctl),
> >> +				CONFIG_MX35_HCLK_FREQ);
> > 
> > Ditto. Please fix globally.
> 
> See my previous comment. I would prefer to not have a different rule in
> u-boot, and not go against some provided tool (we use both checkpatch)
> if not strictly required.

Did you try it?

For me, both

+       if (reg4 & 0x200)
+               pll = get_mcu_main_clk();
+       else
+               pll = decode_pll(readl(&ccm->ppctl),
+                       CONFIG_MX35_HCLK_FREQ);

and

+       if (reg4 & 0x200) {
+               pll = get_mcu_main_clk();
+       } else {
+               pll = decode_pll(readl(&ccm->ppctl),
+                       CONFIG_MX35_HCLK_FREQ);
+       }

generate _NO_ warnings with checkpatch.


I feel that when the "single statement" is split across several lines,
eventually even including blank lines (see yesterday's discussion
here), then braces are needed.


> > Indeed they should.  Why don't you autogenerate these, then?
> > 
> > We have all the tools in place, use them.
> 
> I will see how to use them.

See tools/scripts/make-asm-offsets

> > Note: the following remark is a question, NOT a change request:
> > 
> > Would it not be possible to reduce all these terrible lists?  As far
> > as I can tell, the list is built sequentially, with both arguments to
> > _MXC_BUILD_NON_GPIO_PIN() being incremented by 4 for the next
> > register.  This begs for automatic code generation, doesn't it?
> 
> I do not know if it helps. The list follows exactly the description in
> user manual, and, if you can see a rule for MX35_PIN_A*, it is not so
> simply to find one for other pins, specially for the MXC_BUILD_GPIO_PIN.
> At least, the list is at the moment coherent for all i.MX processors
> (ok, ugly for all). The name of the pin cannot be generated, and it is
> the name found in manual. Miore as generated, the list is sorted....

OK, thanks for the explanation.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"You're just jealous." "What, of an overgrown puppy  with  a  single-
figure IQ?"                      - Terry Pratchett, _Moving Pictures_