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

[Marek Vasut <marex@denx.de>]

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.