From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755598AbbE2H6y (ORCPT ); Fri, 29 May 2015 03:58:54 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:32899 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752189AbbE2H6p (ORCPT ); Fri, 29 May 2015 03:58:45 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee68f-f793b6d000005f66-1e-55681c2b915c Content-transfer-encoding: 8BIT Message-id: <55681C2A.6090105@samsung.com> Date: Fri, 29 May 2015 16:58:34 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130106 Thunderbird/17.0.2 To: "Ivan T. Ivanov" Cc: Roger Quadros , linux-kernel , Robert Baldyga , Peter Chen , Kishon Vijay Abraham I , Felipe Balbi , "myungjoo.ham@samsung.com" Subject: Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors References: <1432728910-10761-1-git-send-email-cw00.choi@samsung.com> <1432728910-10761-2-git-send-email-cw00.choi@samsung.com> <5565D6E3.9000907@ti.com> <1432803731.2348.28.camel@mm-sol.com> <1432805845.2348.33.camel@mm-sol.com> In-reply-to: <1432805845.2348.33.camel@mm-sol.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrMIsWRmVeSWpSXmKPExsWyRsSkSFdbJiPUYH2fvMXB+/UWzyY+ZLS4 8LSHzeLyrjlsFrcbV7BZHJv9l8niweGd7BY9j7QcODz+He5n8ti0fxqzR9+WVYwex29sZ/L4 vEkugDWKyyYlNSezLLVI3y6BK6PrnETBJdmKtQdVGhhPinUxcnJICJhITPl5mRXCFpO4cG89 WxcjF4eQwFJGiU87trHBFE2ec5URIjGdUWLPmxawDl4BQYkfk++xdDFycDALyEscuZQNEmYW UJeYNG8RM0T9A0aJK8samEBqeAW0JOYsLgKpYRFQlXjd8YEZxGYDCu9/cQNsl6hAmMTK6VfA RooIaEpsOhMMMoZZYD6TxIwNF8HWCgvUSmz4twLq0N9MEq/fTWIHSXAKGElsXHwBLCEhcI1d 4sPJO6wQ2wQkvk0+BDZVQkBWYtMBZojHJCUOrrjBMoFRbBaSd2YhvDMLyTsLGJlXMYqmFiQX FCelFxnrFSfmFpfmpesl5+duYgTG3el/z/p3MN49YH2IUYCDUYmHt+NGeqgQa2JZcWXuIUZT oCMmMkuJJucDozuvJN7Q2MzIwtTE1NjI3NJMSZx3odTPYCGB9MSS1OzU1ILUovii0pzU4kOM TBycUsDY4vfdW5piO/VxtxtPw7XrvVlveHaqNaw9/s6375yo8BGJQ0ufHbBfcX9iN0v9iqfF qTXlR/54fb5zqOHKN8EKPpst0Vw7pvvfXHlHfpbCO74vH25eKX17k/Fhl0WEbOWRY4G5/hmp 6l/O3PEs2tRr4no+0fXxknKDLVdnnu54f9tu+YlFa3xClFiKMxINtZiLihMBFYjpi7YCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrLIsWRmVeSWpSXmKPExsVy+t9jQV1tmYxQg655khYH79dbPJv4kNHi wtMeNovLu+awWdxuXMFmcWz2XyaLB4d3slv0PNJy4PD4d7ifyWPT/mnMHn1bVjF6HL+xncnj 8ya5ANaoBkabjNTElNQihdS85PyUzLx0WyXv4HjneFMzA0NdQ0sLcyWFvMTcVFslF58AXbfM HKBTlBTKEnNKgUIBicXFSvp2mCaEhrjpWsA0Ruj6hgTB9RgZoIGENYwZXeckCi7JVqw9qNLA eFKsi5GTQ0LARGLynKuMELaYxIV769m6GLk4hASmM0rsedPCCpLgFRCU+DH5HksXIwcHs4C8 xJFL2SBhZgF1iUnzFjFD1D9glLiyrIEJpIZXQEtizuIikBoWAVWJ1x0fmEFsNqDw/hc32EBs UYEwiZXTr4CNFBHQlNh0JhhkDLPAfCaJGRsugq0VFqiV2PBvBdQ9v5kkXr+bxA6S4BQwkti4 +ALbBEaBWUjOm4Vw3iwk5y1gZF7FKJpakFxQnJSea6hXnJhbXJqXrpecn7uJERzVz6R2MK5s sDjEKMDBqMTD23EjPVSINbGsuDL3EKMEB7OSCO/Ot0Ah3pTEyqrUovz4otKc1OJDjKZA301k lhJNzgcmnLySeENjEzMjSyNzQwsjY3Mlcd6T+T6hQgLpiSWp2ampBalFMH1MHJxSDYwNMcee P9e6+n/XpLlp725bLXCoevn1bdfmFZJ3w7yU7qx4l63Ia3eaMzV9eYyrXhrH0/l/rz2Py/x5 syHmRznjxCuVeQLiWS2HtC4niqcsvlivZ1DTlfdy0duw7QY80oJabVXsDG4rA163f6hb+M/v 0mGrBJ7VDplhYa07+Z58MPk1ax1/zAElluKMREMt5qLiRABSyjTRAAMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/28/2015 06:37 PM, Ivan T. Ivanov wrote: > Hi, > > On Thu, 2015-05-28 at 18:17 +0900, Chanwoo Choi wrote: >> >> >> 2015년 5월 28일 목요일, Ivan T. Ivanov님이 작성한 메시지: >>> Hi Chanwoo, >>> >>> On Thu, 2015-05-28 at 00:06 +0900, Chanwoo Choi wrote: >>>> On Wed, May 27, 2015 at 11:38 PM, Roger Quadros wrote: >>>>> Chanwoo, >>>>> >>>>> On 27/05/15 15:15, Chanwoo Choi wrote: >>>>>> This patch adds the extcon_set_cable_line_state() function to inform >>>>>> the additional state of each external connector and 'enum >>>>>> extcon_line_state' >>>>>> enumeration which include the specific states of each external connector. >>>>>> >>>>>> The each external connector might need the different line state. So, >>>>>> current >>>>>> 'extcon_line_state' enumeration contains the specific state for USB as >>>>>> following: >>>>>> >>>>>> - Following the state mean the state of both ID and VBUS line for USB: >>>>>> enum extcon_line_state { >>>>>> EXTCON_USB_ID_LOW = BIT(1), /* ID line is low. */ >>>>>> EXTCON_USB_ID_HIGH = BIT(2), /* ID line is high. */ >>>>> >>>>> >>>>> ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit >>>>> position. >>>>> if the bit is set means it is high, if it is cleared means it is low. >>>> >>>> These are the notifier event. So, extcon_line_state have to include >>>> each event for both low or high state of ID. >>>> because the extcon consumer driver using the >>>> extcon_register_notifier() need the each event to distinguish the type >>>> of event. >>>> >>>>>> EXTCON_USB_VBUS_LOW = BIT(3), /* VBUS line is low. */ >>>>>> EXTCON_USB_VBUS_HIGH = BIT(4), /* VBUS line is high. */ >>>>> >>>>> >>>>> same here. >>>> >>>> ditto. >>>> >>>>> enum doesn't look like the right type for extcon_line_state as it is more of >>>>> a bitmask. >>>> >>>> Why? I prefer to use the enum if there are no problem. >>> >> If you insist your opinion, you shoud suggest the reason. Could you tell me the reason? > > It looks unreadable. There is no need to mix interface API (enum extcon_line_state) > with implementation details. You can use plain enum at interface level and hide > shifting inside implementation. OK, I define it instead of enum. > >> >> >>>>>> >>>>>> +enum extcon_line_state { >>>>>> + /* Following two definition are used for whether external >>>>>> connectors >>>>>> + * is attached or detached. */ >>>>>> + EXTCON_DETACHED = 0x0, >>>>>> + EXTCON_ATTACHED = 0x1, >>>>>> + >>>>>> + /* Following states are only used for EXTCON_USB. */ >>>>>> + EXTCON_USB_ID_LOW = BIT(1), /* ID line is low. */ >>>>>> + EXTCON_USB_ID_HIGH = BIT(2), /* ID line is high. */ >>>>>> + EXTCON_USB_VBUS_LOW = BIT(3), /* VBUS line is low. */ >>>>>> + EXTCON_USB_VBUS_HIGH = BIT(4), /* VBUS line is high. */ >>>>>> +}; >>>>> >>>>> >>>>> Why do you use enum? How about the following bit definitions for line state. >>>>> >>>>> #define EXTCON_ATTACHED_DETACHED_BIT BIT(0) >>>>> #define EXTCON_USB_ID_BIT BIT(1) >>>>> #define EXTCON_USB_VBUS_BIT BIT(2) >>>>> ... >>>>> >>>>> code must check if appropriate bit is set or cleared to get the high/low >>>>> state. >>>> >>>> I answer about it on upper. >>>> >>> >>> Funny. We can use the one bit for DETACHED/ATTACHED, but we >> hmm. Did you read my answer about low/high? >> >> But, I'm considering again about using the detached/attached value. > > Ok, but consumers will receive one event at time, right? Yes. Cheers, Chanwoo Choi