From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754403AbcHBIJK (ORCPT ); Tue, 2 Aug 2016 04:09:10 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:40433 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751325AbcHBIIn (ORCPT ); Tue, 2 Aug 2016 04:08:43 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee68e-f79cb6d000006cfe-20-57a055019920 Content-transfer-encoding: 8BIT Message-id: <57A05501.8040508@samsung.com> Date: Tue, 02 Aug 2016 17:08:33 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Roger Quadros , linux-kernel@vger.kernel.org, Felipe Balbi Cc: myungjoo.ham@samsung.com, zyw@rock-chips.com, groeck@chromium.org, chanwoo@kernel.org, Krzysztof Kozlowski Subject: Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type References: <1470103105-5992-1-git-send-email-cw00.choi@samsung.com> <1470103105-5992-3-git-send-email-cw00.choi@samsung.com> <1c9b6c33-5699-2767-f97c-d14fb0db7ec6@ti.com> In-reply-to: <1c9b6c33-5699-2767-f97c-d14fb0db7ec6@ti.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrEIsWRmVeSWpSXmKPExsWyRsSkUJcxdEG4waFHxhbH2p6wW0y8cYXF 4tSrZcwWr18YWlzeNYfN4nbjCjaLnkdaFvMf7GB24PCY3XCRxWPTqk42j7+z9rN49G1Zxehx /MZ2Jo/Pm+QC2KK4bFJSczLLUov07RK4MnYuVytYXlVx/v4H9gbGjqQuRk4OCQETiR9HprNA 2GISF+6tZ+ti5OIQEljBKHGz6SQrTFHP5r0sEIlZjBI7j+xmBknwCghK/Jh8DyjBwcEsIC9x 5FI2SJhZQF1i0rxFYCVCAg8YJf7t4gUp4RXQkpi3yggkzCKgKjHh9Fk2EJsNKLz/xQ0wm19A UeLqj8eMIOWiAhES3ScqQcIiAkkSP45fALuAWaCLUeLdnANg9cJAiUdNV9ghVjUzSfw9Lg3S yylgJXFgvSJIvYTAR3aJxTuamCD2Ckh8m3wI7GIJAVmJTQeYIT6UlDi44gbLBEbxWUj+moXw 1ywkfy1gZF7FKJpakFxQnJReZKRXnJhbXJqXrpecn7uJERiVp/8969vBePOA9SFGAQ5GJR5e jQfzw4VYE8uKK3MPMZoCHTGRWUo0OR8Y+3kl8YbGZkYWpiamxkbmlmZK4rwJUj+DhQTSE0tS s1NTC1KL4otKc1KLDzEycXBKNTDavSk3vfT5HGdzsK5hb+bN3JZdnr/W88S9DnoZl760RvPh 1f3SmdV9Tvv2SPFMz49/bt2gcCyMOV307/HXKVOnC25+vdVEwJDJv1FrGb/wq+SdZUleUi9O etr8YJm0Ir4/7pePVOfVOxsbZLz8xLJSTs4vtP/2VPnCk/pf524J3ZnK/u7+0klKLMUZiYZa zEXFiQDpt4lVxQIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrPIsWRmVeSWpSXmKPExsVy+t9jQV3G0AXhBv+7dSyOtT1ht5h44wqL xalXy5gtXr8wtLi8aw6bxe3GFWwWPY+0LOY/2MHswOExu+Eii8emVZ1sHn9n7Wfx6NuyitHj +I3tTB6fN8kFsEU1MNpkpCampBYppOYl56dk5qXbKnkHxzvHm5oZGOoaWlqYKynkJeam2iq5 +AToumXmAN2jpFCWmFMKFApILC5W0rfDNCE0xE3XAqYxQtc3JAiux8gADSSsYczYuVytYHlV xfn7H9gbGDuSuhg5OSQETCR6Nu9lgbDFJC7cW8/WxcjFISQwi1Fi55HdzCAJXgFBiR+T7wEV cXAwC8hLHLmUDRJmFlCXmDRvEViJkMADRol/u3hBSngFtCTmrTICCbMIqEpMOH2WDcRmAwrv f3EDzOYXUJS4+uMxI0i5qECERPeJSpCwiECSxI/jF1hALmAW6GKUeDfnAFi9MFDiUdMVdohV zUwSf49Lg/RyClhJHFivOIFRcBaSO2ch3DkLyZ0LGJlXMUqkFiQXFCel5xrmpZbrFSfmFpfm pesl5+duYgRH/jOpHYwHd7kfYhTgYFTi4bV4Nz9ciDWxrLgy9xCjBAezkghvddCCcCHelMTK qtSi/Pii0pzU4kOMpkCvTmSWEk3OByalvJJ4Q2MTMyNLI3NDCyNjcyVx3sf/14UJCaQnlqRm p6YWpBbB9DFxcEo1MLp/5hd98FKlt31Xzfxm0V3fEg+dvXi1lPv/z10zk36Irp3Q6N2z6eV3 84+zd5tI187/7Xb3pKjUzOtbbx9eoiD3N/iT3Qynj4G6IS0dBYJbWhfO853jIGh1ulpg60GW zmkax8WX2H2T+S5986Ry6pwNX+fciT++4+VCG84ptyzOc+5fsCo8+eU6JZbijERDLeai4kQA xEaymBIDAAA= 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, On 2016년 08월 02일 16:43, Roger Quadros wrote: > +Felipe > > Hi, > > On 02/08/16 04:58, 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 >> Tested-by: Chris Zhong >> Tested-by: Guenter Roeck >> Reviewed-by: Guenter Roeck >> --- >> drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++- >> include/linux/extcon.h | 86 +++++++++++++++++++++ >> 2 files changed, 286 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index 129afc87313e..bb6e99fe84c8 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,27 @@ 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 index) >> +{ >> + 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 +284,34 @@ 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) >> +{ >> + 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. */ >> + return !!(extcon_info[id].type & type); >> +} >> + >> +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]; >> + >> + if (EXTCON_TYPE_USB & type) >> + memset(cable->usb_propval, 0, sizeof(cable->usb_propval)); >> + if (EXTCON_TYPE_CHG & type) >> + memset(cable->chg_propval, 0, sizeof(cable->chg_propval)); >> + if (EXTCON_TYPE_JACK & type) >> + memset(cable->jack_propval, 0, sizeof(cable->jack_propval)); >> + if (EXTCON_TYPE_DISP & type) >> + memset(cable->disp_propval, 0, sizeof(cable->disp_propval)); >> +} >> + >> static ssize_t state_show(struct device *dev, struct device_attribute *attr, >> char *buf) >> { >> @@ -421,7 +475,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 is_extcon_attached(edev, index); >> } >> EXPORT_SYMBOL_GPL(extcon_get_cable_state_); >> >> @@ -449,12 +503,157 @@ 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); >> + > > I'm a bit concerned about this for USB case. See below why. > >> 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; >> + >> + spin_lock_irqsave(&edev->lock, flags); >> + >> + /* >> + * Check whether the external connector is attached. >> + * If external connector is detached, the user can not >> + * get the property value. >> + */ > > How will this work for USB case? We need to know VBUS and ID states > even if the USB cable is detached. When USB is detached, extcon_get_property return the default value without any operation. The default value of supported property are 0 (zero). If new property need the differnt default value, I'll support it. > > Moreover there is no specific mechanism to detect if the USB cable is attached > or not in the extcon-usb-gpio.c case. > One solution could be to set EXTCON_USB as always attached on probe in extcon-usb-gpio.c. > > Is this acceptable? No. the extcon have to detect the correct state. When USB cable is detached, the extcon cannot set the attached state for EXTCON_USB. > >> + if (!is_extcon_attached(edev, 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; > > All these should be simplified to > *prop_val = cable->foo_propval[prop]; > see below why. You mean that the macro? I already used the macro. But, as comment, I don't use it. > >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_get_property); >> + >> +/** >> + * extcon_set_property() - Set 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 including the new value of property. >> + * >> + * 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_set_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; >> + >> + 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; >> + >> + spin_lock_irqsave(&edev->lock, flags); >> + >> + cable = &edev->cables[index]; >> + >> + /* Set the property value according to extcon type */ >> + switch (prop) { >> + case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX: >> + cable->usb_propval[prop - EXTCON_PROP_USB_MIN] = prop_val; >> + break; >> + case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX: >> + cable->chg_propval[prop - EXTCON_PROP_CHG_MIN] = prop_val; >> + break; >> + case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX: >> + cable->jack_propval[prop - EXTCON_PROP_JACK_MIN] = prop_val; >> + break; >> + case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX: >> + cable->disp_propval[prop - EXTCON_PROP_DISP_MIN] = prop_val; >> + break; > > All these should be simplified to > *prop_val = cable->foo_propval[prop]; > see below why. ditto. > >> + default: >> + ret = -EINVAL; >> + break; >> + } >> + >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_set_property); >> + >> +/** >> * extcon_get_extcon_dev() - Get the extcon device instance from the name >> * @extcon_name: The extcon name provided with extcon_dev_register() >> */ >> diff --git a/include/linux/extcon.h b/include/linux/extcon.h >> index 46d802892c82..fe583fca7732 100644 >> --- a/include/linux/extcon.h >> +++ b/include/linux/extcon.h >> @@ -77,6 +77,68 @@ >> >> #define EXTCON_NUM 63 >> >> +/* >> + * Define the property of supported external connectors. >> + * >> + * When adding the new extcon property, they *must* have >> + * the type/value/default information. Also, you *have to* >> + * modify the EXTCON_PROP_[type]_START/END definitions >> + * which mean the range of the supported properties >> + * for each extcon type. >> + * >> + * The naming style of property >> + * : EXTCON_PROP_[type]_[property name] >> + * >> + * EXTCON_PROP_USB_[property name] : USB property >> + * EXTCON_PROP_CHG_[property name] : Charger property >> + * EXTCON_PROP_JACK_[property name] : Jack property >> + * EXTCON_PROP_DISP_[property name] : Display property >> + */ >> + >> +/* >> + * Properties of EXTCON_TYPE_USB. >> + * >> + * - EXTCON_PROP_USB_ID >> + * @type: integer (intval) >> + * @value: 0 (low) or 1 (high) >> + * @default: 0 (low) >> + * - EXTCON_PROP_USB_VBUS >> + * @type: integer (intval) >> + * @value: 0 (low) or 1 (high) >> + * @default: 0 (low) >> + */ >> +#define EXTCON_PROP_USB_ID 0 >> +#define EXTCON_PROP_USB_VBUS 1 >> + >> +#define EXTCON_PROP_USB_MIN 0 >> +#define EXTCON_PROP_USB_MAX 1 >> +#define EXTCON_PROP_USB_CNT (EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1) > > As the properties themselves are separated by the cable type > they can overlap. > > So MIN is not needed as it is always 0 Yes. Just adding the EXTCON_PROP_USB_MIN because remaining the consistent expression for other types (EXTCON_PROP_[type]_MIN). > > #define EXTCON_PROP_USB_ID 0 > #define EXTCON_PROP_USB_VBUS 1 > #define EXTCON_PROP_USB_MAX 1 > > #define EXTCON_PROP_CHG_MAX 1 > #define EXTCON_PROP_CHG_CNT (EXTCON_PROP_CHG_MAX + 1) > > and so on. > >> + >> +/* Properties of EXTCON_TYPE_CHG. */ >> +#define EXTCON_PROP_CHG_MIN 50 >> +#define EXTCON_PROP_CHG_MAX 50 >> +#define EXTCON_PROP_CHG_CNT (EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1) >> + >> +/* Properties of EXTCON_TYPE_JACK. */ >> +#define EXTCON_PROP_JACK_MIN 100 >> +#define EXTCON_PROP_JACK_MAX 100 >> +#define EXTCON_PROP_JACK_CNT (EXTCON_PROP_JACK_MAX - EXTCON_PROP_JACK_MIN + 1) >> + >> +/* Properties of EXTCON_TYPE_DISP. */ >> +#define EXTCON_PROP_DISP_MIN 150 >> +#define EXTCON_PROP_DISP_MAX 150 >> +#define EXTCON_PROP_DISP_CNT (EXTCON_PROP_DISP_MAX - EXTCON_PROP_DISP_MIN + 1) >> + >> +/* >> + * Define the type of property's value. >> + * >> + * Define the property's value as union type. Because each property >> + * would need the different data type to store it. >> + */ >> +union extcon_property_value { >> + int intval; /* type : integer (intval) */ >> +}; >> + > > Why is this a union? There is only one member. No. The extcon support the various type of external connector. This patch don't support the only USB type. > >> struct extcon_cable; >> >> /** >> @@ -167,6 +229,17 @@ extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id, >> bool cable_state); >> >> /* >> + * get/set_property access the property value of each external connector. >> + * They are used to access the property of each cable based on the property id. >> + */ >> +extern int extcon_get_property(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop, >> + union extcon_property_value *prop_val); >> +extern int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop, >> + union extcon_property_value prop_val); >> + >> +/* >> * Following APIs are to monitor every action of a notifier. >> * Registrar gets notified for every external port of a connection device. >> * Probably this could be used to debug an action of notifier; however, >> @@ -239,6 +312,19 @@ static inline int extcon_set_cable_state_(struct extcon_dev *edev, >> return 0; >> } >> >> +static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop, >> + union extcon_property_value *prop_val) >> +{ >> + return 0; >> +} >> +static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop, >> + union extcon_property_value prop_val) >> +{ >> + return 0; >> +} >> + >> static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) >> { >> return NULL; >> > Thanks, Chanwoo Choi