On 2/19/2011 10:43 PM, Russell King - ARM Linux wrote:

On Thu, Jan 20, 2011 at 12:56:14PM +0530, Viresh Kumar wrote:

quoted

From: Vipin Kumar <redacted>

Subject: [PATCH V5 36/63] ST SPEAr : EMI (Extrenal Memory Interface)
        controller driver

"External"

Sorry!!

quoted

+	clk = clk_get(NULL, "emi");

What is the platform device pointer passed into this function?  It seems
unused - would using &pdev->dev here be appropriate?  Possibly not as it
seems to be the physmap device.

Yes, the platform device pointer is unused
I would remove it....

quoted

+void __init
+emi_init_board_info(struct platform_device *pdev, struct resource *resources,
+		int res_num, struct mtd_partition *partitions,
+		unsigned int nr_partitions, unsigned int width)
+{
+	struct physmap_flash_data *emi_plat_data = dev_get_platdata(&pdev->dev);
+
+	pdev->resource = resources;
+	pdev->num_resources = res_num;
+
+	if (partitions) {
+		emi_plat_data->parts = partitions;
+		emi_plat_data->nr_parts = nr_partitions;
+	}
+
+	emi_plat_data->width = width;
+}

I don't see why this has to be code rather than in the platform specific
files as static initializers.  

The device is instantiated in the machine file and above information comes
from the board file. So we kept them this way.

Also, you seem to be passing stuff like
'EMI_FLASH_WIDTH32' in for 'width', which seems very odd as this is the
byte width.  I don't see why you need #defined constants for this.

It's waiting someone who doesn't realise that it has a proper meaning
to change it to an enum or something like that.

If you think the physmap flash API is lacking some definitions for this,
please talk to the MTD people.

Yes. We shouldn't have created new macro's for this. Will be removed.

vipin