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

[Sourav Poddar <sourav.poddar@ti.com>]

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. ?
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_*). ?

> Best regards,
> Marek Vasut