lkml.org 
[lkml]   [2013]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 4/4] extcon: arizona: Eliminate dead error handling code
On 11/12/2013 11:30 PM, Charles Keepax wrote:
> On Tue, Nov 12, 2013 at 09:15:29AM +0900, Chanwoo Choi wrote:
>> Hi CHarles,
>>
>> On 11/08/2013 10:19 PM, Charles Keepax wrote:
>>> As a small disclaimer I would personally prefer to not merge this patch.
>>> I have added it based on previous code review of the other patches in
>>> this chain.
>>>
>>> arizona_hpdet_do_id currently can only return 0 or -EAGAIN making the
>>> else if clause handling error codes redundant, this patch removes this
>>> clause.
>>>
>>> Whilst this clause is not currently hit removing it makes the code
>>> fragile. It will not be obvious whilst editing arizona_hpdet_do_id that
>>> you shouldn't add a return value other than 0 or -EAGAIN.
>>>
>>> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>> ---
>>> drivers/extcon/extcon-arizona.c | 5 +++--
>>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>> index 0d70bf6..2313b1e 100644
>>> --- a/drivers/extcon/extcon-arizona.c
>>> +++ b/drivers/extcon/extcon-arizona.c
>>> @@ -476,6 +476,9 @@ static int arizona_hpdet_read(struct arizona_extcon_info *info)
>>> return val;
>>> }
>>>
>>> +/* This function should only return 0 or -EAGAIN, if other return values are
>>> + * added additional handling should be added in arizona_hpdet_irq.
>>> + */
>>
>> As Lee Jones commented, you should modify this comment of arizona_hpdet_do_id() and add
>> the description of return value. Because arizoa_hpdet_do_id() has different meaning
>> between -EAGAIN and other minus value.
>
> I take it you are still very keen on applying a patch for this
> dead code elimination? I really do feel it would be better to
> leave this part of the code as it currently is, the extra safety
> clearly outweights the cost of a redundant else if.

In that case, I think that you could modify arizona_hpdet_read() to add exception handling
for minus return value instead of elimination.

I knew about your opinion. But I think it isn't proper method remaining dead code on mainline tree.
People who don't know the history about this dead code would think that "Is this code necessary?".

Thanks,
Chanwoo Choi




\
 
 \ /
  Last update: 2013-11-13 03:21    [W:0.054 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site