Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Chanwoo Choi]

On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@xxxxxx> 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@xxxxxxxxxxx>
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> ---
>>   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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/