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 03/01/2011 09:22 AM, viresh kumar wrote:
> 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.
> 

Ok. I think one thing can be done. We can define a routine in device.h
static inline void *dev_set_platdata(const struct device *dev, void *pdata);

Now, we can define plat_data in boards file and then call this routine.

-- 
viresh