Re: [PATCH] drivers: mtd: m25p80: Add quad read support.

[Marek Vasut <marex@denx.de>]

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