| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGTfZH3gxfRd1RGkrs=ZfqyLy27jLwn3aSbPzxjdfj+V3Z-QQA@mail.gmail.com>
Date: Thu, 28 May 2015 00:06:14 +0900
From: Chanwoo Choi <cwchoi00@...il.com>
To: Roger Quadros <rogerq@...com>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
Robert Baldyga <r.baldyga@...sung.com>,
Peter Chen <peter.chen@...escale.com>,
Kishon Vijay Abraham I <kishon@...com>,
Felipe Balbi <balbi@...com>, iivanov@...sol.com,
"myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>
Subject: Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to
inform the additional state of external connectors
On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@...com> wrote:
> Chanwoo,
>
> On 27/05/15 15:15, Chanwoo Choi wrote:
>>
>> This patch adds the extcon_set_cable_line_state() function to inform
>> the additional state of each external connector and 'enum
>> extcon_line_state'
>> enumeration which include the specific states of each external connector.
>>
>> The each external connector might need the different line state. So,
>> current
>> 'extcon_line_state' enumeration contains the specific state for USB as
>> following:
>>
>> - Following the state mean the state of both ID and VBUS line for USB:
>> enum extcon_line_state {
>> EXTCON_USB_ID_LOW = BIT(1), /* ID line is low. */
>> EXTCON_USB_ID_HIGH = BIT(2), /* ID line is high. */
>
>
> ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit
> position.
> if the bit is set means it is high, if it is cleared means it is low.
These are the notifier event. So, extcon_line_state have to include
each event for both low or high state of ID.
because the extcon consumer driver using the
extcon_register_notifier() need the each event to distinguish the type
of event.
>
>> EXTCON_USB_VBUS_LOW = BIT(3), /* VBUS line is low. */
>> EXTCON_USB_VBUS_HIGH = BIT(4), /* VBUS line is high. */
>
>
> same here.
ditto.
>
> enum doesn't look like the right type for extcon_line_state as it is more of
> a bitmask.
Why? I prefer to use the enum if there are no problem.
>
>
>> };
>>
>> Cc: Myungjoo Ham <cw00.choi@...sung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> ---
>> drivers/extcon/extcon.c | 74
>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/extcon.h | 24 ++++++++++++++++
>> 2 files changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 5099c11..142bf0f 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -279,7 +279,9 @@ int extcon_update_state(struct extcon_dev *edev, u32
>> mask, u32 state)
>>
>> for (index = 0; index < edev->max_supported; index++) {
>> if (is_extcon_changed(edev->state, state, index,
>> &attached))
>> - raw_notifier_call_chain(&edev->nh[index],
>> attached, edev);
>> + raw_notifier_call_chain(&edev->nh[index],
>> + attached ? EXTCON_ATTACHED :
>> + EXTCON_DETACHED, edev);
>> }
>>
>> edev->state &= ~mask;
>> @@ -418,6 +420,69 @@ int extcon_set_cable_state(struct extcon_dev *edev,
>> EXPORT_SYMBOL_GPL(extcon_set_cable_state);
>>
>> /**
>> + * extcon_set_cable_line_state() - Set the line state of specific cable.
>> + * @edev: the extcon device that has the cable.
>> + * @id: the unique id of each external connector.
>> + * @state: the line state for specific cable.
>> + *
>> + * Note that this function support the only USB connector to inform the
>> state
>> + * of both ID and VBUS line until now. This function may be extended to
>> support
>> + * the additional external connectors.
>> + *
>> + * If the id is EXTCON_USB, it can support only following line states:
>> + * - EXTCON_USB_ID_LOW
>> + * - EXTCON_USB_ID_HIGH,
>> + * - EXTCON_USB_VBUS_LOW
>> + * - EXTCON_USB_VBUS_HIGH
>> + */
>> +int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id,
>> + enum extcon_line_state state)
>> +{
>> + unsigned long flags;
>> + unsigned long line_state;
>> + int ret = 0, index;
>> +
>> + index = find_cable_index_by_id(edev, id);
>> + if (index < 0)
>> + return index;
>> +
>> + spin_lock_irqsave(&edev->lock, flags);
>> + line_state = edev->line_state[index];
>> +
>> + switch (id) {
>> + case EXTCON_USB:
>> + if (line_state & state) {
>> + dev_warn(&edev->dev,
>> + "0x%x state is already set for %s\n",
>> + state, extcon_name[id]);
>> + goto err;
>> + }
>> +
>> + if ((state & EXTCON_USB_ID_LOW) || (state &
>> EXTCON_USB_ID_HIGH))
>> + line_state &= ~(EXTCON_USB_ID_LOW |
>> EXTCON_USB_ID_HIGH);
>> +
>> + if ((state & EXTCON_USB_VBUS_LOW)
>> + || (state & EXTCON_USB_VBUS_HIGH))
>> + line_state &=
>> + ~(EXTCON_USB_VBUS_LOW |
>> EXTCON_USB_VBUS_HIGH);
>> +
>> + line_state |= state;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + goto err;
>> + }
>> + edev->line_state[index] = line_state;
>> +
>> + ret = raw_notifier_call_chain(&edev->nh[index], line_state, edev);
>> +err:
>> + spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_set_cable_line_state);
>> +
>> +/**
>> * extcon_get_extcon_dev() - Get the extcon device instance from the
>> name
>> * @extcon_name: The extcon name provided with
>> extcon_dev_register()
>> */
>> @@ -897,6 +962,13 @@ int extcon_dev_register(struct extcon_dev *edev)
>> goto err_dev;
>> }
>>
>> + edev->line_state = devm_kzalloc(&edev->dev,
>> + sizeof(*edev->line_state) * edev->max_supported,
>> GFP_KERNEL);
>> + if (!edev->line_state) {
>> + ret = -ENOMEM;
>> + goto err_dev;
>> + }
>> +
>> for (index = 0; index < edev->max_supported; index++)
>> RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index be9652b..79e5073 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -66,6 +66,19 @@ enum extcon {
>> EXTCON_END,
>> };
>>
>> +enum extcon_line_state {
>> + /* Following two definition are used for whether external
>> connectors
>> + * is attached or detached. */
>> + EXTCON_DETACHED = 0x0,
>> + EXTCON_ATTACHED = 0x1,
>> +
>> + /* Following states are only used for EXTCON_USB. */
>> + EXTCON_USB_ID_LOW = BIT(1), /* ID line is low. */
>> + EXTCON_USB_ID_HIGH = BIT(2), /* ID line is high. */
>> + EXTCON_USB_VBUS_LOW = BIT(3), /* VBUS line is low. */
>> + EXTCON_USB_VBUS_HIGH = BIT(4), /* VBUS line is high. */
>> +};
>
>
> Why do you use enum? How about the following bit definitions for line state.
>
> #define EXTCON_ATTACHED_DETACHED_BIT BIT(0)
> #define EXTCON_USB_ID_BIT BIT(1)
> #define EXTCON_USB_VBUS_BIT BIT(2)
> ...
>
> code must check if appropriate bit is set or cleared to get the high/low
> state.
I answer about it on upper.
Cheers,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists