[PATCH V5 36/63] ST SPEAr : EMI (Extrenal Memory Interface) controller driver
Vipin Kumar
vipin.kumar at st.com
Mon Feb 21 01:26:10 EST 2011
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:
>> From: Vipin Kumar <vipin.kumar at st.com>
>
>
> Subject: [PATCH V5 36/63] ST SPEAr : EMI (Extrenal Memory Interface)
> controller driver
>
> "External"
>
Sorry!!
>> + 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....
>> +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
More information about the linux-arm-kernel
mailing list