From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752915AbbE0PGS (ORCPT ); Wed, 27 May 2015 11:06:18 -0400 Received: from mail-ig0-f181.google.com ([209.85.213.181]:37564 "EHLO mail-ig0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751340AbbE0PGP (ORCPT ); Wed, 27 May 2015 11:06:15 -0400 MIME-Version: 1.0 Reply-To: cw00.choi@samsung.com In-Reply-To: <5565D6E3.9000907@ti.com> 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> Date: Thu, 28 May 2015 00:06:14 +0900 Message-ID: Subject: Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors From: Chanwoo Choi To: Roger Quadros Cc: linux-kernel , Robert Baldyga , Peter Chen , Kishon Vijay Abraham I , Felipe Balbi , iivanov@mm-sol.com, "myungjoo.ham@samsung.com" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > >> }; >> >> Cc: Myungjoo Ham >> Signed-off-by: Chanwoo Choi >> --- >> drivers/extcon/extcon.c | 74 >> ++++++++++++++++++++++++++++++++++++++++++++++++- >> include/linux/extcon.h | 24 ++++++++++++++++ >> 2 files changed, 97 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index 5099c11..142bf0f 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -279,7 +279,9 @@ int extcon_update_state(struct extcon_dev *edev, u32 >> mask, u32 state) >> >> for (index = 0; index < edev->max_supported; index++) { >> if (is_extcon_changed(edev->state, state, index, >> &attached)) >> - raw_notifier_call_chain(&edev->nh[index], >> attached, edev); >> + raw_notifier_call_chain(&edev->nh[index], >> + attached ? EXTCON_ATTACHED : >> + EXTCON_DETACHED, edev); >> } >> >> edev->state &= ~mask; >> @@ -418,6 +420,69 @@ int extcon_set_cable_state(struct extcon_dev *edev, >> EXPORT_SYMBOL_GPL(extcon_set_cable_state); >> >> /** >> + * extcon_set_cable_line_state() - Set the line state of specific cable. >> + * @edev: the extcon device that has the cable. >> + * @id: the unique id of each external connector. >> + * @state: the line state for specific cable. >> + * >> + * Note that this function support the only USB connector to inform the >> state >> + * of both ID and VBUS line until now. This function may be extended to >> support >> + * the additional external connectors. >> + * >> + * If the id is EXTCON_USB, it can support only following line states: >> + * - EXTCON_USB_ID_LOW >> + * - EXTCON_USB_ID_HIGH, >> + * - EXTCON_USB_VBUS_LOW >> + * - EXTCON_USB_VBUS_HIGH >> + */ >> +int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id, >> + enum extcon_line_state state) >> +{ >> + unsigned long flags; >> + unsigned long line_state; >> + int ret = 0, index; >> + >> + index = find_cable_index_by_id(edev, id); >> + if (index < 0) >> + return index; >> + >> + spin_lock_irqsave(&edev->lock, flags); >> + line_state = edev->line_state[index]; >> + >> + switch (id) { >> + case EXTCON_USB: >> + if (line_state & state) { >> + dev_warn(&edev->dev, >> + "0x%x state is already set for %s\n", >> + state, extcon_name[id]); >> + goto err; >> + } >> + >> + if ((state & EXTCON_USB_ID_LOW) || (state & >> EXTCON_USB_ID_HIGH)) >> + line_state &= ~(EXTCON_USB_ID_LOW | >> EXTCON_USB_ID_HIGH); >> + >> + if ((state & EXTCON_USB_VBUS_LOW) >> + || (state & EXTCON_USB_VBUS_HIGH)) >> + line_state &= >> + ~(EXTCON_USB_VBUS_LOW | >> EXTCON_USB_VBUS_HIGH); >> + >> + line_state |= state; >> + break; >> + default: >> + ret = -EINVAL; >> + goto err; >> + } >> + edev->line_state[index] = line_state; >> + >> + ret = raw_notifier_call_chain(&edev->nh[index], line_state, edev); >> +err: >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_set_cable_line_state); >> + >> +/** >> * extcon_get_extcon_dev() - Get the extcon device instance from the >> name >> * @extcon_name: The extcon name provided with >> extcon_dev_register() >> */ >> @@ -897,6 +962,13 @@ int extcon_dev_register(struct extcon_dev *edev) >> goto err_dev; >> } >> >> + edev->line_state = devm_kzalloc(&edev->dev, >> + sizeof(*edev->line_state) * edev->max_supported, >> GFP_KERNEL); >> + if (!edev->line_state) { >> + ret = -ENOMEM; >> + goto err_dev; >> + } >> + >> for (index = 0; index < edev->max_supported; index++) >> RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]); >> >> diff --git a/include/linux/extcon.h b/include/linux/extcon.h >> index be9652b..79e5073 100644 >> --- a/include/linux/extcon.h >> +++ b/include/linux/extcon.h >> @@ -66,6 +66,19 @@ enum extcon { >> EXTCON_END, >> }; >> >> +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. Cheers, Chanwoo Choi