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

[Chanwoo Choi]

Hi Ivan,

On Sat, May 30, 2015 at 2:15 AM, Chanwoo Choi <cwchoi00@xxxxxxxxx> wrote:
> On Fri, May 29, 2015 at 11:32 PM, Ivan T. Ivanov <iivanov@xxxxxxxxxx> 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);

This design has the problem.

If extcon uses only extcon_set_cable_state_(), the prototype of
extcon_{set|get}_cable_state_() functions should be changed.

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)

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

The extcon client should do the bit masking operation to get the
wanted vaule of extcon_get_cable_state_(). I think it makes the
complicated on extcon client aspect.

If extcon add the new extcon_set_cable_line_state(), the extcon client
can get the state of whether external connector is attached or
detached by just calling the extcon_get_cable_state_() without any bit
operation.

Thanks,
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/