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 <ckee...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
