From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752832AbbEQIOJ (ORCPT ); Sun, 17 May 2015 04:14:09 -0400 Received: from mail-ig0-f196.google.com ([209.85.213.196]:34884 "EHLO mail-ig0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752453AbbEQIKy (ORCPT ); Sun, 17 May 2015 04:10:54 -0400 MIME-Version: 1.0 Reply-To: cw00.choi@samsung.com In-Reply-To: References: <1431700278-30465-1-git-send-email-cw00.choi@samsung.com> <1431700278-30465-2-git-send-email-cw00.choi@samsung.com> Date: Sun, 17 May 2015 16:53:59 +0900 Message-ID: Subject: Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string From: Chanwoo Choi To: Krzysztof Kozlowski Cc: linux-kernel , "myungjoo.ham@samsung.com" , Charles Keepax , gg@slimlogic.co.uk, Kishon Vijay Abraham I , jaewon02.kim@samsung.com, Roger Quadros , "Pallala, Ramakrishna" , george.cherian@ti.com, Felipe Balbi , Aaro Koskinen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Krzysztof, On Sun, May 17, 2015 at 4:10 PM, Krzysztof Kozlowski wrote: > 2015-05-15 23:31 GMT+09:00 Chanwoo Choi : >> This patch uses the unique id to identify the type of external connector instead >> of string name. The string name have the many potential issues. So, this patch >> defines the 'extcon' enumeration which includes all supported external connector >> on EXTCON subsystem. If new external connector is necessary, the unique id of >> new connector have to be added in 'extcon' enumeration. There are current >> supported external connector in 'enum extcon' as following: > > I like the idea of switching to unique identifier. Some comments below. > >> >> enum extcon { >> EXTCON_NONE = 0x0, /* NONE */ >> >> /* USB external connector */ >> EXTCON_USB = 0x1, /* USB */ >> EXTCON_USB_HOST = 0x2, /* USB-HOST */ >> >> /* Charger external connector */ >> EXTCON_TA = 0x10, /* TA */ >> EXTCON_FAST_CHARGER = 0x11, /* FAST-CHARGER */ >> EXTCON_SLOW_CHARGER = 0x12, /* SLOW-CHARGER */ >> EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-DOWNSTREAM */ >> >> /* Audio and video external connector */ >> EXTCON_LINE_IN = 0x20, /* LINE-IN */ >> EXTCON_LINE_OUT = 0x21, /* LINE-OUT */ >> EXTCON_MICROPHONE = 0x22, /* MICROPHONE */ >> EXTCON_HEADPHONE = 0x23, /* HEADPHONE */ >> >> EXTCON_HDMI = 0x30, /* HDMI */ >> EXTCON_MHL = 0x31, /* MHL */ >> EXTCON_DVI = 0x32, /* DVI */ >> EXTCON_VGA = 0x33, /* VGA */ >> EXTCON_SPDIF_IN = 0x34, /* SPDIF-IN */ >> EXTCON_SPDIF_OUT = 0x35, /* SPDIF-OUT */ >> EXTCON_VIDEO_IN = 0x36, /* VIDEO-IN */ >> EXTCON_VIDEO_OUT = 0x37, /* VIDEO-OUT */ >> >> /* Miscellaneous external connector */ >> EXTCON_DOCK = 0x50, /* DOCK */ >> EXTCON_JIG = 0x51, /* JIG */ >> EXTCON_MECHANICAL = 0x52, /* MECHANICAL */ >> >> __EXTCON_END, >> }; >> >> For exmaple in extcon-arizoan.c: > s/exmaple/example/ > s/arizoan/arizona/ Good catch. I'll fix it. > >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index 2fb5f75..4aeb585 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -3,6 +3,9 @@ >> * >> * External connector (extcon) class driver >> * >> + * Copyright (C) 2015 Samsung Electronics >> + * Author: Chanwoo Choi >> + * >> * Copyright (C) 2012 Samsung Electronics >> * Author: Donggeun Kim >> * Author: MyungJoo Ham >> @@ -32,36 +35,43 @@ >> #include >> #include >> >> -/* >> - * extcon_cable_name suggests the standard cable names for commonly used >> - * cable types. >> - * >> - * However, please do not use extcon_cable_name directly for extcon_dev >> - * struct's supported_cable pointer unless your device really supports >> - * every single port-type of the following cable names. Please choose cable >> - * names that are actually used in your extcon device. >> - */ >> -const char extcon_cable_name[][CABLE_NAME_MAX + 1] = { >> +#define SUPPORTED_CABLE_MAX 32 > > Why only 32 cables are supported? I mean what is the reason behind the > hard limit? Current extcon core use the 32bit variable to identify the supported external connectors of each extcon device. But, the number of supported connectors should be updated. I have the plan about it. > >> +#define CABLE_NAME_MAX 30 >> + >> +static const char *extcon_name[] = { >> + [EXTCON_NONE] = "NONE", >> + >> + /* USB external connector */ >> [EXTCON_USB] = "USB", >> - [EXTCON_USB_HOST] = "USB-Host", >> + [EXTCON_USB_HOST] = "USB-HOST", >> + >> + /* Charger external connector */ >> [EXTCON_TA] = "TA", >> - [EXTCON_FAST_CHARGER] = "Fast-charger", >> - [EXTCON_SLOW_CHARGER] = "Slow-charger", >> - [EXTCON_CHARGE_DOWNSTREAM] = "Charge-downstream", >> + [EXTCON_FAST_CHARGER] = "FAST-CHARGER", >> + [EXTCON_SLOW_CHARGER] = "SLOW-CHARGER", >> + [EXTCON_CHARGE_DOWNSTREAM] = "CHARGE-DOWNSTREAM", >> + >> + /* Audio/Video external connector */ >> + [EXTCON_LINE_IN] = "LINE-IN", >> + [EXTCON_LINE_OUT] = "LINE-OUT", >> + [EXTCON_MICROPHONE] = "MICROPHONE", >> + [EXTCON_HEADPHONE] = "HEADPHONE", >> + >> [EXTCON_HDMI] = "HDMI", >> [EXTCON_MHL] = "MHL", >> [EXTCON_DVI] = "DVI", >> [EXTCON_VGA] = "VGA", >> - [EXTCON_DOCK] = "Dock", >> - [EXTCON_LINE_IN] = "Line-in", >> - [EXTCON_LINE_OUT] = "Line-out", >> - [EXTCON_MIC_IN] = "Microphone", >> - [EXTCON_HEADPHONE_OUT] = "Headphone", >> - [EXTCON_SPDIF_IN] = "SPDIF-in", >> - [EXTCON_SPDIF_OUT] = "SPDIF-out", >> - [EXTCON_VIDEO_IN] = "Video-in", >> - [EXTCON_VIDEO_OUT] = "Video-out", >> - [EXTCON_MECHANICAL] = "Mechanical", >> + [EXTCON_SPDIF_IN] = "SPDIF-IN", >> + [EXTCON_SPDIF_OUT] = "SPDIF-OUT", >> + [EXTCON_VIDEO_IN] = "VIDEO-IN", >> + [EXTCON_VIDEO_OUT] = "VIDEO-OUT", >> + >> + /* Etc external connector */ >> + [EXTCON_DOCK] = "DOCK", >> + [EXTCON_JIG] = "JIG", >> + [EXTCON_MECHANICAL] = "MECHANICAL", >> + >> + NULL, >> }; > > This change does not look related to the topic. Can you split it to > separate patch? Do you mean about changing the name of external connectors from small letter to capital letter? > >> >> static struct class *extcon_class; >> @@ -118,11 +128,9 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr, >> if (edev->max_supported == 0) >> return sprintf(buf, "%u\n", edev->state); >> >> - for (i = 0; i < SUPPORTED_CABLE_MAX; i++) { >> - if (!edev->supported_cable[i]) >> - break; >> + for (i = 0; i < edev->max_supported; i++) { >> count += sprintf(buf + count, "%s=%d\n", >> - edev->supported_cable[i], >> + extcon_name[edev->supported_cable[i]], >> !!(edev->state & (1 << i))); >> } >> >> @@ -171,9 +179,10 @@ static ssize_t cable_name_show(struct device *dev, >> { >> struct extcon_cable *cable = container_of(attr, struct extcon_cable, >> attr_name); >> + int i = cable->cable_index; >> >> return sprintf(buf, "%s\n", >> - cable->edev->supported_cable[cable->cable_index]); >> + extcon_name[cable->edev->supported_cable[i]]); >> } >> >> static ssize_t cable_state_show(struct device *dev, >> @@ -282,39 +291,57 @@ int extcon_set_state(struct extcon_dev *edev, u32 state) >> } >> EXPORT_SYMBOL_GPL(extcon_set_state); >> >> -/** >> - * extcon_find_cable_index() - Get the cable index based on the cable name. >> - * @edev: the extcon device that has the cable. >> - * @cable_name: cable name to be searched. >> - * >> - * Note that accessing a cable state based on cable_index is faster than >> - * cable_name because using cable_name induces a loop with strncmp(). >> - * Thus, when get/set_cable_state is repeatedly used, using cable_index >> - * is recommended. >> - */ >> -int extcon_find_cable_index(struct extcon_dev *edev, const char *cable_name) >> +static int extcon_find_cable_index(struct extcon_dev *edev, >> + const char *cable_name) >> { >> + enum extcon id = EXTCON_NONE; >> int i; >> >> - if (edev->supported_cable) { >> - for (i = 0; edev->supported_cable[i]; i++) { >> - if (!strncmp(edev->supported_cable[i], >> - cable_name, CABLE_NAME_MAX)) >> - return i; >> + if (edev->max_supported == 0) >> + return -EINVAL; >> + >> + /* Find the the number of extcon cable */ >> + for (i = 0; i < __EXTCON_END; i++) { >> + if (!extcon_name[i]) >> + continue; >> + if (!strncmp(extcon_name[i], cable_name, CABLE_NAME_MAX)) { >> + id = i; >> + break; >> } >> } >> >> + if (id == EXTCON_NONE) >> + return -EINVAL; >> + >> + /* Find the the index of extcon cable in edev->supported_cable */ >> + for (i = 0; i < edev->max_supported; i++) { >> + if (edev->supported_cable[i] == id) >> + return i; >> + } >> + >> return -EINVAL; >> } >> -EXPORT_SYMBOL_GPL(extcon_find_cable_index); >> >> /** >> * extcon_get_cable_state_() - Get the status of a specific cable. >> * @edev: the extcon device that has the cable. >> - * @index: cable index that can be retrieved by extcon_find_cable_index(). >> + * @id: the unique id of each external connector in extcon enumeration. >> */ >> -int extcon_get_cable_state_(struct extcon_dev *edev, int index) >> +int extcon_get_cable_state_(struct extcon_dev *edev, const enum extcon id) >> { >> + int i, index = -EINVAL; >> + >> + /* Find the the index of extcon cable in edev->supported_cable */ >> + for (i = 0; edev->max_supported < i; i++) { >> + if (edev->supported_cable[i] == id) { >> + index = i; >> + break; >> + } >> + } >> + >> + if (i == edev->max_supported) >> + return -EINVAL; >> + >> if (index < 0 || (edev->max_supported && edev->max_supported <= index)) >> return -EINVAL; >> >> @@ -339,15 +366,27 @@ EXPORT_SYMBOL_GPL(extcon_get_cable_state); >> /** >> * extcon_set_cable_state_() - Set the status of a specific cable. >> * @edev: the extcon device that has the cable. >> - * @index: cable index that can be retrieved by >> - * extcon_find_cable_index(). >> - * @cable_state: the new cable status. The default semantics is >> + * @id: the unique id of each external connector >> + * in extcon enumeration. >> + * @state: the new cable status. The default semantics is >> * true: attached / false: detached. >> */ >> -int extcon_set_cable_state_(struct extcon_dev *edev, >> - int index, bool cable_state) >> +int extcon_set_cable_state_(struct extcon_dev *edev, enum extcon id, >> + bool cable_state) >> { >> u32 state; >> + int i, index = -EINVAL; >> + >> + /* Find the the index of extcon cable in edev->supported_cable */ >> + for (i = 0; i < edev->max_supported; i++) { >> + if (edev->supported_cable[i] == id) { >> + index = i; >> + break; >> + } >> + } > > This loop shows in few places, maybe split it to separate static > function? Just to avoid duplication of code. OK. > >> + >> + if (i == edev->max_supported) >> + return -EINVAL; >> >> if (index < 0 || (edev->max_supported && edev->max_supported <= index)) >> return -EINVAL; >> @@ -605,7 +644,7 @@ static void dummy_sysfs_dev_release(struct device *dev) >> * >> * Return the pointer of extcon device if success or ERR_PTR(err) if fail >> */ >> -struct extcon_dev *extcon_dev_allocate(const char **supported_cable) >> +struct extcon_dev *extcon_dev_allocate(const enum extcon *supported_cable) > > I think you also have to update the documentation. At least for > {devm}_extcon_dev_allocate but maybe in other places too. Previously > the documentation states that supported_cable is an array of strings. > Additionally AFAIU now it must end with EXTCON_NONE. This > sentinel-like info must be clearly documented. OK. > >> { >> struct extcon_dev *edev; >> >> @@ -659,7 +698,7 @@ static void devm_extcon_dev_release(struct device *dev, void *res) >> * or ERR_PTR(err) if fail >> */ >> struct extcon_dev *devm_extcon_dev_allocate(struct device *dev, >> - const char **supported_cable) >> + const enum extcon *supported_cable) >> { >> struct extcon_dev **ptr, *edev; >> >> @@ -709,17 +748,15 @@ int extcon_dev_register(struct extcon_dev *edev) >> return ret; >> } >> >> - if (edev->supported_cable) { >> - /* Get size of array */ >> - for (index = 0; edev->supported_cable[index]; index++) >> - ; >> - edev->max_supported = index; >> - } else { >> - edev->max_supported = 0; >> - } >> + if (!edev->supported_cable) >> + return -EINVAL; >> >> + for (; edev->supported_cable[index] != EXTCON_NONE; index++); >> + >> + edev->max_supported = index; >> if (index > SUPPORTED_CABLE_MAX) { >> - dev_err(&edev->dev, "extcon: maximum number of supported cables exceeded.\n"); >> + dev_err(&edev->dev, >> + "exceed the maximum number of supported cables\n"); >> return -EINVAL; >> } >> >> @@ -1070,6 +1107,7 @@ static void __exit extcon_class_exit(void) >> } >> module_exit(extcon_class_exit); >> >> +MODULE_AUTHOR("Chanwoo Choi "); >> MODULE_AUTHOR("Mike Lockwood "); >> MODULE_AUTHOR("Donggeun Kim "); >> MODULE_AUTHOR("MyungJoo Ham "); >> diff --git a/include/linux/extcon.h b/include/linux/extcon.h >> index 799474d9d..de158a1 100644 >> --- a/include/linux/extcon.h >> +++ b/include/linux/extcon.h >> @@ -1,6 +1,9 @@ >> /* >> * External connector (extcon) class driver >> * >> + * Copyright (C) 2015 Samsung Electronics >> + * Author: Chanwoo Choi >> + * >> * Copyright (C) 2012 Samsung Electronics >> * Author: Donggeun Kim >> * Author: MyungJoo Ham >> @@ -27,8 +30,41 @@ >> #include >> #include >> >> -#define SUPPORTED_CABLE_MAX 32 >> -#define CABLE_NAME_MAX 30 >> +enum extcon { >> + EXTCON_NONE = 0x0, /* NONE */ >> + >> + /* USB external connector */ >> + EXTCON_USB = 0x1, /* USB */ >> + EXTCON_USB_HOST = 0x2, /* USB-HOST */ >> + >> + /* Charger external connector */ >> + EXTCON_TA = 0x10, /* TA */ >> + EXTCON_FAST_CHARGER = 0x11, /* FAST-CHARGER */ >> + EXTCON_SLOW_CHARGER = 0x12, /* SLOW-CHARGER */ >> + EXTCON_CHARGE_DOWNSTREAM= 0x13, /* CHARGE-DOWNSTREAM */ > > Space before '='. I know it will mess the alignment in indentation but > it would be still more readable. OK. > >> + >> + /* Audio/Video external connector */ >> + EXTCON_LINE_IN = 0x20, /* LINE-IN */ >> + EXTCON_LINE_OUT = 0x21, /* LINE-OUT */ >> + EXTCON_MICROPHONE = 0x22, /* MICROPHONE */ >> + EXTCON_HEADPHONE = 0x23, /* HEADPHONE */ >> + >> + EXTCON_HDMI = 0x30, /* HDMI */ >> + EXTCON_MHL = 0x31, /* MHL */ >> + EXTCON_DVI = 0x32, /* DVI */ >> + EXTCON_VGA = 0x33, /* VGA */ >> + EXTCON_SPDIF_IN = 0x34, /* SPDIF-IN */ >> + EXTCON_SPDIF_OUT = 0x35, /* SPDIF-OUT */ >> + EXTCON_VIDEO_IN = 0x36, /* VIDEO-IN */ >> + EXTCON_VIDEO_OUT = 0x37, /* VIDEO-OUT */ >> + >> + /* Etc external connector */ >> + EXTCON_DOCK = 0x50, /* DOCK */ >> + EXTCON_JIG = 0x51, /* JIG */ >> + EXTCON_MECHANICAL = 0x52, /* MECHANICAL */ >> + > > There is no benefit of each comment here. The comment just duplicates > name, it does not give any additional information. Get rid of it. OK. > >> + __EXTCON_END, > > Why "__" prefix? This variable should be used in extcon core. So, I just add '__' prefix. But, It is not clear. I'll define it without '__' prefix. > >> +}; >> >> /* >> * The standard cable name is to help support general notifier >> @@ -48,29 +84,6 @@ >> * you don't need such convention. This convention is helpful when >> * notifier can distinguish but notifiee cannot. >> */ > > Isn't the comment above related to the enum "extcon_cable_name" which > you are removing? OK. I'll modify the upper comment. Thanks, Chanwoo Choi