lkml.org 
[lkml]   [2017]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev
From
Date
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 */
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>>
>
>

\
 
 \ /
  Last update: 2017-03-29 11:52    [W:0.507 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site