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

[Marek Vasut <marex@denx.de>]

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.

[...]

Best regards,
Marek Vasut