Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611633 > unrolled thread

Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev

Started byChanwoo Choi <cw00.choi@samsung.com>
First post2017-03-29 09:10 +0200
Last post2017-03-29 12:00 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] extcon: Allow registering a single notifier for all  cables on an extcon_dev Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-29 09:10 +0200
    Re: [PATCH v2] extcon: Allow registering a single notifier for all  cables on an extcon_dev Hans de Goede <hdegoede@redhat.com> - 2017-03-29 09:40 +0200
      Re: [PATCH v2] extcon: Allow registering a single notifier for all  cables on an extcon_dev Chanwoo Choi <cw00.choi@samsung.com> - 2017-03-29 12:00 +0200
        Re: [PATCH v2] extcon: Allow registering a single notifier for all  cables on an extcon_dev Hans de Goede <hdegoede@redhat.com> - 2017-03-29 12:00 +0200

#1611633 — Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev

FromChanwoo Choi <cw00.choi@samsung.com>
Date2017-03-29 09:10 +0200
SubjectRe: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev
Message-ID<tqfTY-3sX-9@gated-at.bofh.it>
Hi,

On 2017년 03월 25일 23:03, Hans de Goede wrote:
> Hi,
> 
> On 20-03-17 10:48, Hans de Goede wrote:
>> Hi,
>>
>> On 20-03-17 01:55, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 2017년 03월 20일 03:23, Hans de Goede wrote:
>>>> In some cases a driver may want to monitor multiple cables on a single
>>>> extcon. For example a charger driver will typically want to monitor all
>>>> of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure
>>>> the max. current it can sink while charging.
>>>>
>>>> Due to the signature of the notifier_call function + how extcon passes
>>>> state and the extcon_dev as parameters this requires using one
>>>> notifier_block + one notifier_call function per cable, otherwise the
>>>> notifier_call function cannot get to its driver's data (using container_of
>>>> requires it to know which notifier block its first parameter is).
>>>>
>>>> For a driver wanting to monitor the above 3 cables that would result
>>>> in something like this:
>>>>
>>>> static const unsigned int vbus_cable_ids[] = {
>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>
>>>> struct driver_data {
>>>>     struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)];
>>>> }
>>>>
>>>> /*
>>>>  * We need 3 copies of this, because there is no way to find out for which
>>>>  * cable id we are being called from the passed in arguments; and we must
>>>>  * have a separate nb for each extcon_register_notifier call.
>>>>  */
>>>> static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>> {
>>>>     struct driver_data *data =
>>>>         container_of(nb, struct driver_data, vbus_nb[0]);
>>>>     ...
>>>> }
>>>>
>>>> static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>> {
>>>>     struct driver_data *data =
>>>>         container_of(nb, struct driver_data, vbus_nb[1]);
>>>>     ...
>>>> }
>>>>
>>>> static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>> {
>>>>     struct driver_data *data =
>>>>         container_of(nb, struct driver_data, vbus_nb[2]);
>>>>     ...
>>>> }
>>>>
>>>> int probe(...)
>>>> {
>>>>     /* Register for vbus notification */
>>>>     data->vbus_nb[0].notifier_call = vbus_cable0_evt;
>>>>     data->vbus_nb[1].notifier_call = vbus_cable1_evt;
>>>>     data->vbus_nb[2].notifier_call = vbus_cable2_evt;
>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>                     vbus_cable_ids[i], &data->vbus_nb[i]);
>>>>         if (ret)
>>>>             ...
>>>>     }
>>>> }
>>>
>>> You can get the notification for multiple external connector
>>> by using the only one notifier_block as following:
>>>
>>> static const unsigned int vbus_cable_ids[] = {
>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>
>>> struct driver_data {
>>>     struct notifier_block vbus_nb;
>>> }
>>>
>>> static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p)
>>> {
>>>     struct driver_data *data =
>>>         container_of(nb, struct driver_data, vbus_nb);
>>>     int sdp_state, cdp_state, dcp_state;
>>>
>>>     sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP);
>>>     cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP);
>>>     dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP);
>>>
>>>     ...
>>> }
>>>
>>> int probe(...)
>>> {
>>>     /* Register for vbus notification */
>>>     data->vbus_nb.notifier_call = vbus_cable_evt;
>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>                     vbus_cable_ids[i], &data->vbus_nb);
>>>         if (ret)
>>>             ...
>>>     }
>>> }
>>
>> No that will not work, you cannot add the same notifier_block to 3
>> different notifier_heads, the list_head can only be part of one list!
>>
>> Also see:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>
>> Which actually fixes an issue I encountered exactly because of doing
>> the above.
>>
>> So we really need something like my patch to make this simpler
>> and to remove the ugliness we currently have in axp288_charger.c
>> because of this problem (and are about to have in more drivers
>> I'm upstreaming). Also note that my patch is quite simple, the
>> amount of lines it adds is way less then the amount of lines
>> it will allow to remove in a single driver!
> 
> Ping ?
> 
> As explained this is a real problem and we really need a proper solution
> for this. I'm about to upstream 2 more drivers which need to monitor
> an antire extcon rather then a single cable on it, and I do not want
> to have to add hacks like this:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
> 
> To both drivers. My patch is IMHO a nice and clean solution for this,
> can you please merge this or provide an alternative solution ?


As you said, when using one notifier_block for multiple external connector,
it has a problem. But, it is not only for extcon framework. It depends
on the notifier_block code. If each external connector uses the
each notifier_block (it is a not hack), there is no any problem.

As I already said on my reply[1] as following,
---------------------------------------------
The extcon_register_notifier() function must support the notification
for only one external connector.
[1] https://www.spinics.net/lists/kernel/msg2468906.html
---------------------------------------------

I agree that the EXTCON needs to support the notification
for all supported external connector of extcon device
by using only one notifier_chain.

I don't prefer to use the '-1' as the unique id for getting the
notification for all external connectors. Each argument of function
must have the correct meaning.

I think that thanks for your report and suggestion.

I'm considering to add the new function as following:
Following function will support the all external connector for notification
and the function argument don't include the unique id for external connector.
- extcon_regiter_all_notifier(struct extcon_dev *edev, struct notifier_block *nb);
(The function name is not fixed)


> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>>>>
>>>> And then in the event handling the driver often checks the state of
>>>> all cables explicitly using extcon_get_state, rather then using the
>>>> event argument to the notifier_call.
>>>>
>>>> This commit makes extcon_[un]register_notifier accept -1 as cable-id,
>>>> which will cause the notifier to get called for changes on any cable
>>>> on the extcon_dev. Compared to the above example code this allows much
>>>> simpler code in drivers which want to monitor multiple cable types.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Changes in v2:
>>>> -Fix false-positive "warning: 'idx' may be used uninitialized" warning
>>>>  seen with some compiler versions
>>>> ---
>>>>  drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++---------
>>>>  drivers/extcon/extcon.h |  1 +
>>>>  2 files changed, 27 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>> index 09ac5e7..2a042ee 100644
>>>> --- a/drivers/extcon/extcon.c
>>>> +++ b/drivers/extcon/extcon.c
>>>> @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>>
>>>>      state = !!(edev->state & BIT(index));
>>>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>> +    raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>>>
>>>>      /* This could be in interrupt handler */
>>>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>>> @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>>>   *                any attach status changes from the extcon.
>>>>   * @edev:    the extcon device that has the external connecotr.
>>>>   * @id:        the unique id of each external connector in extcon enumeration.
>>>> + *        or -1 to get notififications for all cables on edev, in this
>>>> + *        case no state info will get passed to the notifier_call.
>>>>   * @nb:        a notifier block to be registered.
>>>>   *
>>>>   * Note that the second parameter given to the callback of nb (val) is
>>>> @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>>      if (!edev || !nb)
>>>>          return -EINVAL;
>>>>
>>>> -    idx = find_cable_index_by_id(edev, id);
>>>> -    if (idx < 0)
>>>> -        return idx;
>>>> +    if ((int)id != -1) {
>>>> +        idx = find_cable_index_by_id(edev, id);
>>>> +        if (idx < 0)
>>>> +            return idx;
>>>> +    }
>>>>
>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>> -    ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>> +    if ((int)id != -1)
>>>> +        ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>> +    else
>>>> +        ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>>> +
>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>
>>>>      return ret;
>>>> @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>>                  struct notifier_block *nb)
>>>>  {
>>>>      unsigned long flags;
>>>> -    int ret, idx;
>>>> +    int ret, idx = 0;
>>>>
>>>>      if (!edev || !nb)
>>>>          return -EINVAL;
>>>>
>>>> -    idx = find_cable_index_by_id(edev, id);
>>>> -    if (idx < 0)
>>>> -        return idx;
>>>> +    if ((int)id != -1) {
>>>> +        idx = find_cable_index_by_id(edev, id);
>>>> +        if (idx < 0)
>>>> +            return idx;
>>>> +    }
>>>>
>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>> -    ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>> +    if ((int)id != -1)
>>>> +        ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>> +    else
>>>> +        ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>>> +
>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>
>>>>      return ret;
>>>> @@ -1212,6 +1227,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>>>      for (index = 0; index < edev->max_supported; index++)
>>>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>>
>>>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>>> +
>>>>      dev_set_drvdata(&edev->dev, edev);
>>>>      edev->state = 0;
>>>>
>>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>>> index 993ddcc..2e6c09d 100644
>>>> --- a/drivers/extcon/extcon.h
>>>> +++ b/drivers/extcon/extcon.h
>>>> @@ -44,6 +44,7 @@ struct extcon_dev {
>>>>      /* Internal data. Please do not set. */
>>>>      struct device dev;
>>>>      struct raw_notifier_head *nh;
>>>> +    struct raw_notifier_head nh_all;
>>>>      struct list_head entry;
>>>>      int max_supported;
>>>>      spinlock_t lock;    /* could be called by irq handler */
>>>>
>>>
>>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[toc] | [next] | [standalone]

#1611656

FromHans de Goede <hdegoede@redhat.com>
Date2017-03-29 09:40 +0200
Message-ID<tqgmZ-3Eh-11@gated-at.bofh.it>
In reply to#1611633
Hi,

On 29-03-17 09:06, Chanwoo Choi wrote:
> Hi,
>
> On 2017년 03월 25일 23:03, Hans de Goede wrote:
>> Hi,
>>
>> On 20-03-17 10:48, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 20-03-17 01:55, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2017년 03월 20일 03:23, Hans de Goede wrote:
>>>>> In some cases a driver may want to monitor multiple cables on a single
>>>>> extcon. For example a charger driver will typically want to monitor all
>>>>> of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure
>>>>> the max. current it can sink while charging.
>>>>>
>>>>> Due to the signature of the notifier_call function + how extcon passes
>>>>> state and the extcon_dev as parameters this requires using one
>>>>> notifier_block + one notifier_call function per cable, otherwise the
>>>>> notifier_call function cannot get to its driver's data (using container_of
>>>>> requires it to know which notifier block its first parameter is).
>>>>>
>>>>> For a driver wanting to monitor the above 3 cables that would result
>>>>> in something like this:
>>>>>
>>>>> static const unsigned int vbus_cable_ids[] = {
>>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>>
>>>>> struct driver_data {
>>>>>     struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)];
>>>>> }
>>>>>
>>>>> /*
>>>>>  * We need 3 copies of this, because there is no way to find out for which
>>>>>  * cable id we are being called from the passed in arguments; and we must
>>>>>  * have a separate nb for each extcon_register_notifier call.
>>>>>  */
>>>>> static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>> {
>>>>>     struct driver_data *data =
>>>>>         container_of(nb, struct driver_data, vbus_nb[0]);
>>>>>     ...
>>>>> }
>>>>>
>>>>> static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>> {
>>>>>     struct driver_data *data =
>>>>>         container_of(nb, struct driver_data, vbus_nb[1]);
>>>>>     ...
>>>>> }
>>>>>
>>>>> static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>> {
>>>>>     struct driver_data *data =
>>>>>         container_of(nb, struct driver_data, vbus_nb[2]);
>>>>>     ...
>>>>> }
>>>>>
>>>>> int probe(...)
>>>>> {
>>>>>     /* Register for vbus notification */
>>>>>     data->vbus_nb[0].notifier_call = vbus_cable0_evt;
>>>>>     data->vbus_nb[1].notifier_call = vbus_cable1_evt;
>>>>>     data->vbus_nb[2].notifier_call = vbus_cable2_evt;
>>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>>                     vbus_cable_ids[i], &data->vbus_nb[i]);
>>>>>         if (ret)
>>>>>             ...
>>>>>     }
>>>>> }
>>>>
>>>> You can get the notification for multiple external connector
>>>> by using the only one notifier_block as following:
>>>>
>>>> static const unsigned int vbus_cable_ids[] = {
>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>
>>>> struct driver_data {
>>>>     struct notifier_block vbus_nb;
>>>> }
>>>>
>>>> static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>> {
>>>>     struct driver_data *data =
>>>>         container_of(nb, struct driver_data, vbus_nb);
>>>>     int sdp_state, cdp_state, dcp_state;
>>>>
>>>>     sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP);
>>>>     cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP);
>>>>     dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP);
>>>>
>>>>     ...
>>>> }
>>>>
>>>> int probe(...)
>>>> {
>>>>     /* Register for vbus notification */
>>>>     data->vbus_nb.notifier_call = vbus_cable_evt;
>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>                     vbus_cable_ids[i], &data->vbus_nb);
>>>>         if (ret)
>>>>             ...
>>>>     }
>>>> }
>>>
>>> No that will not work, you cannot add the same notifier_block to 3
>>> different notifier_heads, the list_head can only be part of one list!
>>>
>>> Also see:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>>
>>> Which actually fixes an issue I encountered exactly because of doing
>>> the above.
>>>
>>> So we really need something like my patch to make this simpler
>>> and to remove the ugliness we currently have in axp288_charger.c
>>> because of this problem (and are about to have in more drivers
>>> I'm upstreaming). Also note that my patch is quite simple, the
>>> amount of lines it adds is way less then the amount of lines
>>> it will allow to remove in a single driver!
>>
>> Ping ?
>>
>> As explained this is a real problem and we really need a proper solution
>> for this. I'm about to upstream 2 more drivers which need to monitor
>> an antire extcon rather then a single cable on it, and I do not want
>> to have to add hacks like this:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>
>> To both drivers. My patch is IMHO a nice and clean solution for this,
>> can you please merge this or provide an alternative solution ?
>
>
> As you said, when using one notifier_block for multiple external connector,
> it has a problem. But, it is not only for extcon framework. It depends
> on the notifier_block code. If each external connector uses the
> each notifier_block (it is a not hack), there is no any problem.
>
> As I already said on my reply[1] as following,
> ---------------------------------------------
> The extcon_register_notifier() function must support the notification
> for only one external connector.
> [1] https://www.spinics.net/lists/kernel/msg2468906.html
> ---------------------------------------------
>
> I agree that the EXTCON needs to support the notification
> for all supported external connector of extcon device
> by using only one notifier_chain.
>
> I don't prefer to use the '-1' as the unique id for getting the
> notification for all external connectors. Each argument of function
> must have the correct meaning.
>
> I think that thanks for your report and suggestion.
>
> I'm considering to add the new function as following:
> Following function will support the all external connector for notification
> and the function argument don't include the unique id for external connector.
> - extcon_regiter_all_notifier(struct extcon_dev *edev, struct notifier_block *nb);
> (The function name is not fixed)

Adding a new extcon_register_all_notifier function for this is fine with
me, shall I send a new patch for this ?

Regards,

Hans




>
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>>>
>>>>> And then in the event handling the driver often checks the state of
>>>>> all cables explicitly using extcon_get_state, rather then using the
>>>>> event argument to the notifier_call.
>>>>>
>>>>> This commit makes extcon_[un]register_notifier accept -1 as cable-id,
>>>>> which will cause the notifier to get called for changes on any cable
>>>>> on the extcon_dev. Compared to the above example code this allows much
>>>>> simpler code in drivers which want to monitor multiple cable types.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> -Fix false-positive "warning: 'idx' may be used uninitialized" warning
>>>>>  seen with some compiler versions
>>>>> ---
>>>>>  drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++---------
>>>>>  drivers/extcon/extcon.h |  1 +
>>>>>  2 files changed, 27 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>>> index 09ac5e7..2a042ee 100644
>>>>> --- a/drivers/extcon/extcon.c
>>>>> +++ b/drivers/extcon/extcon.c
>>>>> @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>>>
>>>>>      state = !!(edev->state & BIT(index));
>>>>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>>> +    raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>>>>
>>>>>      /* This could be in interrupt handler */
>>>>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>>>> @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>>>>   *                any attach status changes from the extcon.
>>>>>   * @edev:    the extcon device that has the external connecotr.
>>>>>   * @id:        the unique id of each external connector in extcon enumeration.
>>>>> + *        or -1 to get notififications for all cables on edev, in this
>>>>> + *        case no state info will get passed to the notifier_call.
>>>>>   * @nb:        a notifier block to be registered.
>>>>>   *
>>>>>   * Note that the second parameter given to the callback of nb (val) is
>>>>> @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>>>      if (!edev || !nb)
>>>>>          return -EINVAL;
>>>>>
>>>>> -    idx = find_cable_index_by_id(edev, id);
>>>>> -    if (idx < 0)
>>>>> -        return idx;
>>>>> +    if ((int)id != -1) {
>>>>> +        idx = find_cable_index_by_id(edev, id);
>>>>> +        if (idx < 0)
>>>>> +            return idx;
>>>>> +    }
>>>>>
>>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>> -    ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>>> +    if ((int)id != -1)
>>>>> +        ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>>> +    else
>>>>> +        ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>>>> +
>>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>>
>>>>>      return ret;
>>>>> @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>>>                  struct notifier_block *nb)
>>>>>  {
>>>>>      unsigned long flags;
>>>>> -    int ret, idx;
>>>>> +    int ret, idx = 0;
>>>>>
>>>>>      if (!edev || !nb)
>>>>>          return -EINVAL;
>>>>>
>>>>> -    idx = find_cable_index_by_id(edev, id);
>>>>> -    if (idx < 0)
>>>>> -        return idx;
>>>>> +    if ((int)id != -1) {
>>>>> +        idx = find_cable_index_by_id(edev, id);
>>>>> +        if (idx < 0)
>>>>> +            return idx;
>>>>> +    }
>>>>>
>>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>> -    ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>>> +    if ((int)id != -1)
>>>>> +        ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>>> +    else
>>>>> +        ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>>>> +
>>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>>
>>>>>      return ret;
>>>>> @@ -1212,6 +1227,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>>>>      for (index = 0; index < edev->max_supported; index++)
>>>>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>>>
>>>>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>>>> +
>>>>>      dev_set_drvdata(&edev->dev, edev);
>>>>>      edev->state = 0;
>>>>>
>>>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>>>> index 993ddcc..2e6c09d 100644
>>>>> --- a/drivers/extcon/extcon.h
>>>>> +++ b/drivers/extcon/extcon.h
>>>>> @@ -44,6 +44,7 @@ struct extcon_dev {
>>>>>      /* Internal data. Please do not set. */
>>>>>      struct device dev;
>>>>>      struct raw_notifier_head *nh;
>>>>> +    struct raw_notifier_head nh_all;
>>>>>      struct list_head entry;
>>>>>      int max_supported;
>>>>>      spinlock_t lock;    /* could be called by irq handler */
>>>>>
>>>>
>>>>
>>
>>
>
>

[toc] | [prev] | [next] | [standalone]

#1611764

FromChanwoo Choi <cw00.choi@samsung.com>
Date2017-03-29 12:00 +0200
Message-ID<tqiyu-55o-9@gated-at.bofh.it>
In reply to#1611656
Hi,

On 2017년 03월 29일 16:38, Hans de Goede wrote:
> Hi,
> 
> On 29-03-17 09:06, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2017년 03월 25일 23:03, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 20-03-17 10:48, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 20-03-17 01:55, Chanwoo Choi wrote:
>>>>> Hi,
>>>>>
>>>>> On 2017년 03월 20일 03:23, Hans de Goede wrote:
>>>>>> In some cases a driver may want to monitor multiple cables on a single
>>>>>> extcon. For example a charger driver will typically want to monitor all
>>>>>> of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure
>>>>>> the max. current it can sink while charging.
>>>>>>
>>>>>> Due to the signature of the notifier_call function + how extcon passes
>>>>>> state and the extcon_dev as parameters this requires using one
>>>>>> notifier_block + one notifier_call function per cable, otherwise the
>>>>>> notifier_call function cannot get to its driver's data (using container_of
>>>>>> requires it to know which notifier block its first parameter is).
>>>>>>
>>>>>> For a driver wanting to monitor the above 3 cables that would result
>>>>>> in something like this:
>>>>>>
>>>>>> static const unsigned int vbus_cable_ids[] = {
>>>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>>>
>>>>>> struct driver_data {
>>>>>>     struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)];
>>>>>> }
>>>>>>
>>>>>> /*
>>>>>>  * We need 3 copies of this, because there is no way to find out for which
>>>>>>  * cable id we are being called from the passed in arguments; and we must
>>>>>>  * have a separate nb for each extcon_register_notifier call.
>>>>>>  */
>>>>>> static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>> {
>>>>>>     struct driver_data *data =
>>>>>>         container_of(nb, struct driver_data, vbus_nb[0]);
>>>>>>     ...
>>>>>> }
>>>>>>
>>>>>> static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>> {
>>>>>>     struct driver_data *data =
>>>>>>         container_of(nb, struct driver_data, vbus_nb[1]);
>>>>>>     ...
>>>>>> }
>>>>>>
>>>>>> static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>> {
>>>>>>     struct driver_data *data =
>>>>>>         container_of(nb, struct driver_data, vbus_nb[2]);
>>>>>>     ...
>>>>>> }
>>>>>>
>>>>>> int probe(...)
>>>>>> {
>>>>>>     /* Register for vbus notification */
>>>>>>     data->vbus_nb[0].notifier_call = vbus_cable0_evt;
>>>>>>     data->vbus_nb[1].notifier_call = vbus_cable1_evt;
>>>>>>     data->vbus_nb[2].notifier_call = vbus_cable2_evt;
>>>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>>>                     vbus_cable_ids[i], &data->vbus_nb[i]);
>>>>>>         if (ret)
>>>>>>             ...
>>>>>>     }
>>>>>> }
>>>>>
>>>>> You can get the notification for multiple external connector
>>>>> by using the only one notifier_block as following:
>>>>>
>>>>> static const unsigned int vbus_cable_ids[] = {
>>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>>
>>>>> struct driver_data {
>>>>>     struct notifier_block vbus_nb;
>>>>> }
>>>>>
>>>>> static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>> {
>>>>>     struct driver_data *data =
>>>>>         container_of(nb, struct driver_data, vbus_nb);
>>>>>     int sdp_state, cdp_state, dcp_state;
>>>>>
>>>>>     sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP);
>>>>>     cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP);
>>>>>     dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP);
>>>>>
>>>>>     ...
>>>>> }
>>>>>
>>>>> int probe(...)
>>>>> {
>>>>>     /* Register for vbus notification */
>>>>>     data->vbus_nb.notifier_call = vbus_cable_evt;
>>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>>                     vbus_cable_ids[i], &data->vbus_nb);
>>>>>         if (ret)
>>>>>             ...
>>>>>     }
>>>>> }
>>>>
>>>> No that will not work, you cannot add the same notifier_block to 3
>>>> different notifier_heads, the list_head can only be part of one list!
>>>>
>>>> Also see:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>>>
>>>> Which actually fixes an issue I encountered exactly because of doing
>>>> the above.
>>>>
>>>> So we really need something like my patch to make this simpler
>>>> and to remove the ugliness we currently have in axp288_charger.c
>>>> because of this problem (and are about to have in more drivers
>>>> I'm upstreaming). Also note that my patch is quite simple, the
>>>> amount of lines it adds is way less then the amount of lines
>>>> it will allow to remove in a single driver!
>>>
>>> Ping ?
>>>
>>> As explained this is a real problem and we really need a proper solution
>>> for this. I'm about to upstream 2 more drivers which need to monitor
>>> an antire extcon rather then a single cable on it, and I do not want
>>> to have to add hacks like this:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>>
>>> To both drivers. My patch is IMHO a nice and clean solution for this,
>>> can you please merge this or provide an alternative solution ?
>>
>>
>> As you said, when using one notifier_block for multiple external connector,
>> it has a problem. But, it is not only for extcon framework. It depends
>> on the notifier_block code. If each external connector uses the
>> each notifier_block (it is a not hack), there is no any problem.
>>
>> As I already said on my reply[1] as following,
>> ---------------------------------------------
>> The extcon_register_notifier() function must support the notification
>> for only one external connector.
>> [1] https://www.spinics.net/lists/kernel/msg2468906.html
>> ---------------------------------------------
>>
>> I agree that the EXTCON needs to support the notification
>> for all supported external connector of extcon device
>> by using only one notifier_chain.
>>
>> I don't prefer to use the '-1' as the unique id for getting the
>> notification for all external connectors. Each argument of function
>> must have the correct meaning.
>>
>> I think that thanks for your report and suggestion.
>>
>> I'm considering to add the new function as following:
>> Following function will support the all external connector for notification
>> and the function argument don't include the unique id for external connector.
>> - extcon_regiter_all_notifier(struct extcon_dev *edev, struct notifier_block *nb);
>> (The function name is not fixed)
> 
> Adding a new extcon_register_all_notifier function for this is fine with
> me, shall I send a new patch for this ?

I'll develop the new following functions with your reported-by.

int extcon_register_notifier_all(struct extcon_dev *edev,
                               struct notifier_block *nb);
int extcon_unregister_notifier_all(struct extcon_dev *edev,
                               struct notifier_block *nb);
int devm_extcon_register_notifier_all(struct device *dev,
                               struct extcon_dev *edev,
                               struct notifier_block *nb);
void devm_extcon_unregister_notifier_all(struct device *dev,
                               struct extcon_dev *edev,
                               struct notifier_block *nb);


> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>>>
>>>>>> And then in the event handling the driver often checks the state of
>>>>>> all cables explicitly using extcon_get_state, rather then using the
>>>>>> event argument to the notifier_call.
>>>>>>
>>>>>> This commit makes extcon_[un]register_notifier accept -1 as cable-id,
>>>>>> which will cause the notifier to get called for changes on any cable
>>>>>> on the extcon_dev. Compared to the above example code this allows much
>>>>>> simpler code in drivers which want to monitor multiple cable types.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> -Fix false-positive "warning: 'idx' may be used uninitialized" warning
>>>>>>  seen with some compiler versions
>>>>>> ---
>>>>>>  drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++---------
>>>>>>  drivers/extcon/extcon.h |  1 +
>>>>>>  2 files changed, 27 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>>>> index 09ac5e7..2a042ee 100644
>>>>>> --- a/drivers/extcon/extcon.c
>>>>>> +++ b/drivers/extcon/extcon.c
>>>>>> @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>>>>
>>>>>>      state = !!(edev->state & BIT(index));
>>>>>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>>>> +    raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>>>>>
>>>>>>      /* This could be in interrupt handler */
>>>>>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>>>>> @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>>>>>   *                any attach status changes from the extcon.
>>>>>>   * @edev:    the extcon device that has the external connecotr.
>>>>>>   * @id:        the unique id of each external connector in extcon enumeration.
>>>>>> + *        or -1 to get notififications for all cables on edev, in this
>>>>>> + *        case no state info will get passed to the notifier_call.
>>>>>>   * @nb:        a notifier block to be registered.
>>>>>>   *
>>>>>>   * Note that the second parameter given to the callback of nb (val) is
>>>>>> @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>>>>      if (!edev || !nb)
>>>>>>          return -EINVAL;
>>>>>>
>>>>>> -    idx = find_cable_index_by_id(edev, id);
>>>>>> -    if (idx < 0)
>>>>>> -        return idx;
>>>>>> +    if ((int)id != -1) {
>>>>>> +        idx = find_cable_index_by_id(edev, id);
>>>>>> +        if (idx < 0)
>>>>>> +            return idx;
>>>>>> +    }
>>>>>>
>>>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>>> -    ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>>>> +    if ((int)id != -1)
>>>>>> +        ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>>>> +    else
>>>>>> +        ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>>>>> +
>>>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>>>
>>>>>>      return ret;
>>>>>> @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>>>>                  struct notifier_block *nb)
>>>>>>  {
>>>>>>      unsigned long flags;
>>>>>> -    int ret, idx;
>>>>>> +    int ret, idx = 0;
>>>>>>
>>>>>>      if (!edev || !nb)
>>>>>>          return -EINVAL;
>>>>>>
>>>>>> -    idx = find_cable_index_by_id(edev, id);
>>>>>> -    if (idx < 0)
>>>>>> -        return idx;
>>>>>> +    if ((int)id != -1) {
>>>>>> +        idx = find_cable_index_by_id(edev, id);
>>>>>> +        if (idx < 0)
>>>>>> +            return idx;
>>>>>> +    }
>>>>>>
>>>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>>> -    ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>>>> +    if ((int)id != -1)
>>>>>> +        ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>>>> +    else
>>>>>> +        ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>>>>> +
>>>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>>>
>>>>>>      return ret;
>>>>>> @@ -1212,6 +1227,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>>>>>      for (index = 0; index < edev->max_supported; index++)
>>>>>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>>>>
>>>>>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>>>>> +
>>>>>>      dev_set_drvdata(&edev->dev, edev);
>>>>>>      edev->state = 0;
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>>>>> index 993ddcc..2e6c09d 100644
>>>>>> --- a/drivers/extcon/extcon.h
>>>>>> +++ b/drivers/extcon/extcon.h
>>>>>> @@ -44,6 +44,7 @@ struct extcon_dev {
>>>>>>      /* Internal data. Please do not set. */
>>>>>>      struct device dev;
>>>>>>      struct raw_notifier_head *nh;
>>>>>> +    struct raw_notifier_head nh_all;
>>>>>>      struct list_head entry;
>>>>>>      int max_supported;
>>>>>>      spinlock_t lock;    /* could be called by irq handler */
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[toc] | [prev] | [next] | [standalone]

#1611767

FromHans de Goede <hdegoede@redhat.com>
Date2017-03-29 12:00 +0200
Message-ID<tqiyu-55o-19@gated-at.bofh.it>
In reply to#1611764
HI,

On 29-03-17 11:50, Chanwoo Choi wrote:
> Hi,
>
> On 2017년 03월 29일 16:38, Hans de Goede wrote:
>> Hi,
>>
>> On 29-03-17 09:06, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 2017년 03월 25일 23:03, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 20-03-17 10:48, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 20-03-17 01:55, Chanwoo Choi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2017년 03월 20일 03:23, Hans de Goede wrote:
>>>>>>> In some cases a driver may want to monitor multiple cables on a single
>>>>>>> extcon. For example a charger driver will typically want to monitor all
>>>>>>> of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure
>>>>>>> the max. current it can sink while charging.
>>>>>>>
>>>>>>> Due to the signature of the notifier_call function + how extcon passes
>>>>>>> state and the extcon_dev as parameters this requires using one
>>>>>>> notifier_block + one notifier_call function per cable, otherwise the
>>>>>>> notifier_call function cannot get to its driver's data (using container_of
>>>>>>> requires it to know which notifier block its first parameter is).
>>>>>>>
>>>>>>> For a driver wanting to monitor the above 3 cables that would result
>>>>>>> in something like this:
>>>>>>>
>>>>>>> static const unsigned int vbus_cable_ids[] = {
>>>>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>>>>
>>>>>>> struct driver_data {
>>>>>>>     struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)];
>>>>>>> }
>>>>>>>
>>>>>>> /*
>>>>>>>  * We need 3 copies of this, because there is no way to find out for which
>>>>>>>  * cable id we are being called from the passed in arguments; and we must
>>>>>>>  * have a separate nb for each extcon_register_notifier call.
>>>>>>>  */
>>>>>>> static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>>> {
>>>>>>>     struct driver_data *data =
>>>>>>>         container_of(nb, struct driver_data, vbus_nb[0]);
>>>>>>>     ...
>>>>>>> }
>>>>>>>
>>>>>>> static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>>> {
>>>>>>>     struct driver_data *data =
>>>>>>>         container_of(nb, struct driver_data, vbus_nb[1]);
>>>>>>>     ...
>>>>>>> }
>>>>>>>
>>>>>>> static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>>> {
>>>>>>>     struct driver_data *data =
>>>>>>>         container_of(nb, struct driver_data, vbus_nb[2]);
>>>>>>>     ...
>>>>>>> }
>>>>>>>
>>>>>>> int probe(...)
>>>>>>> {
>>>>>>>     /* Register for vbus notification */
>>>>>>>     data->vbus_nb[0].notifier_call = vbus_cable0_evt;
>>>>>>>     data->vbus_nb[1].notifier_call = vbus_cable1_evt;
>>>>>>>     data->vbus_nb[2].notifier_call = vbus_cable2_evt;
>>>>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>>>>                     vbus_cable_ids[i], &data->vbus_nb[i]);
>>>>>>>         if (ret)
>>>>>>>             ...
>>>>>>>     }
>>>>>>> }
>>>>>>
>>>>>> You can get the notification for multiple external connector
>>>>>> by using the only one notifier_block as following:
>>>>>>
>>>>>> static const unsigned int vbus_cable_ids[] = {
>>>>>>     EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
>>>>>>
>>>>>> struct driver_data {
>>>>>>     struct notifier_block vbus_nb;
>>>>>> }
>>>>>>
>>>>>> static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p)
>>>>>> {
>>>>>>     struct driver_data *data =
>>>>>>         container_of(nb, struct driver_data, vbus_nb);
>>>>>>     int sdp_state, cdp_state, dcp_state;
>>>>>>
>>>>>>     sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP);
>>>>>>     cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP);
>>>>>>     dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP);
>>>>>>
>>>>>>     ...
>>>>>> }
>>>>>>
>>>>>> int probe(...)
>>>>>> {
>>>>>>     /* Register for vbus notification */
>>>>>>     data->vbus_nb.notifier_call = vbus_cable_evt;
>>>>>>     for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>>>>>>         ret = devm_extcon_register_notifier(dev, data->vbus_extcon,
>>>>>>                     vbus_cable_ids[i], &data->vbus_nb);
>>>>>>         if (ret)
>>>>>>             ...
>>>>>>     }
>>>>>> }
>>>>>
>>>>> No that will not work, you cannot add the same notifier_block to 3
>>>>> different notifier_heads, the list_head can only be part of one list!
>>>>>
>>>>> Also see:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>>>>
>>>>> Which actually fixes an issue I encountered exactly because of doing
>>>>> the above.
>>>>>
>>>>> So we really need something like my patch to make this simpler
>>>>> and to remove the ugliness we currently have in axp288_charger.c
>>>>> because of this problem (and are about to have in more drivers
>>>>> I'm upstreaming). Also note that my patch is quite simple, the
>>>>> amount of lines it adds is way less then the amount of lines
>>>>> it will allow to remove in a single driver!
>>>>
>>>> Ping ?
>>>>
>>>> As explained this is a real problem and we really need a proper solution
>>>> for this. I'm about to upstream 2 more drivers which need to monitor
>>>> an antire extcon rather then a single cable on it, and I do not want
>>>> to have to add hacks like this:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
>>>>
>>>> To both drivers. My patch is IMHO a nice and clean solution for this,
>>>> can you please merge this or provide an alternative solution ?
>>>
>>>
>>> As you said, when using one notifier_block for multiple external connector,
>>> it has a problem. But, it is not only for extcon framework. It depends
>>> on the notifier_block code. If each external connector uses the
>>> each notifier_block (it is a not hack), there is no any problem.
>>>
>>> As I already said on my reply[1] as following,
>>> ---------------------------------------------
>>> The extcon_register_notifier() function must support the notification
>>> for only one external connector.
>>> [1] https://www.spinics.net/lists/kernel/msg2468906.html
>>> ---------------------------------------------
>>>
>>> I agree that the EXTCON needs to support the notification
>>> for all supported external connector of extcon device
>>> by using only one notifier_chain.
>>>
>>> I don't prefer to use the '-1' as the unique id for getting the
>>> notification for all external connectors. Each argument of function
>>> must have the correct meaning.
>>>
>>> I think that thanks for your report and suggestion.
>>>
>>> I'm considering to add the new function as following:
>>> Following function will support the all external connector for notification
>>> and the function argument don't include the unique id for external connector.
>>> - extcon_regiter_all_notifier(struct extcon_dev *edev, struct notifier_block *nb);
>>> (The function name is not fixed)
>>
>> Adding a new extcon_register_all_notifier function for this is fine with
>> me, shall I send a new patch for this ?
>
> I'll develop the new following functions with your reported-by.
>
> int extcon_register_notifier_all(struct extcon_dev *edev,
>                                struct notifier_block *nb);
> int extcon_unregister_notifier_all(struct extcon_dev *edev,
>                                struct notifier_block *nb);
> int devm_extcon_register_notifier_all(struct device *dev,
>                                struct extcon_dev *edev,
>                                struct notifier_block *nb);
> void devm_extcon_unregister_notifier_all(struct device *dev,
>                                struct extcon_dev *edev,
>                                struct notifier_block *nb);

Looks good to me, thank you for looking into this. When you've a patch ready
please Cc me and I'll test it.

Regards,

Hans


>
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>
>>>
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Hans
>>>>>
>>>>>
>>>>>>>
>>>>>>> And then in the event handling the driver often checks the state of
>>>>>>> all cables explicitly using extcon_get_state, rather then using the
>>>>>>> event argument to the notifier_call.
>>>>>>>
>>>>>>> This commit makes extcon_[un]register_notifier accept -1 as cable-id,
>>>>>>> which will cause the notifier to get called for changes on any cable
>>>>>>> on the extcon_dev. Compared to the above example code this allows much
>>>>>>> simpler code in drivers which want to monitor multiple cable types.
>>>>>>>
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> -Fix false-positive "warning: 'idx' may be used uninitialized" warning
>>>>>>>  seen with some compiler versions
>>>>>>> ---
>>>>>>>  drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++---------
>>>>>>>  drivers/extcon/extcon.h |  1 +
>>>>>>>  2 files changed, 27 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>>>>> index 09ac5e7..2a042ee 100644
>>>>>>> --- a/drivers/extcon/extcon.c
>>>>>>> +++ b/drivers/extcon/extcon.c
>>>>>>> @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>>>>>>
>>>>>>>      state = !!(edev->state & BIT(index));
>>>>>>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>>>>>> +    raw_notifier_call_chain(&edev->nh_all, 0, edev);
>>>>>>>
>>>>>>>      /* This could be in interrupt handler */
>>>>>>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>>>>>> @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>>>>>>   *                any attach status changes from the extcon.
>>>>>>>   * @edev:    the extcon device that has the external connecotr.
>>>>>>>   * @id:        the unique id of each external connector in extcon enumeration.
>>>>>>> + *        or -1 to get notififications for all cables on edev, in this
>>>>>>> + *        case no state info will get passed to the notifier_call.
>>>>>>>   * @nb:        a notifier block to be registered.
>>>>>>>   *
>>>>>>>   * Note that the second parameter given to the callback of nb (val) is
>>>>>>> @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>>>>>>      if (!edev || !nb)
>>>>>>>          return -EINVAL;
>>>>>>>
>>>>>>> -    idx = find_cable_index_by_id(edev, id);
>>>>>>> -    if (idx < 0)
>>>>>>> -        return idx;
>>>>>>> +    if ((int)id != -1) {
>>>>>>> +        idx = find_cable_index_by_id(edev, id);
>>>>>>> +        if (idx < 0)
>>>>>>> +            return idx;
>>>>>>> +    }
>>>>>>>
>>>>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>>>> -    ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>>>>> +    if ((int)id != -1)
>>>>>>> +        ret = raw_notifier_chain_register(&edev->nh[idx], nb);
>>>>>>> +    else
>>>>>>> +        ret = raw_notifier_chain_register(&edev->nh_all, nb);
>>>>>>> +
>>>>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>>>>
>>>>>>>      return ret;
>>>>>>> @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>>>>>>                  struct notifier_block *nb)
>>>>>>>  {
>>>>>>>      unsigned long flags;
>>>>>>> -    int ret, idx;
>>>>>>> +    int ret, idx = 0;
>>>>>>>
>>>>>>>      if (!edev || !nb)
>>>>>>>          return -EINVAL;
>>>>>>>
>>>>>>> -    idx = find_cable_index_by_id(edev, id);
>>>>>>> -    if (idx < 0)
>>>>>>> -        return idx;
>>>>>>> +    if ((int)id != -1) {
>>>>>>> +        idx = find_cable_index_by_id(edev, id);
>>>>>>> +        if (idx < 0)
>>>>>>> +            return idx;
>>>>>>> +    }
>>>>>>>
>>>>>>>      spin_lock_irqsave(&edev->lock, flags);
>>>>>>> -    ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>>>>> +    if ((int)id != -1)
>>>>>>> +        ret = raw_notifier_chain_unregister(&edev->nh[idx], nb);
>>>>>>> +    else
>>>>>>> +        ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>>>>>>> +
>>>>>>>      spin_unlock_irqrestore(&edev->lock, flags);
>>>>>>>
>>>>>>>      return ret;
>>>>>>> @@ -1212,6 +1227,8 @@ int extcon_dev_register(struct extcon_dev *edev)
>>>>>>>      for (index = 0; index < edev->max_supported; index++)
>>>>>>>          RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>>>>>>
>>>>>>> +    RAW_INIT_NOTIFIER_HEAD(&edev->nh_all);
>>>>>>> +
>>>>>>>      dev_set_drvdata(&edev->dev, edev);
>>>>>>>      edev->state = 0;
>>>>>>>
>>>>>>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
>>>>>>> index 993ddcc..2e6c09d 100644
>>>>>>> --- a/drivers/extcon/extcon.h
>>>>>>> +++ b/drivers/extcon/extcon.h
>>>>>>> @@ -44,6 +44,7 @@ struct extcon_dev {
>>>>>>>      /* Internal data. Please do not set. */
>>>>>>>      struct device dev;
>>>>>>>      struct raw_notifier_head *nh;
>>>>>>> +    struct raw_notifier_head nh_all;
>>>>>>>      struct list_head entry;
>>>>>>>      int max_supported;
>>>>>>>      spinlock_t lock;    /* could be called by irq handler */
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web