From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752118AbcG2HC2 (ORCPT ); Fri, 29 Jul 2016 03:02:28 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:52229 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750799AbcG2HC0 convert rfc822-to-8bit (ORCPT ); Fri, 29 Jul 2016 03:02:26 -0400 X-AuditID: cbfee68d-f79286d000007a9a-11-579aff809176 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <579AFF7F.2000006@samsung.com> Date: Fri, 29 Jul 2016 16:02:23 +0900 From: Chanwoo Choi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Guenter Roeck Cc: linux-kernel , myungjoo.ham@samsung.com, Chris Zhong , Guenter Roeck , cwchoi00@gmail.com, "cpgs (cpgs@samsung.com)" Subject: Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type References: <1469534988-8305-1-git-send-email-cw00.choi@samsung.com> <1469534988-8305-3-git-send-email-cw00.choi@samsung.com> In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrNIsWRmVeSWpSXmKPExsWyRsSkULfh/6xwg03/OSxeHtK0eHZU2+LU q2XMFr2LlrBYXN41h83iduMKNov5D3YwO7B7zG64yOKxc9Zddo8Fm0o9/s7az+LRt2UVo8fn TXIBbFFcNimpOZllqUX6dglcGSuXPGEqOBFesbb1FHsDY7NdFyMnh4SAicTNmy+ZIGwxiQv3 1rN1MXJxCAmsYJR4sWY9C0zR5mXTGCESSxklHsw9AJbgFRCU+DH5HpDNwcEsoC4xZUouSJhZ QFTiUecpRghbW2LZwtfMEL0PGCVunz/ODFLPK6AlsbJfDaSGRUBV4nt7H9gRbEDh/S9usIGU iApESHSfqAQJiwCVnHp6HOw2ZoFrjBJNi5eCzRcWiJc4uH0qK8T8WUwSWzc8YwVJcAoES3yY uRHqgWvsEmdmpEAsE5D4NvkQ2M0SArISmw4wQ5RIShxccYNlAqP4LCSfzUL4bBaSz2Yh+WwB I8sqRtHUguSC4qT0IkO94sTc4tK8dL3k/NxNjMA4Pf3vWe8OxtsHrA8xCnAwKvHwzpCeFS7E mlhWXJl7iNEU6KCJzFKiyfnAZJBXEm9obGZkYWpiamxkbmmmJM6rKPUzWEggPbEkNTs1tSC1 KL6oNCe1+BAjEwenVAPjsd/vzROUDOZusxMtWvgiP8hl1aK9AlMPPj+mdTjBdaqn9y+uh1W2 k/9ytLfMjT0/z8n9x5Ws8ISAIzkTd1zbpbx/eStz/uWss6J6MltZ7oRzP7epfh+luU1vyv6g jpMSr/wfSYpJbI36vcX5p6zTwr7cpCDmE3O0q05O+S+Vu2Uur7fS4c6zSizFGYmGWsxFxYkA Tn3g1s4CAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrPIsWRmVeSWpSXmKPExsVy+t9jAd2G/7PCDWb1MFm8PKRp8eyotsWp V8uYLXoXLWGxuLxrDpvF7cYVbBbzH+xgdmD3mN1wkcVj56y77B4LNpV6/J21n8Wjb8sqRo/P m+QC2KIaGG0yUhNTUosUUvOS81My89JtlbyD453jTc0MDHUNLS3MlRTyEnNTbZVcfAJ03TJz gC5RUihLzCkFCgUkFhcr6dthmhAa4qZrAdMYoesbEgTXY2SABhLWMGY8+raOqaAtvGLarPds DYwHbbsYOTkkBEwkNi+bxghhi0lcuLeerYuRi0NIYCmjxIO5B1hAErwCghI/Jt8Dsjk4mAXk JY5cyoYw1SWmTMmFKH/AKHH7/HFmkDivgJbEyn41kE4WAVWJ7+19TCA2G1B4/4sbbCAlogIR Et0nKkHCIkAlp54eB9vKLHCNUaJp8VKwc4QF4iUObp/KCjF/FpPE1g3PWEESnALBEh9mbmSZ wCgwC8l1sxCum4Vw3QJG5lWMEqkFyQXFSem5Rnmp5XrFibnFpXnpesn5uZsYwZH9THoH4+Fd 7ocYBTgYlXh4G+bMChdiTSwrrsw9xCjBwawkwnv/D1CINyWxsiq1KD++qDQntfgQoynQfxOZ pUST84FJJ68k3tDYxMzI0sjc0MLI2FxJnPfx/3VhQgLpiSWp2ampBalFMH1MHJxSDYz7FayE 2j1K9K63szGXK0Sv2rR6m9mOd3afzaPflt4s0pM8eGHTPYHL94vfn6s5sTHjgk0r66nLjRFu X2dv/qssvHD9kl86Rk5VyQKxKZ+KDrx/zN18S6tj719zpc4TTLe+vzdTOb9La5v4ypNKNXNe ruc8cq5UyXWrVOLGIze9wwNnLJHQ8g5QYinOSDTUYi4qTgQA7YJargIDAAA= 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 Hi Guenter, On 2016년 07월 28일 02:24, Guenter Roeck wrote: > On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi wrote: >> This patch support the extcon property for the external connector >> because each external connector might have the property according to >> the H/W design and the specific characteristics. >> >> - EXTCON_PROP_USB_[property name] >> - EXTCON_PROP_CHG_[property name] >> - EXTCON_PROP_JACK_[property name] >> - EXTCON_PROP_DISP_[property name] >> >> Add the new extcon APIs to get/set the property value as following: >> - int extcon_get_property(struct extcon_dev *edev, unsigned int id, >> unsigned int prop, >> union extcon_property_value *prop_val) >> - int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> unsigned int prop, >> union extcon_property_value prop_val) >> >> Signed-off-by: Chanwoo Choi >> --- >> drivers/extcon/extcon.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++- >> include/linux/extcon.h | 86 ++++++++++++++++++++ >> 2 files changed, 291 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index b1e6ee6194bc..2317aaea063f 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -196,6 +196,11 @@ struct extcon_cable { >> struct device_attribute attr_state; >> >> struct attribute *attrs[3]; /* to be fed to attr_g.attrs */ >> + >> + union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT]; >> + union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT]; >> + union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT]; >> + union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT]; >> }; >> >> static struct class *extcon_class; >> @@ -248,6 +253,28 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id >> return -EINVAL; >> } >> >> +static int get_extcon_type(unsigned int prop) >> +{ >> + switch (prop) { >> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX: >> + return EXTCON_TYPE_USB; >> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX: >> + return EXTCON_TYPE_CHG; >> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX: >> + return EXTCON_TYPE_JACK; >> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX: >> + return EXTCON_TYPE_DISP; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int id, > > 'id' isn't used. I'll remove it on parameter list. > >> + unsigned int index) >> +{ >> + return ((!!(edev->state & (1 << index))) == 1) ? true : false; > > Minor comment: This is identical to > > return !!(edev->state & (1 << index)); > or, with bitops > return !!(edev->state & BIT(index)); I'll use the bitops as you comment. return !!(edev->state & BIT(index)); > >> +} >> + >> static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached) >> { >> if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) { >> @@ -258,6 +285,41 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached) >> return false; >> } >> >> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop) >> +{ >> + unsigned int type; >> + >> + /* Check whether the property is supported or not. */ >> + type = get_extcon_type(prop); >> + if (type < 0) >> + return false; >> + >> + /* Check whether a specific extcon id supports the property or not. */ >> + if (extcon_info[id].type | type) >> + return true; >> + >> + return false; > > simpler: > return !!(extcon_info[id].type & type); OK. I'll use it as you comment. > > Strictly speaking, the !! isn't even necessary in those mask > operations since C auto-converts to bool, though people sometimes get > confused by that. Thanks for your explanation. For readability, I remain the '!!' operation. > >> +} >> + >> +#define INIT_PROPERTY(name, name_lower, type) \ >> + if (EXTCON_TYPE_##name || type) { \ >> + for (i = 0; i < EXTCON_PROP_##name##_CNT; i++) \ >> + cable->name_lower##_propval[i] = val; \ >> + } \ >> + >> +static void init_property(struct extcon_dev *edev, unsigned int id, int index) >> +{ >> + unsigned int type = extcon_info[id].type; >> + struct extcon_cable *cable = &edev->cables[index]; >> + union extcon_property_value val = (union extcon_property_value)(0); >> + int i; >> + >> + INIT_PROPERTY(USB, usb, type); >> + INIT_PROPERTY(CHG, chg, type); >> + INIT_PROPERTY(JACK, jack, type); >> + INIT_PROPERTY(DISP, disp, type); > > Just wondering if this would be a bit cleaner/simpler. > > switch(type) { > case EXTCON_TYPE_USB: > memset(cable->usb_propval, sizeof(cable->usb_propval), 0); > break; > case EXTCON_TYPE_CHG: > memset(cable->chg_propval, sizeof(cable->chg_propval), 0); > break; > case EXTCON_TYPE_JACK: > memset(cable->jack_propval, sizeof(cable->jack_propval), 0); > break; > case EXTCON_TYPE_DISP: > memset(cable->disp_propval, sizeof(cable->disp_propval), 0); > break; > } As you comment, I'll modify it as following: But the each id is able to have the one more extcon type. So, I use the 'if' instead of 'switch'. if (EXTCON_TYPE_USB & type) memset(cable->usb_propval, sizeof(cable->usb_propval), 0); if (EXTCON_TYPE_CHG & type) memset(cable->chg_propval, sizeof(cable->chg_propval), 0); if (EXTCON_TYPE_JACK & type) memset(cable->jack_propval, sizeof(cable->jack_propval), 0); if (EXTCON_TYPE_DISP & type) memset(cable->disp_propval, sizeof(cable->disp_propval), 0); > >> +} >> + >> static ssize_t state_show(struct device *dev, struct device_attribute *attr, >> char *buf) >> { >> @@ -421,7 +483,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id) >> if (edev->max_supported && edev->max_supported <= index) >> return -EINVAL; >> >> - return !!(edev->state & (1 << index)); >> + return (int)(is_extcon_attached(edev, id, index)); >> } >> EXPORT_SYMBOL_GPL(extcon_get_cable_state_); >> >> @@ -449,12 +511,154 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id, >> if (edev->max_supported && edev->max_supported <= index) >> return -EINVAL; >> >> + /* >> + * Initialize the value of extcon property before setting >> + * the detached state for an external connector. >> + */ >> + if (!cable_state) >> + init_property(edev, id, index); >> + >> + /* Set the state for external connector as the detached state. */ >> state = cable_state ? (1 << index) : 0; >> return extcon_update_state(edev, 1 << index, state); >> } >> EXPORT_SYMBOL_GPL(extcon_set_cable_state_); >> >> /** >> + * extcon_get_property() - Get the property value of a specific cable. >> + * @edev: the extcon device that has the cable. >> + * @id: the unique id of each external connector >> + * in extcon enumeration. >> + * @prop: the property id among enum extcon_property. >> + * @prop_val: the pointer which store the value of property. >> + * >> + * When getting the property value of external connector, the external connector >> + * should be attached. If detached state, function just return 0 without >> + * property value. Also, the each property should be included in the list of >> + * supported properties according to the type of external connectors. >> + * >> + * Returns 0 if success or error number if fail >> + */ >> +int extcon_get_property(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop, >> + union extcon_property_value *prop_val) >> +{ >> + struct extcon_cable *cable; >> + unsigned long flags; >> + int index, ret = 0; >> + >> + *prop_val = (union extcon_property_value)(0); >> + >> + if (!edev) >> + return -EINVAL; >> + >> + /* Check whether the property is supported or not */ >> + if (!is_extcon_property_supported(id, prop)) >> + return -EINVAL; >> + >> + /* Find the cable index of external connector by using id */ >> + index = find_cable_index_by_id(edev, id); >> + if (index < 0) >> + return index; >> + >> + /* >> + * Check whether the external connector is attached. >> + * If external connector is detached, the user can not >> + * get the property value. >> + */ >> + if (!is_extcon_attached(edev, id, index)) >> + return 0; >> + > > Wonder if this should be inside the lock. Otherwise the cable state > might change after the check, but before the lock is acquired. > >> + cable = &edev->cables[index]; >> + spin_lock_irqsave(&edev->lock, flags); You're right. I'll fix it as following. spin_lock_irqsave(&edev->lock, flags); if (!is_extcon_attached(edev, id, index)) { spin_unlock_irqrestore(&edev->lock, flags); return 0; } cable = &edev->cables[index]; >> + >> + /* Get the property value according to extcon type */ >> + switch (prop) { >> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX: >> + *prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN]; >> + break; >> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX: >> + *prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN]; >> + break; >> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX: >> + *prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN]; >> + break; >> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX: >> + *prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN]; >> + break; >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_get_property); [snip] Regards, Chanwoo Choi