From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-out.m-online.net ([212.18.0.9]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VbBD3-0003rb-CK for linux-mtd@lists.infradead.org; Tue, 29 Oct 2013 15:27:58 +0000 From: Marek Vasut To: Sourav Poddar Subject: Re: [PATCH] drivers: mtd: m25p80: Add quad read support. Date: Tue, 29 Oct 2013 16:27:33 +0100 References: <1382693145-15750-1-git-send-email-sourav.poddar@ti.com> <201310291501.06051.marex@denx.de> <526FC153.1020004@ti.com> In-Reply-To: <526FC153.1020004@ti.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201310291627.34003.marex@denx.de> 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: , 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. Best regards, Marek Vasut