From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from comal.ext.ti.com ([198.47.26.152]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1VbCqk-000260-84 for linux-mtd@lists.infradead.org; Tue, 29 Oct 2013 17:13:03 +0000 Message-ID: <526FEC7D.6060403@ti.com> Date: Tue, 29 Oct 2013 22:42:29 +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> <201310291627.34003.marex@denx.de> <526FE7B9.3000602@ti.com> <201310291808.58939.marex@denx.de> In-Reply-To: <201310291808.58939.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 10:38 PM, Marek Vasut wrote: > Dear Sourav Poddar, > >> 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. ? > Yes of course it is necessary, how else would you be able to tell if the value > is valid ? Sure, you can depend on negative integer here and on the fact that > the u8 will never be 32-bits wide (to produce a negative integer when the return > value is valid), but personally I think this is error-prone as hell. > >> 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_*). ? > Yes, you will be checking the bit in SR only if you are sure the value is valid. hmm..alrite I will do the cleanup and send v2.