linux-kernel - Re: [PATCH 2/3] Platform changes for UCC TDM driver for MPC8323ERDB.Also includes related QE changes and dts entries.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20080124173423.6f71f016.sfr@canb.auug.org.au>
Date:	Thu, 24 Jan 2008 17:34:23 +1100
From:	Stephen Rothwell <sfr@...b.auug.org.au>
To:	Poonam_Aggrwal-b10812 <b10812@...escale.com>
Cc:	kumar.gala@...escale.com, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	rubini@...ion.unipv.it, linuxppc-dev@...abs.org,
	michael.barkowski@...escale.com, rich.cutler@...escale.com,
	timur@...escale.com, ashish.kalra@...escale.com
Subject: Re: [PATCH 2/3] Platform changes for UCC TDM driver for
 MPC8323ERDB.Also includes related QE changes and dts entries.

This patch needs to come before the previous one ("UCC TDM driver for
QE based MPC83xx platforms") as that uses some of the fields defined here.

On Thu, 24 Jan 2008 10:19:44 +0530 (IST) Poonam_Aggrwal-b10812 <b10812@...escale.com> wrote:
>
> +u32 get_brg_clk(enum qe_clock brgclk, enum qe_clock *brg_source)
>  {

> -	struct device_node *qe;
> -	if (brg_clk)
> -		return brg_clk;
> +	struct device_node *qe, *brg, *clocks;
> +	enum qe_clock brg_src;
> +	u32 brg_input_freq = 0;
> +	u32 brg_num;
> +	int ret;
> +	const unsigned int *prop;
>  
> -	qe = of_find_node_by_type(NULL, "qe");
> -	if (qe) {
> +	*brg_source = 0;
> +
> +	brg_num = brgclk - QE_BRG1;
> +	brg = of_find_compatible_node(NULL, NULL, "fsl,cpm-brg");
> +	if (brg) {

If you did
	if (!brg) {
		.
		.
		goto err;
	}
Then you would save indenting all the rest of this function.

> +		prop = of_get_property(brg,
> +					"fsl,brg-sources", &size);

Join these lines.

> +		of_node_put(brg);
> +
> +		if (prop)
> +			brg_src = *(prop + brg_num);
> +		else {
> +			printk(KERN_ERR "%s: invalid fsl,brg-sources in device "
> +				"tree\n", __FUNCTION__);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +		if (brg_src == 0) {
> +			*brg_source = 0;
> +			if (brg_clk > 0)
> +				return brg_clk;
> +			qe = of_find_node_by_type(NULL, "qe");
> +			if (qe) {

Again testing (!qe) and jumping to err would save another level if
indentation.

> +				unsigned int size;
> +				prop = of_get_property
> +						(qe, "brg-frequency", &size);

And you wouldn't have to split things like this.

> +				if (!prop) {
> +					printk(KERN_ERR "%s: QE brg-frequency"
> +						"not present in device tree\n",
> +						 __FUNCTION__);
> +					ret = -EINVAL;
> +					of_node_put(qe);
> +					goto err;
> +				}
> +				if (*prop) {
> +					of_node_put(qe);
> +					brg_clk = *prop;
> +					return *prop;
> +				} else {

This else (and indentation) is unnecessary as you just returned above.

> +		} else {
> +			*brg_source = brg_src + QE_CLK1 - 1;
> +			clocks = of_find_compatible_node(NULL, NULL,
> +							"fsl,cpm-clocks");
> +			if (!clocks) {
> +				printk(KERN_ERR "%s: no clocks node in device"
> +					" tree \n", __FUNCTION__);
> +				ret = -EINVAL;
> +				goto err;
> +			} else {

Same here.

> +	} else {
> +		printk(KERN_ERR "%s: no brg node in device tree\n",
> +			 __FUNCTION__);
> +		ret = -EINVAL;
> +		goto err;

This goto is redundant.

> +	}
> +err: return ret;

Put the label on a line by itself and indent it one space (that means that
"diff -p will reference the funstion anem instead of the label).

> @@ -152,6 +152,10 @@ struct ucc_fast_info {
>  	enum ucc_fast_rx_decoding_method renc;
>  	enum ucc_fast_transparent_tcrc tcrc;
>  	enum ucc_fast_sync_len synl;
> +	char *tdm_rx_clk;
> +	char *tdm_tx_clk;
> +	char *tdm_rx_sync;
> +	char *tdm_tx_sync;

If you make these "const char *" you won't have to cast the results of
of_get_property() that you assign to them.

-- 
Cheers,
Stephen Rothwell                    sfr@...b.auug.org.au
http://www.canb.auug.org.au/~sfr/

Content of type "application/pgp-signature" skipped