Generic Platform NAND Driver

Generic Platform NAND Driver

[Semih Hazar]

Hi,

In the new generic platforn NAND driver, line 54;

	data->chip.priv = &data;
	data->mtd.priv = &data->chip;

mtd->priv->priv points to struct plat_nand_data which is defined in the 
plat_nand.c and never used by it.
Can we change it so that this priv points to a host specific data, and 
the helper functions (board specific) can make use of it.
Below is a suggested patch:

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index d2365c8..256957b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -599,10 +599,12 @@ struct platform_nand_ctrl {
  * struct platform_nand_data - container structure for platform-specific data
  * @chip:		chip level chip structure
  * @ctrl:		controller level device structure
+ * @host:		pointer to host specific data structure
  */
 struct platform_nand_data {
 	struct platform_nand_chip	chip;
 	struct platform_nand_ctrl	ctrl;
+	void				*host;
 };
 
 /* Some helpers to access the data structures */
diff --git a/drivers/mtd/nand/plat_nand.c b/drivers/mtd/nand/plat_nand.c
index cd725fc..1e47ad3 100644
--- a/drivers/mtd/nand/plat_nand.c
+++ b/drivers/mtd/nand/plat_nand.c
@@ -51,7 +51,7 @@ static int __init plat_nand_probe(struct platform_device *pdev)
 		return -EIO;
 	}
 
-	data->chip.priv = &data;
+	data->chip.priv = pdata->host;
 	data->mtd.priv = &data->chip;

 	data->mtd.owner = THIS_MODULE;

Re: Generic Platform NAND Driver

[Semih Hazar]

Hi Vitaly,

I couldn't get any replies from the list and I wonder if I'm missing a 
point here.

struct plat_nand_data seems useless to users of this generic nand 
driver, and there is no way of passing user data in mtd structures.

Does that make sense, or am I missing something ?

Also the priv members in platform_nand_chip and platform_nand_ctrl are 
not used in any way. Maybe a better solution would be to utilize these ?

> Hi,
>
> In the new generic platforn NAND driver, line 54;
>
> 	data->chip.priv = &data;
> 	data->mtd.priv = &data->chip;
>
> mtd->priv->priv points to struct plat_nand_data which is defined in the 
> plat_nand.c and never used by it.
> Can we change it so that this priv points to a host specific data, and 
> the helper functions (board specific) can make use of it.
> Below is a suggested patch:
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index d2365c8..256957b 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -599,10 +599,12 @@ struct platform_nand_ctrl {
>   * struct platform_nand_data - container structure for platform-specific data
>   * @chip:		chip level chip structure
>   * @ctrl:		controller level device structure
> + * @host:		pointer to host specific data structure
>   */
>  struct platform_nand_data {
>  	struct platform_nand_chip	chip;
>  	struct platform_nand_ctrl	ctrl;
> +	void				*host;
>  };
>  
>  /* Some helpers to access the data structures */
> diff --git a/drivers/mtd/nand/plat_nand.c b/drivers/mtd/nand/plat_nand.c
> index cd725fc..1e47ad3 100644
> --- a/drivers/mtd/nand/plat_nand.c
> +++ b/drivers/mtd/nand/plat_nand.c
> @@ -51,7 +51,7 @@ static int __init plat_nand_probe(struct platform_device *pdev)
>  		return -EIO;
>  	}
>  
> -	data->chip.priv = &data;
> +	data->chip.priv = pdata->host;
>  	data->mtd.priv = &data->chip;
>
>  	data->mtd.owner = THIS_MODULE;
>   

Re: Generic Platform NAND Driver

[Vladimir A. Barinov]

Semih Hazar wrote:
> Hi Vitaly,
>
> I couldn't get any replies from the list and I wonder if I'm missing a 
> point here.
>
> struct plat_nand_data seems useless to users of this generic nand 
> driver, and there is no way of passing user data in mtd structures.
>
> Does that make sense, or am I missing something ?
>
> Also the priv members in platform_nand_chip and platform_nand_ctrl are 
> not used in any way. Maybe a better solution would be to utilize these ?
>   
That structures are for arch related code for NAND flashes that 
registers are memory mapped but
has special h/w control.

You can look at the first (I guess) usage example here:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4385/2

