Patchwork [1/2] drivers: mtd: m25p80: convert "bool" read check into an enum

login
register
mail settings
Submitter Poddar, Sourav
Date Nov. 6, 2013, 2:35 p.m.
Message ID <1383748535-20462-2-git-send-email-sourav.poddar@ti.com>
Download mbox | patch
Permalink /patch/288932/
State New
Headers show

Comments

Poddar, Sourav - Nov. 6, 2013, 2:35 p.m.
This is a cleanup prior to adding quad read support. This will fecilitate 
easy addition of more read commands check under an enum rather that defining a 
seperate bool for it.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
Suggested-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c |   71 +++++++++++++++++++++++++++++++++--------
 1 files changed, 57 insertions(+), 14 deletions(-)
Marek Vasut - Nov. 6, 2013, 8:24 p.m.
Dear Sourav Poddar,

> This is a cleanup prior to adding quad read support. This will fecilitate
> easy addition of more read commands check under an enum rather that
> defining a seperate bool for it.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> Suggested-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c |   71
> +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
> insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..cfafdce 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -84,6 +84,11 @@
> 
>  /*************************************************************************
> ***/
> 
> +enum read_type {
> +	M25P80_NORMAL = 0,
> +	M25P80_FAST,
> +};
> +
>  struct m25p {
>  	struct spi_device	*spi;
>  	struct mutex		lock;
> @@ -94,7 +99,7 @@ struct m25p {
>  	u8			read_opcode;
>  	u8			program_opcode;
>  	u8			*command;
> -	bool			fast_read;
> +	enum read_type		flash_read;
>  };
> 
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct
> erase_info *instr) }
> 
>  /*
> + * Dummy Cycle calculation for different type of read.
> + * It can be used to support more commands with
> + * different dummy cycle requirement.
> + */
> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> +{
> +	switch (flash->flash_read) {
> +	case M25P80_FAST:
> +		return 1;
> +	case M25P80_NORMAL:
> +		return 0;
> +	default:
> +		dev_err(&flash->spi->dev, "No valid read type supported");

Does dev_err() insert newlines automatically?
Brian Norris - Nov. 7, 2013, 8:06 a.m.
On Wed, Nov 06, 2013 at 09:24:37PM +0100, Marek Vasut wrote:
> Dear Sourav Poddar,
> 
> > This is a cleanup prior to adding quad read support. This will fecilitate
> > easy addition of more read commands check under an enum rather that
> > defining a seperate bool for it.
> > 
> > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> > Suggested-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/devices/m25p80.c |   71
> > +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
> > insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 7eda71d..cfafdce 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -84,6 +84,11 @@
> > 
> >  /*************************************************************************
> > ***/
> > 
> > +enum read_type {
> > +	M25P80_NORMAL = 0,
> > +	M25P80_FAST,
> > +};
> > +
> >  struct m25p {
> >  	struct spi_device	*spi;
> >  	struct mutex		lock;
> > @@ -94,7 +99,7 @@ struct m25p {
> >  	u8			read_opcode;
> >  	u8			program_opcode;
> >  	u8			*command;
> > -	bool			fast_read;
> > +	enum read_type		flash_read;
> >  };
> > 
> >  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> > @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct
> > erase_info *instr) }
> > 
> >  /*
> > + * Dummy Cycle calculation for different type of read.
> > + * It can be used to support more commands with
> > + * different dummy cycle requirement.
> > + */
> > +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> > +{
> > +	switch (flash->flash_read) {
> > +	case M25P80_FAST:
> > +		return 1;
> > +	case M25P80_NORMAL:
> > +		return 0;
> > +	default:
> > +		dev_err(&flash->spi->dev, "No valid read type supported");
> 
> Does dev_err() insert newlines automatically?

Not sure if this is a rhetorical question, but I'll answer: no,
dev_err() does not insert newlines automatically. The string parameter
should contain the '\n'.

This pair of patches looks good otherwise. I'll see if others have
reviews to make, and I can just fixup the newlines if that's the only
change to make.

Thanks,
Brian
Poddar, Sourav - Nov. 7, 2013, 8:29 a.m.
On Thursday 07 November 2013 01:36 PM, Brian Norris wrote:
> On Wed, Nov 06, 2013 at 09:24:37PM +0100, Marek Vasut wrote:
>> Dear Sourav Poddar,
>>
>>> This is a cleanup prior to adding quad read support. This will fecilitate
>>> easy addition of more read commands check under an enum rather that
>>> defining a seperate bool for it.
>>>
>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>> Suggested-by: Brian Norris<computersforpeace@gmail.com>
>>> ---
>>>   drivers/mtd/devices/m25p80.c |   71
>>> +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
>>> insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>> index 7eda71d..cfafdce 100644
>>> --- a/drivers/mtd/devices/m25p80.c
>>> +++ b/drivers/mtd/devices/m25p80.c
>>> @@ -84,6 +84,11 @@
>>>
>>>   /*************************************************************************
>>> ***/
>>>
>>> +enum read_type {
>>> +	M25P80_NORMAL = 0,
>>> +	M25P80_FAST,
>>> +};
>>> +
>>>   struct m25p {
>>>   	struct spi_device	*spi;
>>>   	struct mutex		lock;
>>> @@ -94,7 +99,7 @@ struct m25p {
>>>   	u8			read_opcode;
>>>   	u8			program_opcode;
>>>   	u8			*command;
>>> -	bool			fast_read;
>>> +	enum read_type		flash_read;
>>>   };
>>>
>>>   static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
>>> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct
>>> erase_info *instr) }
>>>
>>>   /*
>>> + * Dummy Cycle calculation for different type of read.
>>> + * It can be used to support more commands with
>>> + * different dummy cycle requirement.
>>> + */
>>> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
>>> +{
>>> +	switch (flash->flash_read) {
>>> +	case M25P80_FAST:
>>> +		return 1;
>>> +	case M25P80_NORMAL:
>>> +		return 0;
>>> +	default:
>>> +		dev_err(&flash->spi->dev, "No valid read type supported");
>> Does dev_err() insert newlines automatically?
> Not sure if this is a rhetorical question, but I'll answer: no,
> dev_err() does not insert newlines automatically. The string parameter
> should contain the '\n'.
>
> This pair of patches looks good otherwise. I'll see if others have
> reviews to make, and I can just fixup the newlines if that's the only
> change to make.
>
Thanks a lot Brian.
> Thanks,
> Brian
Marek Vasut - Nov. 7, 2013, 12:53 p.m.
Hi Brian,

> On Wed, Nov 06, 2013 at 09:24:37PM +0100, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> > > This is a cleanup prior to adding quad read support. This will
> > > fecilitate easy addition of more read commands check under an enum
> > > rather that defining a seperate bool for it.
> > > 
> > > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> > > Suggested-by: Brian Norris <computersforpeace@gmail.com>
> > > ---
> > > 
> > >  drivers/mtd/devices/m25p80.c |   71
> > > 
> > > +++++++++++++++++++++++++++++++++-------- 1 files changed, 57
> > > insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/devices/m25p80.c
> > > b/drivers/mtd/devices/m25p80.c index 7eda71d..cfafdce 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -84,6 +84,11 @@
> > > 
> > >  /*********************************************************************
> > >  ****
> > > 
> > > ***/
> > > 
> > > +enum read_type {
> > > +	M25P80_NORMAL = 0,
> > > +	M25P80_FAST,
> > > +};
> > > +
> > > 
> > >  struct m25p {
> > >  
> > >  	struct spi_device	*spi;
> > >  	struct mutex		lock;
> > > 
> > > @@ -94,7 +99,7 @@ struct m25p {
> > > 
> > >  	u8			read_opcode;
> > >  	u8			program_opcode;
> > >  	u8			*command;
> > > 
> > > -	bool			fast_read;
> > > +	enum read_type		flash_read;
> > > 
> > >  };
> > >  
> > >  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> > > 
> > > @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd,
> > > struct erase_info *instr) }
> > > 
> > >  /*
> > > 
> > > + * Dummy Cycle calculation for different type of read.
> > > + * It can be used to support more commands with
> > > + * different dummy cycle requirement.
> > > + */
> > > +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> > > +{
> > > +	switch (flash->flash_read) {
> > > +	case M25P80_FAST:
> > > +		return 1;
> > > +	case M25P80_NORMAL:
> > > +		return 0;
> > > +	default:
> > > +		dev_err(&flash->spi->dev, "No valid read type supported");
> > 
> > Does dev_err() insert newlines automatically?
> 
> Not sure if this is a rhetorical question, but I'll answer: no,
> dev_err() does not insert newlines automatically. The string parameter
> should contain the '\n'.
> 
> This pair of patches looks good otherwise. I'll see if others have
> reviews to make, and I can just fixup the newlines if that's the only
> change to make.

Yep, I think the patches are good otherwise. Thanks!
 
> Thanks,
> Brian
Brian Norris - Nov. 8, 2013, 6:06 p.m.
On Wed, Nov 06, 2013 at 08:05:34PM +0530, Sourav Poddar wrote:
> This is a cleanup prior to adding quad read support. This will fecilitate 
> easy addition of more read commands check under an enum rather that defining a 
> seperate bool for it.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> Suggested-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c |   71 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7eda71d..cfafdce 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -84,6 +84,11 @@
>  
>  /****************************************************************************/
>  
> +enum read_type {
> +	M25P80_NORMAL = 0,
> +	M25P80_FAST,
> +};
> +
>  struct m25p {
>  	struct spi_device	*spi;
>  	struct mutex		lock;
> @@ -94,7 +99,7 @@ struct m25p {
>  	u8			read_opcode;
>  	u8			program_opcode;
>  	u8			*command;
> -	bool			fast_read;
> +	enum read_type		flash_read;
>  };
>  
>  static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>  }
>  
>  /*
> + * Dummy Cycle calculation for different type of read.
> + * It can be used to support more commands with
> + * different dummy cycle requirement.

I changed:

s/requirement/requirements/

> + */
> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
> +{
> +	switch (flash->flash_read) {
> +	case M25P80_FAST:
> +		return 1;
> +	case M25P80_NORMAL:
> +		return 0;
> +	default:
> +		dev_err(&flash->spi->dev, "No valid read type supported");

Added '\n' to the dev_err() strings (in multiple places).

> +		return -1;
> +	}
> +}
> +
> +/*
>   * Read an address range from the flash chip.  The address range
>   * may be any size provided it is within the physical boundaries.
>   */

...

And pushed to l2-mtd.git. Thanks!

Brian
Poddar, Sourav - Nov. 8, 2013, 6:28 p.m.
On Friday 08 November 2013 11:36 PM, Brian Norris wrote:
> On Wed, Nov 06, 2013 at 08:05:34PM +0530, Sourav Poddar wrote:
>> This is a cleanup prior to adding quad read support. This will fecilitate
>> easy addition of more read commands check under an enum rather that defining a
>> seperate bool for it.
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> Suggested-by: Brian Norris<computersforpeace@gmail.com>
>> ---
>>   drivers/mtd/devices/m25p80.c |   71 +++++++++++++++++++++++++++++++++--------
>>   1 files changed, 57 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index 7eda71d..cfafdce 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -84,6 +84,11 @@
>>
>>   /****************************************************************************/
>>
>> +enum read_type {
>> +	M25P80_NORMAL = 0,
>> +	M25P80_FAST,
>> +};
>> +
>>   struct m25p {
>>   	struct spi_device	*spi;
>>   	struct mutex		lock;
>> @@ -94,7 +99,7 @@ struct m25p {
>>   	u8			read_opcode;
>>   	u8			program_opcode;
>>   	u8			*command;
>> -	bool			fast_read;
>> +	enum read_type		flash_read;
>>   };
>>
>>   static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
>> @@ -350,6 +355,24 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
>>   }
>>
>>   /*
>> + * Dummy Cycle calculation for different type of read.
>> + * It can be used to support more commands with
>> + * different dummy cycle requirement.
> I changed:
>
> s/requirement/requirements/
>
>> + */
>> +static inline int m25p80_dummy_cycles_read(struct m25p *flash)
>> +{
>> +	switch (flash->flash_read) {
>> +	case M25P80_FAST:
>> +		return 1;
>> +	case M25P80_NORMAL:
>> +		return 0;
>> +	default:
>> +		dev_err(&flash->spi->dev, "No valid read type supported");
> Added '\n' to the dev_err() strings (in multiple places).
>
>> +		return -1;
>> +	}
>> +}
>> +
>> +/*
>>    * Read an address range from the flash chip.  The address range
>>    * may be any size provided it is within the physical boundaries.
>>    */
> ...
>
> And pushed to l2-mtd.git. Thanks!
>
> Brian
Thanks Brian!
Just curious, is 2nd patch of this series not pushed for purpose? Its
reviewed by Marek and you said in one of your reply that its look good
to you too.

Patch

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7eda71d..cfafdce 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -84,6 +84,11 @@ 
 
 /****************************************************************************/
 
+enum read_type {
+	M25P80_NORMAL = 0,
+	M25P80_FAST,
+};
+
 struct m25p {
 	struct spi_device	*spi;
 	struct mutex		lock;
@@ -94,7 +99,7 @@  struct m25p {
 	u8			read_opcode;
 	u8			program_opcode;
 	u8			*command;
-	bool			fast_read;
+	enum read_type		flash_read;
 };
 
 static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -350,6 +355,24 @@  static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
 }
 
 /*
+ * Dummy Cycle calculation for different type of read.
+ * It can be used to support more commands with
+ * different dummy cycle requirement.
+ */
+static inline int m25p80_dummy_cycles_read(struct m25p *flash)
+{
+	switch (flash->flash_read) {
+	case M25P80_FAST:
+		return 1;
+	case M25P80_NORMAL:
+		return 0;
+	default:
+		dev_err(&flash->spi->dev, "No valid read type supported");
+		return -1;
+	}
+}
+
+/*
  * Read an address range from the flash chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
@@ -360,6 +383,7 @@  static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	struct spi_transfer t[2];
 	struct spi_message m;
 	uint8_t opcode;
+	int dummy;
 
 	pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
 			__func__, (u32)from, len);
@@ -367,8 +391,14 @@  static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	spi_message_init(&m);
 	memset(t, 0, (sizeof t));
 
+	dummy =  m25p80_dummy_cycles_read(flash);
+	if (dummy < 0) {
+		dev_err(&flash->spi->dev, "No valid read command supported");
+		return -EINVAL;
+	}
+
 	t[0].tx_buf = flash->command;
-	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);
+	t[0].len = m25p_cmdsz(flash) + dummy;
 	spi_message_add_tail(&t[0], &m);
 
 	t[1].rx_buf = buf;
@@ -391,8 +421,7 @@  static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 
 	spi_sync(flash->spi, &m);
 
-	*retlen = m.actual_length - m25p_cmdsz(flash) -
-			(flash->fast_read ? 1 : 0);
+	*retlen = m.actual_length - m25p_cmdsz(flash) - dummy;
 
 	mutex_unlock(&flash->lock);
 
@@ -1051,22 +1080,31 @@  static int m25p_probe(struct spi_device *spi)
 	flash->page_size = info->page_size;
 	flash->mtd.writebufsize = flash->page_size;
 
-	if (np)
+	if (np) {
 		/* If we were instantiated by DT, use it */
-		flash->fast_read = of_property_read_bool(np, "m25p,fast-read");
-	else
+		if (of_property_read_bool(np, "m25p,fast-read"))
+			flash->flash_read = M25P80_FAST;
+	} else {
 		/* If we weren't instantiated by DT, default to fast-read */
-		flash->fast_read = true;
+		flash->flash_read = M25P80_FAST;
+	}
 
 	/* Some devices cannot do fast-read, no matter what DT tells us */
 	if (info->flags & M25P_NO_FR)
-		flash->fast_read = false;
+		flash->flash_read = M25P80_NORMAL;
 
 	/* Default commands */
-	if (flash->fast_read)
+	switch (flash->flash_read) {
+	case M25P80_FAST:
 		flash->read_opcode = OPCODE_FAST_READ;
-	else
+		break;
+	case M25P80_NORMAL:
 		flash->read_opcode = OPCODE_NORM_READ;
+		break;
+	default:
+		dev_err(&flash->spi->dev, "No Read opcode defined");
+		return -EINVAL;
+	}
 
 	flash->program_opcode = OPCODE_PP;
 
@@ -1077,9 +1115,14 @@  static int m25p_probe(struct spi_device *spi)
 		flash->addr_width = 4;
 		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
-			flash->read_opcode = flash->fast_read ?
-				OPCODE_FAST_READ_4B :
-				OPCODE_NORM_READ_4B;
+			switch (flash->flash_read) {
+			case M25P80_FAST:
+				flash->read_opcode = OPCODE_FAST_READ_4B;
+				break;
+			case M25P80_NORMAL:
+				flash->read_opcode = OPCODE_NORM_READ_4B;
+				break;
+			}
 			flash->program_opcode = OPCODE_PP_4B;
 			/* No small sector erase for 4-byte command set */
 			flash->erase_opcode = OPCODE_SE_4B;