Hi Ivan,
On Sat, May 30, 2015 at 2:15 AM, Chanwoo Choi <cwcho...@gmail.com> wrote:
> On Fri, May 29, 2015 at 11:32 PM, Ivan T. Ivanov <iiva...@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);
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