>   
>> Hi,
>>
>> In the new generic platforn NAND driver, line 54;
>>
>> 	data->chip.priv = &data;
>> 	data->mtd.priv = &data->chip;
>>
>> mtd->priv->priv points to struct plat_nand_data which is defined in the 
>> plat_nand.c and never used by it.
>> Can we change it so that this priv points to a host specific data, and 
>> the helper functions (board specific) can make use of it.
>> Below is a suggested patch:
>>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index d2365c8..256957b 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -599,10 +599,12 @@ struct platform_nand_ctrl {
>>   * struct platform_nand_data - container structure for platform-specific data
>>   * @chip:		chip level chip structure
>>   * @ctrl:		controller level device structure
>> + * @host:		pointer to host specific data structure
>>   */
>>  struct platform_nand_data {
>>  	struct platform_nand_chip	chip;
>>  	struct platform_nand_ctrl	ctrl;
>> +	void				*host;
>>  };
>>  
>>  /* Some helpers to access the data structures */
>> diff --git a/drivers/mtd/nand/plat_nand.c b/drivers/mtd/nand/plat_nand.c
>> index cd725fc..1e47ad3 100644
>> --- a/drivers/mtd/nand/plat_nand.c
>> +++ b/drivers/mtd/nand/plat_nand.c
>> @@ -51,7 +51,7 @@ static int __init plat_nand_probe(struct platform_device *pdev)
>>  		return -EIO;
>>  	}
>>  
>> -	data->chip.priv = &data;
>> +	data->chip.priv = pdata->host;
>>  	data->mtd.priv = &data->chip;
>>
>>  	data->mtd.owner = THIS_MODULE;
>>   
>>     
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>   

Re: Generic Platform NAND Driver

[Semih Hazar]

Vladimir A. Barinov wrote:
> You can look at the first (I guess) usage example here:
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4385/2
>
>   

+ixdp425_flash_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
+{
+	struct nand_chip *this = mtd->priv;
+	int offset = (int)this->priv;

Here offset is read from nand_chip->priv, which actually points to struct plat_nand_data of plat_nand.c

Am I wrong ?

Re: Generic Platform NAND Driver

[Vladimir A. Barinov]

Hi Semih,

Semih Hazar wrote:
> Vladimir A. Barinov wrote:
>> You can look at the first (I guess) usage example here:
>> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4385/2
>>
>>   
>
> +ixdp425_flash_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned 
> int ctrl)
> +{
> +    struct nand_chip *this = mtd->priv;
> +    int offset = (int)this->priv;
>
> Here offset is read from nand_chip->priv, which actually points to 
> struct plat_nand_data of plat_nand.c
>
> Am I wrong ? 
I agree with you.
And you are right that all things work fine because the 
mtd_info->priv->priv that points to plat_nand_data is not used.

I agree that the line 54 in plat_nand.c should be removed:

-	data->chip.priv = &data;


But does it really usefull to create void * host pointer if it could be 
done in arch code like the above patch does?

Vladimir

Re: Generic Platform NAND Driver

[Semih Hazar]

Vladimir A. Barinov wrote:
>>
>> +ixdp425_flash_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned 
>> int ctrl)
>> +{
>> +    struct nand_chip *this = mtd->priv;
>> +    int offset = (int)this->priv;
>>
>> Here offset is read from nand_chip->priv, which actually points to 
>> struct plat_nand_data of plat_nand.c
>>
>> Am I wrong ? 
> I agree with you.
> And you are right that all things work fine because the 
> mtd_info->priv->priv that points to plat_nand_data is not used.
>
> I agree that the line 54 in plat_nand.c should be removed:
>
> -    data->chip.priv = &data;
>
>
> But does it really usefull to create void * host pointer if it could 
> be done in arch code like the above patch does?
I thought board-specific code can make use of this area (just as in your 
patch) so it's good to be able pass some _known_ data there.

My code which uses generic nand and is very much like the patch you've 
sent. But we may have board specific CLE, ACE, CE pins so they're not 
#define'd in the machine code, rather they're passed in a struct from 
the board code. And I thought the right thing to so that would be to 
pass that data to the priv member rather than accessing it in a static way.

If it's better to access have these board-specific in a static manner, 
then I'll just go ahead and change my code that way.

Thanks for your feedback.

Regards,
-- 
Semih Hazar