Re: [PATCH V2 resend] fsmc-nand: Add fsmc_nand_set_plat_data in drivers/mtd/nand/fsmc_nand.c

[viresh kumar <viresh.kumar@st.com>]

On 02/28/2011 06:21 PM, Artem Bityutskiy wrote:
> On Mon, 2011-02-28 at 09:39 +0530, Viresh Kumar wrote:
>> In most of the cases partitions info, width, etc comes from board files. And
>> device structure may be defined in machine files, common to all board files.
>> Thus, we need to set platform data from board file, for which
>> fsmc_nand_set_plat_data routine is required.
> 
> Hi, sorry, but after looking a bit closer I do not see why you need this
> function.
> 
> Why could not you set up the width, options, and partitions in the
> straight in board file, e.g., like arch/arm/mach-omap2/board-igep0030.c
> sets partitions for onenand driver? Also, for partitions you can use
> command line arguments, like this is done in
> drivers/mtd/onenand/generic.c
>  

This is what i explained in the commit message also. We don't declare device
structures in board files, as this information is machine dependent, so this is present
in common machine file to all boards.
Now we have to set platform data. This can be be done in board_init() routine in
the board specific file. But then this routine will contain below mentioned code,
and so will not look clean enough. so we thought of creating this function which can
simply be reused by all board files.

>> +/* This function is used to set platform data field of pdev->dev */
>> +void fsmc_nand_set_plat_data(struct platform_device *pdev,
>> +		struct mtd_partition *partitions, unsigned int nr_partitions,
>> +		unsigned int options, unsigned int width)
>> +{
>> +	struct fsmc_nand_platform_data *plat_data;
>> +	plat_data = dev_get_platdata(&pdev->dev);
>> +
>> +	if (partitions) {
>> +		plat_data->partitions = partitions;
>> +		plat_data->nr_partitions = nr_partitions;
>> +	}
>> +
>> +	plat_data->options = options;
>> +	plat_data->width = width;
>> +}
>> +EXPORT_SYMBOL_GPL(fsmc_nand_set_plat_data);
> 
> Just looks a bit too much to add a function which simply assigns
> parameters and then export it. If you'll need to initialize more
> parameters later, will you add more arguments there?
> 

Almost all plat_data fields are filled here. But yes your question
still stands valid. Obviously we must not have a large number of parameters
here.

--
viresh