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

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

[Viresh Kumar]

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(-)

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 */
-- 
1.7.2.2

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

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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

[viresh kumar]

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

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

[viresh kumar]

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

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

[Artem Bityutskiy]

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?

-- 
Best Regards,
Artem Bityutskiy (Битюцкий Артём)

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

[viresh kumar]

On 03/01/2011 11:21 AM, Artem Bityutskiy wrote:
> 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?

This patch was on setting fsmc_nand_platform_data from board file.
So i thought this might be the best place for doing so. As this can be
reused by others too.

Probably this patch can be dropped now, as i have changed the approach a bit.
device structure will be in machine.c file. plat data will be defined in boards
file and i will introduce following routine, which can be used by board files.
I have already sent this patch to linux-arm-kernel list.


---------------

[RFC] device.h: add device_set_platdata routine

device.h supports device_get_platdata but doesn't support device_set_platdata.
This routine is required by platforms in which device structure is declared
in a machine specific file and platform data comes from board specific file.

This will be used by SPEAr patches sent in separate patch series.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 include/linux/device.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 1bf5cf0..6ce0f20 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -576,6 +576,11 @@ static inline void *dev_get_platdata(const struct device *dev)
 	return dev->platform_data;
 }
 
+static inline void dev_set_platdata(struct device *dev, void *platdata)
+{
+	dev->platform_data = platdata;
+}
+
 /*
  * Manual binding of a device to driver. See drivers/base/bus.c
  * for information on use.

-- 
viresh