From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752253AbbEQHKe (ORCPT ); Sun, 17 May 2015 03:10:34 -0400 Received: from mail-vn0-f41.google.com ([209.85.216.41]:32943 "EHLO mail-vn0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751821AbbEQHK0 (ORCPT ); Sun, 17 May 2015 03:10:26 -0400 MIME-Version: 1.0 In-Reply-To: <1431700278-30465-2-git-send-email-cw00.choi@samsung.com> 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:10:24 +0900 X-Google-Sender-Auth: 40V9s2B9FAGt5lnMthRmpseM4uA Message-ID: Subject: Re: [PATCH 1/2] extcon: Use the unique id for external connector instead of string From: Krzysztof Kozlowski To: Chanwoo Choi Cc: linux-kernel@vger.kernel.org, myungjoo.ham@samsung.com, Kozik , ckeepax@opensource.wolfsonmicro.com, gg@slimlogic.co.uk, kishon@ti.com, jaewon02.kim@samsung.com, rogerq@ti.com, ramakrishna.pallala@intel.com, george.cherian@ti.com, balbi@ti.com, aaro.koskinen@iki.fi Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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/ > 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? > +#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? > > 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. > + > + 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. > { > 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. > + > + /* 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. > + __EXTCON_END, Why "__" 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? > -enum extcon_cable_name { > - EXTCON_USB = 0, > - EXTCON_USB_HOST, > - EXTCON_TA, /* Travel Adaptor */ > - EXTCON_FAST_CHARGER, > - EXTCON_SLOW_CHARGER, > - EXTCON_CHARGE_DOWNSTREAM, /* Charging an external device */ > - EXTCON_HDMI, > - EXTCON_MHL, > - EXTCON_DVI, > - EXTCON_VGA, > - EXTCON_DOCK, > - EXTCON_LINE_IN, > - EXTCON_LINE_OUT, > - EXTCON_MIC_IN, > - EXTCON_HEADPHONE_OUT, > - EXTCON_SPDIF_IN, > - EXTCON_SPDIF_OUT, > - EXTCON_VIDEO_IN, > - EXTCON_VIDEO_OUT, > - EXTCON_MECHANICAL, > -}; Best regards, Krzysztof