Message-id: <58DCCDED.6040605@samsung.com>
Date:   Thu, 30 Mar 2017 18:20:45 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Hans de Goede <hdegoede@...hat.com>, linux-kernel@...r.kernel.org
Cc:     chanwoo@...nel.org, myungjoo.ham@...sung.com
Subject: Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to
 monitor all external connectors

On 2017년 03월 30일 18:04, Hans de Goede wrote:
> Hi Chanwoo,
> 
> On 30-03-17 10:39, Chanwoo Choi wrote:
>> The extcon core already provides the extcon_register_notifier() function
>> in order to register the notifier block which is used to monitor
>> the status change for the specific external connector such as EXTCON_USB,
>> EXTCON_USB_HOST and so on. The extcon consumer uses the this function.
>>
>> The extcon consumer may need to monitor the all supported external
>> connectors from the extcon device. In this case, The extcon consumer
>> should have each notifier_block structure for each external connector.
>>
>> This patch adds the new extcon_register_notifier_all() function
>> that extcon consumer is able to monitor the status change of all
>> supported external connectors by using only one notifier_block structure.
>>
>> - List of new added functions:
>> 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);
>>
>> Suggested-by: Hans de Goede <hdegoede@...hat.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> ---
>>  drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/extcon/extcon.h |  3 +++
>>  include/linux/extcon.h  | 21 ++++++++++++----
>>  4 files changed, 145 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c
>> index b40eb1805927..3fc8eea52f1d 100644
>> --- a/drivers/extcon/devres.c
>> +++ b/drivers/extcon/devres.c
>> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res)
>>      extcon_unregister_notifier(this->edev, this->id, this->nb);
>>  }
>>
>> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res)
>> +{
>> +    struct extcon_dev_notifier_devres *this = res;
>> +
>> +    extcon_unregister_notifier_all(this->edev, this->nb);
>> +}
>> +
>>  /**
>>   * devm_extcon_dev_allocate - Allocate managed extcon device
>>   * @dev:        device owning the extcon device being created
>> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev,
>>                     devm_extcon_dev_match, edev));
>>  }
>>  EXPORT_SYMBOL(devm_extcon_unregister_notifier);
>> +
>> +/**
>> + * devm_extcon_register_notifier_all()
>> + *        - Resource-managed extcon_register_notifier_all()
>> + * @dev:    device to allocate extcon device
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + *
>> + * This function manages automatically the notifier of extcon device using
>> + * device resource management and simplify the control of unregistering
>> + * the notifier of extcon device.
>> + *
>> + * Note that the second parameter given to the callback of nb (val) is
>> + * the current state and third parameter is the edev pointer.
>> + *
>> + * Returns 0 if success or negaive error number if failure.
>> + */
>> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    struct extcon_dev_notifier_devres *ptr;
>> +    int ret;
>> +
>> +    ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr),
>> +                GFP_KERNEL);
>> +    if (!ptr)
>> +        return -ENOMEM;
>> +
>> +    ret = extcon_register_notifier_all(edev, nb);
>> +    if (ret) {
>> +        devres_free(ptr);
>> +        return ret;
>> +    }
>> +
>> +    ptr->edev = edev;
>> +    ptr->nb = nb;
>> +    devres_add(dev, ptr);
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL(devm_extcon_register_notifier_all);
>> +
>> +/**
>> + * devm_extcon_unregister_notifier_all()
>> +            - Resource-managed extcon_unregister_notifier_all()
>> + * @dev:    device to allocate extcon device
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + */
>> +void devm_extcon_unregister_notifier_all(struct device *dev,
>> +                struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg,
>> +                   devm_extcon_dev_match, edev));
>> +}
>> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all);
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 193a3a673d10..4873fe8d5cd8 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id)
>>      spin_lock_irqsave(&edev->lock, flags);
>>
>>      state = !!(edev->state & BIT(index));
>> +
>> +    /*
>> +     * Call functions in a raw notifier chain for the specific one
>> +     * external connector.
>> +     */
>>      raw_notifier_call_chain(&edev->nh[index], state, edev);
>>
>> +    /*
>> +     * Call functions in a raw notifier chain for the all supported
>> +     * external connectors.
>> +     */
>> +    raw_notifier_call_chain(&edev->nh_all, state, edev);
>> +
> 
> Passing state here is not useful since the receiver of
> the notifier call will not know for which cable the state is.

