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

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.

This routine will be used in SPEAr fsmc patches which are sent in a different
patchset.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c |   18 ++++++++++++++++++
 include/linux/mtd/fsmc.h     |    4 ++++
 2 files changed, 22 insertions(+), 0 deletions(-)

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 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?

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

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.

On Tue, 2011-03-01 at 09:22 +0530, viresh kumar wrote:
> 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.

Not sure why it will be not clean enough. Just add it to your board
files, this is few lines of code after all.

if (partitions) {
	plat_data->partitions = partitions;
	plat_data->nr_partitions = nr_partitions;
}
plat_data->options = options;
plat_data->width = width;

How many board files you have?

>  so we thought of creating this function which can
> simply be reused by all board files.

OK, but I think you should try to find some other place instead of
putting it to the driver. What is the fundamental reason to put this
code to the driver? Why it belongs to the driver?