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

On Thu, 24 Jan 2008 10:16:42 +0530 (IST) Poonam_Aggrwal-b10812 [off-list ref] 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@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/