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

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

On Tuesday 29 October 2013 10:42 PM, Sourav Poddar wrote:
> 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.
I think it will be better to take the above recommended cleanup as a 
seperate patch
on top of $subject patch? Since, read_sr is not only used by the pieces 
added in this patch.
There are other already available apis in the driver which make use of 
this function.
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/