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

[Stephen Rothwell <sfr@canb.auug.org.au>]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

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@freescale.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@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]