No, the receiver can use the 'state' value whether connector is attached
or detached. Also, the extcon have to keep the same value
for both extcon_register_notifier() and extcon_register_notfier_all().

> 
> Also this should be moved outside of the area of the
> function holding the edev->lock spinlock, since we don't
> pass state we do not need the lock and the called
> notifier function may very well want to call extcon_get_state
> to find out the state of one or more of the cables,
> which takes the lock.

The extcon uses the spinlock for the short delay.
Extcon should update the status of external connector
to the extcon consumer as soon as possible.

> 
> Otherwise looks good.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>      /* This could be in interrupt handler */
>>      prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
>>      if (!prop_buf) {
>> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id,
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>>
>> +/**
>> + * extcon_register_notifier_all() - Register a notifier block to get the noti
>> + *                of the status change for all supported external
>> + *                connectors from extcon.
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + *
>> + * Note that the second parameter given to the callback of nb (val) is
>> + * the current state and third parameter is the edev pointer.
>> + */
>> +int extcon_register_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    unsigned long flags;
>> +    int ret;
>> +
>> +    if (!edev || !nb)
>> +        return -EINVAL;
>> +
>> +    spin_lock_irqsave(&edev->lock, flags);
>> +    ret = raw_notifier_chain_register(&edev->nh_all, nb);
>> +    spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all);
>> +
>> +/**
>> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device.
>> + * @edev:    the extcon device that has the external connecotr.
>> + * @nb:        a notifier block to be registered.
>> + */
>> +int extcon_unregister_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb)
>> +{
>> +    unsigned long flags;
>> +    int ret;
>> +
>> +    if (!edev || !nb)
>> +        return -EINVAL;
>> +
>> +    spin_lock_irqsave(&edev->lock, flags);
>> +    ret = raw_notifier_chain_unregister(&edev->nh_all, nb);
>> +    spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all);
>> +
>>  static struct attribute *extcon_attrs[] = {
>>      &dev_attr_state.attr,
>>      &dev_attr_name.attr,
>> @@ -1199,6 +1259,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 993ddccafe11..dddddcfa0587 100644
>> --- a/drivers/extcon/extcon.h
>> +++ b/drivers/extcon/extcon.h
>> @@ -21,6 +21,8 @@
>>   * @dev:        Device of this extcon.
>>   * @state:        Attach/detach state of this extcon. Do not provide at
>>   *            register-time.
>> + * @nh_all:        Notifier for the state change events for all supported
>> + *            external connectors from this extcon.
>>   * @nh:            Notifier for the state change events from this extcon
>>   * @entry:        To support list of extcon devices so that users can
>>   *            search for extcon devices based on the extcon name.
>> @@ -43,6 +45,7 @@ struct extcon_dev {
>>
>>      /* Internal data. Please do not set. */
>>      struct device dev;
>> +    struct raw_notifier_head nh_all;
>>      struct raw_notifier_head *nh;
>>      struct list_head entry;
>>      int max_supported;
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index 3929d2e8a3c7..61e8d8c591a4 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev,
>>                  unsigned int id, unsigned int prop);
>>
>>  /*
>> - * Following APIs are to monitor every action of a notifier.
>> - * Registrar gets notified for every external port of a connection device.
>> - * Probably this could be used to debug an action of notifier; however,
>> - * we do not recommend to use this for normal 'notifiee' device drivers who
>> - * want to be notified by a specific external port of the notifier.
>> + * Following APIs are to monitor the status change of the external connectors.
>> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier
>> + *            for specific external connector from the extcon.
>> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier
>> + *            for all supported external connectors from the extcon.
>>   */
>>  extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
>>                      struct notifier_block *nb);
>> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev,
>>                  struct extcon_dev *edev, unsigned int id,
>>                  struct notifier_block *nb);
>>
>> +extern int extcon_register_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +extern int devm_extcon_register_notifier_all(struct device *dev,
>> +                struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +extern void devm_extcon_unregister_notifier_all(struct device *dev,
>> +                struct extcon_dev *edev,
>> +                struct notifier_block *nb);
>> +
>>  /*
>>   * Following API get the extcon device from devicetree.
>>   * This function use phandle of devicetree to get extcon device directly.
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Powered by blists - more mailing lists