Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi <cwchoi00@gmail.com>]

On Fri, May 29, 2015 at 11:32 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>
> On Fri, 2015-05-29 at 19:44 +0900, Chanwoo Choi wrote:
>> Hi Ivan,
>>
>> On 05/28/2015 05:45 PM, Ivan T. Ivanov wrote:
>> > Hi Chanwoo,
>> >
>> > On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>> > > Previously, I discussed how to inform the changed state of both ID
>> > > and VBUS pin for USB connector on patch-set[1].
>> > > [1] https://lkml.org/lkml/2015/4/2/310
>> > >
>> > > So, this patch adds the extcon_set_cable_line_state() function to inform
>> > > the additional state of external connectors without additional register/
>> > > unregister functions. This function uses the existing notifier chain
>> > > which is registered by extcon_register_notifier() / extcon_register_interest().
>> > >
>> > > The extcon_set_cable_line_state() can inform the new state of both
>> > > ID and VBUS pin state through extcon_set_cable_line_state().
>> > >
>> > > For exmaple:
>> > > - On extcon-usb-gpio.c as extcon provider driver as following:
>> > >         static void usb_extcon_detect_cable(struct work_struct *work)
>> > >         {
>> > >                 ...
>> > >                 /* check ID and update cable state */
>> > >                 id = gpiod_get_value_cansleep(info->id_gpiod);
>> > >                 if (id) {
>> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>> > >
>> > >                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>> > >                                                         EXTCON_USB_ID_HIGH);
>> >
>> > I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>> > It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>> > required information from the function one line above, do I miss something?
>>
>> The EXTCON fwk has the follwoing different functions:
>> - extcon_set_cable_state()
>> : Send whether external connectors is attached or detached to the extcon consumer driver in kernel space and to the user-space by using the uevent.
>> - extcon_set_cable_line_state()
>> : Send the specific line state of both ID and VBUS pin state of USB connector to only the extcon consumer driver in kernel space. This function don't send the uevent to the user-space because user-
>> space process don't consider the h/w pin state.
>
> My understanding, from discussion several letters back, is that clients
> will receive single event per change (EXTCON_USB_ID_HIGH, EXTCON_USB_ID_LOW...),

There are following constraints between events for EXTCON_USB:
- EXTCON_USB_ID_HIGH and EXTCON_USB_ID_LOW are mutually exclusive.
- EXTCON_USB_VBUS_HIGH and EXTCON_USB_VBUS_LOW are mutually exclusive.

If extcon provider (e.g., extcon-usb-gpio) don't violate the constraints,
extcon provider can send the multiple events as following. Namely,
extcon consumer/clients will receive the the multiple events.

For example:
extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH |
EXTCON_USB_VBUS_LOW);

>
> My point was, why we need another function (extcon_set_cable_line_state) if
> extcon_set_cable_state_ already have required information? Or the plan is to
> move extcon_set_cable_state_ to USB drivers and use only extcon_set_cable_line_state
> by extcon drivers?

You pointed out appropriately. I worried about adding new
extcon_set_cable_line_state().

I'll use extcon_set_cable_state_() without adding
extcon_set_cable_line_state() as following but it need the
modification of function prototype.

- extcon_set_cable_state(struct extcon_dev *edev, enum extcon id, bool state)
-> extcon_set_cable_state(struct extcon_dev *edev, enum extcon id, u64 state)

And extcon use the bit mask for both attached state and detached state
of external connectors instead of boolean.

#define EXTCON_DETACHED             BIT(0)
#define EXTCON_ATTACHED              BIT(1)

#define EXTCON_USB_ID_LOW          BIT(2)
#define EXTCON_USB_ID_HIGH         BIT(3)
#define EXTCON_USB_VBUS_LOW    BIT(4)
#define EXTCON_USB_VBUS_HIGH   BIT(5)

After it, extcon_set_cable_state() would send multiple events at time
as following:

e.g., extcon_set_cable_state(edev, EXTCON_USB, EXTCON_ATTACHED |
EXTCON_USB_ID_HIGH | EXTCON_USB_VBUS_HIGH);

Thanks,
Chanwoo Choi