Re: [PATCH v2 1/4] mtd: cfi_cmdset_0002: Add support for reading OTP

[Christian Riesch <christian.riesch@omicron.at>]

On 2013-05-29 15:53, Artem Bityutskiy wrote:
> On Wed, 2013-05-29 at 14:54 +0200, Christian Riesch wrote:
>> Artem,
>> Thank you very much for your comments!
>>
>> On 2013-05-29 09:11, Artem Bityutskiy wrote:
>>> On Fri, 2013-04-26 at 21:10 +0200, Christian Riesch wrote:
>>>> +static int cfi_amdstd_get_fact_prot_info(struct mtd_info *mtd,
>>>> +                                          struct otp_info *buf,
>>>> size_t len)
>>>> +{
>>>> +       size_t retlen;
>>>> +       int ret;
>>>> +
>>>> +       ret = cfi_amdstd_otp_walk(mtd, 0, len, &retlen, (u_char *)buf,
>>>> NULL, 0);
>>>> +       return ret ? : retlen;
>>>> +}
>>>
>>> Hmm, I thought we would return 0 in case of success and a negative error
>>> code in case of error. Returning retlen looks wrong.
>>>
>>
>> I don't think so. Somehow the caller of this function should be told how
>> many bytes have been written into buf, there is no other way to return
>> the data length than the return value. Anyway, I just stole this code
>> from cfi_cmdset_0001.c (function cfi_intelext_get_fact_prot_info), it is
>> done in the same way there.
>
> But retlen is where you return the amount of bytes you actually wrote.
> The return code is either 0 or a negative error code number. This is
> what we do in mtd_write() and I'd expect the OTP functions to be
> consistent.

Unlike mtd_write, mtd->_get_fact_prot_info (which is a pointer to 
cfi_amdstd_get_fact_prot_info here) does not have a retlen parameter. Do 
you suggest to change this?

Regards, Christian