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

login
register
mail settings
Submitter Viresh KUMAR
Date Feb. 28, 2011, 4:09 a.m.
Message ID <8b8879499520ac641f7e38ac0e40182eb7f32f85.1298866118.git.viresh.kumar@st.com>
Download mbox | patch
Permalink /patch/84722/
State New
Headers show

Comments

Viresh KUMAR - Feb. 28, 2011, 4:09 a.m.
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(-)
Artem Bityutskiy - Feb. 28, 2011, 12:51 p.m.
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?
Viresh KUMAR - March 1, 2011, 3:52 a.m.
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
Viresh KUMAR - March 1, 2011, 4:07 a.m.
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.
Artem Bityutskiy - March 1, 2011, 5:51 a.m.
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?

Patch

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 205b10b..8f56caa 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -226,6 +226,24 @@  struct fsmc_nand_data {
 	void			(*select_chip)(uint32_t bank, uint32_t busw);
 };
 
+/* 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);
+
 /* Assert CS signal based on chipnr */
 static void fsmc_select_chip(struct mtd_info *mtd, int chipnr)
 {
diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
index 6987995..d071db4 100644
--- a/include/linux/mtd/fsmc.h
+++ b/include/linux/mtd/fsmc.h
@@ -160,4 +160,8 @@  extern void __init fsmc_init_board_info(struct platform_device *pdev,
 		struct mtd_partition *partitions, unsigned int nr_partitions,
 		unsigned int width);
 
+void fsmc_nand_set_plat_data(struct platform_device *pdev,
+		struct mtd_partition *partitions, unsigned int nr_partitions,
+		unsigned int options, unsigned int width);
+
 #endif /* __MTD_FSMC_H */