From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755750AbcHBIaN (ORCPT ); Tue, 2 Aug 2016 04:30:13 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:37361 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751980AbcHBI3g (ORCPT ); Tue, 2 Aug 2016 04:29:36 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee68e-f79cb6d000006cfe-d5-57a05743275e Content-transfer-encoding: 8BIT Message-id: <57A05743.9080503@samsung.com> Date: Tue, 02 Aug 2016 17:18:11 +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 Cc: myungjoo.ham@samsung.com, zyw@rock-chips.com, groeck@chromium.org, chanwoo@kernel.org, Krzysztof Kozlowski Subject: Re: [PATCH v3 3/6] extcon: Add the support for the capability of each property References: <1470103105-5992-1-git-send-email-cw00.choi@samsung.com> <1470103105-5992-4-git-send-email-cw00.choi@samsung.com> In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrIIsWRmVeSWpSXmKPExsWyRsSkUNc5fEG4wZnPFhYTb1xhsTj1ahmz xesXhhaXd81hs7jduILNoueRlsX8BzuYHdg9ZjdcZPHYtKqTzePvrP0sHn1bVjF6HL+xncnj 8ya5ALYoLpuU1JzMstQifbsErowLX1YxFSz3q5i74C5bA+MPuy5GTg4JAROJh5P72SFsMYkL 99azdTFycQgJrGCUWN2ynR2maOuRbawQiaWMEq+PfWQCSfAKCEr8mHyPpYuRg4NZQF7iyKVs kDCzgLrEpHmLmEFsIYEHjBKLXgZBlGtJnLnYBjaTRUBV4tb7HjCbDSi+/8UNNhCbX0BR4uqP x4wgI0UFIiS6T1SChEUE7CV6Z88CO4FZoItR4t2cA2D1wgKREpe7l0Ld1swk8XD9QrDFnAJW Ep2XN4J9IyHwll2ibfU9ZojNAhLfJh8CO1pCQFZi0wFmiCclJQ6uuMEygVF8FpLXZiG8NgvJ awsYmVcxiqYWJBcUJ6UXGekVJ+YWl+al6yXn525iBMbk6X/P+nYw3jxgfYhRgINRiYdX48H8 cCHWxLLiytxDjKZAR0xklhJNzgdGfl5JvKGxmZGFqYmpsZG5pZmSOG+C1M9gIYH0xJLU7NTU gtSi+KLSnNTiQ4xMHJxSDYyTpOSCz93c/PLJsSZZ08xNzcxTHxVt9WJyUH6466rz15R7f93v 2e1+qXaJr7Hq7WTph4/UDzNsc9gwZ4dYfYbU5Ws79HTfqWb+uSmeJdNQYZNw/Nv8tYyrq/5P fffUwj5gkva2U2I3TyhL+yQJcC7ckZ2e9veVU4nL/5PBc6cVFzE0BD7ZbzVfiaU4I9FQi7mo OBEAC+tLN8QCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrHIsWRmVeSWpSXmKPExsVy+t9jAV3n8AXhBme6jCwm3rjCYnHq1TJm i9cvDC0u75rDZnG7cQWbRc8jLYv5D3YwO7B7zG64yOKxaVUnm8ffWftZPPq2rGL0OH5jO5PH 501yAWxRDYw2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6hpYW5koKeYm5qbZKLj4Bum6Z OUCnKCmUJeaUAoUCEouLlfTtME0IDXHTtYBpjND1DQmC6zEyQAMJaxgzLnxZxVSw3K9i7oK7 bA2MP+y6GDk5JARMJLYe2cYKYYtJXLi3nq2LkYtDSGApo8TrYx+ZQBK8AoISPybfY+li5OBg FpCXOHIpGyTMLKAuMWneImYQW0jgAaPEopdBEOVaEmcutrGD2CwCqhK33veA2WxA8f0vbrCB 2PwCihJXfzxmBBkpKhAh0X2iEiQsImAv0Tt7FivICcwCXYwS7+YcAKsXFoiUuNy9lBXitmYm iYfrF4It5hSwkui8vJFtAqPgLCSnzkI4dRaSUxcwMq9ilEgtSC4oTkrPNcpLLdcrTswtLs1L 10vOz93ECI77Z9I7GA/vcj/EKMDBqMTD2/FyfrgQa2JZcWXuIUYJDmYlEd5PoQvChXhTEiur Uovy44tKc1KLDzGaAj07kVlKNDkfmJLySuINjU3MjCyNzA0tjIzNlcR5H/9fFyYkkJ5Ykpqd mlqQWgTTx8TBKdXAyPxWL6d3YYX1dGldq4kBkySELSW+ruRzXvHsdpNY2ORzn5KdbYz53jaf v5S3YKlyc7nblMK2Uz/EvTTN2JOUdq9/nul1ZJWSjP+DbVLxLI8M//VXFpxU0kt72mr991iS 6BeRu3MaRV9F7Dfoae9xOWN1UNXz68YrCad27A3PDl62PHOlv62REktxRqKhFnNRcSIAunX6 ixEDAAA= 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일 17:07, Roger Quadros wrote: > Hi, > > On 02/08/16 04:58, Chanwoo Choi wrote: >> This patch adds the support of the property capability setting. This function >> decides the supported properties of each external connector on extcon provider >> driver. >> >> Ths list of new extcon APIs to get/set the capability of property as following: >> - int extcon_get_property_capability(struct extcon_dev *edev, >> unsigned int id, unsigned int prop); >> - int extcon_set_property_capability(struct extcon_dev *edev, >> unsigned int id, unsigned int prop); >> >> Signed-off-by: Chanwoo Choi >> Tested-by: Chris Zhong >> Tested-by: Guenter Roeck >> Reviewed-by: Guenter Roeck >> --- >> drivers/extcon/extcon.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/extcon.h | 22 ++++++++ >> 2 files changed, 163 insertions(+) >> >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index bb6e99fe84c8..7ae0771f29e7 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -201,6 +201,11 @@ struct extcon_cable { >> 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]; >> + >> + unsigned long usb_bits[BITS_TO_LONGS(EXTCON_PROP_USB_CNT)]; >> + unsigned long chg_bits[BITS_TO_LONGS(EXTCON_PROP_CHG_CNT)]; >> + unsigned long jack_bits[BITS_TO_LONGS(EXTCON_PROP_JACK_CNT)]; >> + unsigned long disp_bits[BITS_TO_LONGS(EXTCON_PROP_DISP_CNT)]; > > This can be replaced by a single > unsigned long cap_bits; > > As different type cables have separate extcon_cable instances. The one external connector is able to have the multiple extcon type. So, the external connector should have the [type]_bits variable. > >> }; >> >> static struct class *extcon_class; >> @@ -297,6 +302,39 @@ static bool is_extcon_property_supported(unsigned int id, unsigned int prop) >> return !!(extcon_info[id].type & type); >> } >> >> +static int is_extcon_property_capability(struct extcon_dev *edev, >> + unsigned int id, int index,unsigned int prop) >> +{ >> + struct extcon_cable *cable; >> + int type, ret; >> + >> + /* Check whether the property is supported or not. */ >> + type = get_extcon_type(prop); >> + if (type < 0) >> + return type; >> + >> + cable = &edev->cables[index]; >> + >> + switch (type) { >> + case EXTCON_TYPE_USB: >> + ret = test_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits); > > ret = test_bit(prop, cable->cap_bits); As I mentioned on previous reply, I just want to maintain the consistent style with other extcon types. > >> + break; >> + case EXTCON_TYPE_CHG: >> + ret = test_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits); >> + break; >> + case EXTCON_TYPE_JACK: >> + ret = test_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits); >> + break; >> + case EXTCON_TYPE_DISP: >> + ret = test_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> + >> static void init_property(struct extcon_dev *edev, unsigned int id, int index) >> { >> unsigned int type = extcon_info[id].type; >> @@ -554,6 +592,12 @@ int extcon_get_property(struct extcon_dev *edev, unsigned int id, >> >> spin_lock_irqsave(&edev->lock, flags); >> >> + /* Check whether the property is available or not. */ >> + if (!is_extcon_property_capability(edev, id, index, prop)) { >> + spin_unlock_irqrestore(&edev->lock, flags); >> + return -EPERM; >> + } >> + >> /* >> * Check whether the external connector is attached. >> * If external connector is detached, the user can not >> @@ -626,6 +670,12 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> >> spin_lock_irqsave(&edev->lock, flags); >> >> + /* Check whether the property is available or not. */ >> + if (!is_extcon_property_capability(edev, id, index, prop)) { >> + spin_unlock_irqrestore(&edev->lock, flags); >> + return -EPERM; >> + } >> + >> cable = &edev->cables[index]; >> >> /* Set the property value according to extcon type */ >> @@ -654,6 +704,97 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> EXPORT_SYMBOL_GPL(extcon_set_property); >> >> /** >> + * extcon_get_property_capability() - Get the capability of property >> + * of an external connector. >> + * @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. >> + * >> + * Returns 1 if the property is available or 0 if not available. >> + */ >> +int extcon_get_property_capability(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop) >> +{ >> + int index; >> + >> + 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; >> + >> + return is_extcon_property_capability(edev, id, index, prop); >> +} >> +EXPORT_SYMBOL_GPL(extcon_get_property_capability); >> + >> +/** >> + * extcon_set_property_capability() - Set the capability of a property >> + * of an external connector. >> + * @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. >> + * >> + * This function set the capability of a property for an external connector >> + * to mark the bit in capability bitmap which mean the available state of >> + * a property. >> + * >> + * Returns 0 if success or error number if fail >> + */ > > So extcon drivers must explicitly enable all properties? The extcon provider driver should enable the capability for each property. > Isn't this an unnecessary step? Why can't all properties by supported by default. > If the hardware doesn't generate that property, the driver will just not toggle it. > Can you please give an example why this won't work? The extcon support the uevent when connector is attached or detached. Also, the message of uevent will include the properties value. If we don't use the capability function, the uevent's message may include the unsupported and un-needed property information. > >> +int extcon_set_property_capability(struct extcon_dev *edev, unsigned int id, >> + unsigned int prop) >> +{ >> + struct extcon_cable *cable; >> + int index, type, 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; >> + >> + /* Check whether the property is supported or not. */ > Again? we've already checked whether property is supported or not. It is wrong description. I'll remove it. > >> + type = get_extcon_type(prop); >> + if (type < 0) >> + return type; >> + >> + cable = &edev->cables[index]; >> + >> + switch (type) { >> + case EXTCON_TYPE_USB: >> + __set_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits); >> + break; >> + case EXTCON_TYPE_CHG: >> + __set_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits); >> + break; >> + case EXTCON_TYPE_JACK: >> + __set_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits); >> + break; >> + case EXTCON_TYPE_DISP: >> + __set_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_set_property_capability); >> + >> +/** >> * 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 fe583fca7732..ffd32a8975d5 100644 >> --- a/include/linux/extcon.h >> +++ b/include/linux/extcon.h >> @@ -240,6 +240,16 @@ extern int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> union extcon_property_value prop_val); >> >> /* >> + * get/set_property_capability set the capability of the property for each >> + * external connector. They are used to set the capability of the property >> + * of each external connector based on the id and property. >> + */ >> +extern int extcon_get_property_capability(struct extcon_dev *edev, >> + unsigned int id, unsigned int prop); >> +extern int extcon_set_property_capability(struct extcon_dev *edev, >> + unsigned int id, unsigned int prop); >> + >> +/* >> * 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, >> @@ -325,6 +335,18 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id, >> return 0; >> } >> >> +static inline int extcon_get_property_capability(struct extcon_dev *edev, >> + unsigned int id, unsigned int prop) >> +{ >> + return 0; >> +} >> + >> +static inline int extcon_set_property_capability(struct extcon_dev *edev, >> + unsigned int id, unsigned int prop) >> +{ >> + return 0; >> +} >> + >> static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name) >> { >> return NULL; >> Thanks, Chanwoo Choi