lkml.org 
[lkml]   [2015]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors
From
On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@ti.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@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.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


\
 
 \ /
  Last update: 2015-05-27 17:21    [W:0.105 / U:11.724 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site