From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bear.ext.ti.com ([192.94.94.41]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VbCX1-0008QQ-GA for linux-mtd@lists.infradead.org; Tue, 29 Oct 2013 16:52:41 +0000 Message-ID: <526FE7B9.3000602@ti.com> Date: Tue, 29 Oct 2013 22:22:09 +0530 From: Sourav Poddar MIME-Version: 1.0 To: Marek Vasut Subject: Re: [PATCH] drivers: mtd: m25p80: Add quad read support. References: <1382693145-15750-1-git-send-email-sourav.poddar@ti.com> <201310291501.06051.marex@denx.de> <526FC153.1020004@ti.com> <201310291627.34003.marex@denx.de> In-Reply-To: <201310291627.34003.marex@denx.de> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: computersforpeace@gmail.com, linux-mtd@lists.infradead.org, balbi@ti.com, dedekind1@gmail.com List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tuesday 29 October 2013 08:57 PM, Marek Vasut wrote: > Dear Sourav Poddar, > >> Dear Marek Vasut, >> >> On Tuesday 29 October 2013 07:31 PM, Marek Vasut wrote: >>> Dear Sourav Poddar, >>> >>>> Hi, >>>> >>>> On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote: >>>>> Dear Sourav Poddar, >>>>> >>>>> [...] >>>>> >>>>>> +static int macronix_quad_enable(struct m25p *flash) >>>>>> +{ >>>>>> + int ret, val; >>>>>> + u8 cmd[2]; >>>>>> + cmd[0] = OPCODE_WRSR; >>>>>> + >>>>>> + val = read_sr(flash); >>>>>> + cmd[1] = val | SR_QUAD_EN_MX; >>>>>> + write_enable(flash); >>>>>> + >>>>>> + spi_write(flash->spi,&cmd, 2); >>>>>> + >>>>>> + if (wait_till_ready(flash)) >>>>>> + return 1; >>>>>> + >>>>>> + ret = read_sr(flash); >>>>> Maybe read_sr() and read_cr() shall be fixed to return retval only and >>>>> the val shall be passed to them as an argument pointer? Aka. ret = >>>>> read_sr(flash,&val); >>>>> >>>>> That way, this dangerous construct below could become: >>>>> >>>>> if (!(val& SR_....)) { >>>>> >>>>> dev_err(); >>>>> ret = -EINVAL; >>>>> >>>>> } >>>>> >>>>> return ret; >>>> I was trying to work on it and realise, we dont need to pass val >>>> directly. We can continue returning the val and can still cleanup the >>>> below code as u suggetsed above. >>>> if (!(ret& SR_....)) { >>>> >>>> dev_err(); >>>> ret = -EINVAL; >>>> >>>> } >>> Uh oh, no. This doesn't seem right. I'd like to be able to clearly check >>> if the function failed to read the register altogether OR if not, check >>> the returned value of the register. Mixing these two together won't do >>> us good. But maybe I just fail to understand your proposal, if so, then >>> I appologize. >> Yes, what I am trying to propose is to eliminate the return error check. > But we want to be able to check if there is a failure :) > >> The check whether register read has happened correctly is embedded in >> read_sr/read_cr function itself. >> if (retval< 0) { >> dev_err(&flash->spi->dev, "error %d reading SR\n", >> (int) retval); >> return retval; >> } >> Same goes for read_cr. >> So, if the above condition is not hit, we simply return the read value and >> check it with the respective bits. > Look here: > > 107 static int read_sr(struct m25p *flash) > 108 { > 109 ssize_t retval; > 110 u8 code = OPCODE_RDSR; > 111 u8 val; > 112 > 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); > 114 > 115 if (retval< 0) { > 116 dev_err(&flash->spi->dev, "error %d reading SR\n", > 117 (int) retval); > 118 return retval; > > here you return error value IFF spi_write_then_read() fails for some reason. > > 119 } > 120 > 121 return val; > > here you return actual value of the register. > > 122 } > > This is how I'd change the function to make it less error-prone: > > *107 static int read_sr(struct m25p *flash, u8 *rval) > 108 { > 109 ssize_t retval; > 110 u8 code = OPCODE_RDSR; > 111 u8 val; > 112 > 113 retval = spi_write_then_read(flash->spi,&code, 1,&val, 1); > 114 > 115 if (retval< 0) { > 116 dev_err(&flash->spi->dev, "error %d reading SR\n", > 117 (int) retval); > 118 return retval; > 119 } > *120 *rval = val; > *121 return 0; > 122 } > > This way, you can check if the SPI read failed and if so, handle it in some way. > The return value would only be valid if this function returned 0. > I got this, but do you think its necessary to have two checks for verifying whether read passed. ? If I go by your code above, after returning from above, check for return value for successful read and then check the respective bit set(SR_*). ? > Best regards, > Marek Vasut