lkml.org 
[lkml]   [2015]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors
    From
    Date

    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 <rogerq@ti.com> 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.

    I do agree with Roger.

    > > >
    > > > +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
    can not use the one bit for LOW/HIGH?

    Regards,
    Ivan


    \
     
     \ /
      Last update: 2015-05-28 11:41    [W:4.356 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site