Re: [PATCH] UCC TDM driver for QE based MPC83xx platforms.

[Stephen Rothwell]

On Thu, 24 Jan 2008 10:16:42 +0530 (IST) Poonam_Aggrwal-b10812 <b10812@xxxxxxxxxxxxx> wrote:
>
> +static int ucc_tdm_probe(struct of_device *ofdev,
> +			 const struct of_device_id *match)
> +{
> +	struct device_node *np = ofdev->node;
> +	struct resource res;
> +	const unsigned int *prop;
> +	u32 ucc_num, device_num, err, ret = 0;
> +	struct device_node *np_tmp = NULL;

You don't need to initialise this.

> +	dma_addr_t physaddr;
> +	void *tdm_buff;
> +	struct ucc_tdm_info *ut_info;
> +
> +	prop = of_get_property(np, "device-id", NULL);

You should check for (prop == NULL).

> +	ucc_num = *prop - 1;
> +	if ((ucc_num < 0) || (ucc_num > 7))
> +		return -ENODEV;
> +
> +	ut_info = &utdm_info[ucc_num];
> +	if (ut_info == NULL) {

This cannot be NULL as you have just taken the address of an array
element.

> +	tdm_ctrl[device_num]->ut_info = ut_info;
> +
> +	tdm_ctrl[device_num]->ut_info->uf_info.ucc_num = ucc_num;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is the same as "ut_info".

> +	tdm_ctrl[device_num]->ut_info->uf_info.tdm_tx_clk =
> +			(char *) of_get_property(np, "fsl,tdm-tx-clk", NULL);
                                ^
We don't normall put spaces here.

> +	tdm_ctrl[device_num]->ut_info->uf_info.tdm_rx_clk =
> +			(char *) of_get_property(np, "fsl,tdm-rx-clk", NULL);
                                ^
Ditto. And later as well.

> +	tdm_ctrl[device_num]->ut_info->uf_info.irq =
> +					irq_of_parse_and_map(np, 0);
> +	err = of_address_to_resource(np, 0, &res);
> +	if (err) {
> +		ret = EINVAL;

This should be -EINVAL or err.

> +		goto get_property_error;

You need to do something about unmapping the irq in the error path.

> +	tdm_ctrl[device_num]->uf_regs = of_iomap(np, 0);
> +
> +	np_tmp = of_find_compatible_node(np_tmp, "slic", "legerity-slic");
> +	if (np_tmp != NULL)
> +		tdm_ctrl[device_num]->leg_slic = 1;
> +	else
> +		tdm_ctrl[device_num]->leg_slic = 0;

	of_node_ut(np_tmp);

> +	tdm_buff = dma_alloc_coherent(NULL, 2 * NR_BUFS * SAMPLE_DEPTH *
> +				tdm_ctrl[device_num]->cfg_ctrl.active_num_ts,
> +					&physaddr, GFP_KERNEL);
> +	if (!tdm_buff) {
> +		printk(KERN_ERR "ucc-tdm: could not allocate buffer"
> +					"descriptors\n");
> +		ret = -ENOMEM;
> +		goto get_property_error;

You need to unmap the uf_regs in the error path.

> +get_property_error:
> +	kfree(tdm_ctrl[device_num]);

Do you need to set "tdm_ctrl[device_num]" to NULL and decrement
num_tdm_devices?

> +	return ret;
> +}
> +
> +static int ucc_tdm_remove(struct of_device *ofdev)
> +{
> +	struct tdm_ctrl *tdm_c;
> +	struct ucc_tdm_info *ut_info;
> +	u32 ucc_num;
> +
> +	tdm_c = dev_get_drvdata(&(ofdev->dev));

	dev_set_drvdata(&of_dev->dev, NULL);

> +	ucc_num = tdm_c->ut_info->uf_info.ucc_num;
> +	ut_info = &utdm_info[ucc_num];
> +	tdm_stop(tdm_c);
> +	tdm_deinit(tdm_c);
> +
> +	ucc_fast_free(tdm_c->uf_private);
> +
> +	dma_free_coherent(NULL, 2 * NR_BUFS * SAMPLE_DEPTH *
> +				tdm_c->cfg_ctrl.active_num_ts,
> +				tdm_c->tdm_input_data,
> +				tdm_c->dma_input_addr);
> +

You need to unmap the uf_reg and the irq.

> +static struct of_device_id ucc_tdm_match[] = {

const, please.

> +	{
> +	 .type = "tdm",
> +	 .compatible = "fsl,ucc-tdm",
> +	 }, {},

We euld normall format this like:

	{ .type = "tdm", .compatible = "fsl,ucc-tdm", },
	{},

> +static struct of_platform_driver ucc_tdm_driver = {

	.driver = {

> +	.name = DRV_NAME,

	},

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

Attachment: pgp00000.pgp
Description: PGP signature