Hi Sangjung,
Thanks for your contribution.
On 04/16/2014 07:26 PM, Sangjung Woo wrote:
> Add resource-managed extcon device register function for convenience.
> For example, if a extcon device is attached with new
> devm_extcon_dev_register(), that extcon device is automatically
> unregistered on driver detach.
>
> Signed-off-by: Sangjung Woo <sangjung....@samsung.com>
> ---
> drivers/extcon/extcon-class.c | 83
> +++++++++++++++++++++++++++++++++++++++++
> include/linux/extcon.h | 8 ++++
> 2 files changed, 91 insertions(+)
>
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index 7ab21aa..accb49c 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -32,6 +32,7 @@
> #include <linux/slab.h>
> #include <linux/sysfs.h>
> #include <linux/of.h>
> +#include <linux/gfp.h>
It is not necessary because 'device.h' already includes 'gfp.h' header file.
>
> /*
> * extcon_cable_name suggests the standard cable names for commonly used
> @@ -819,6 +820,88 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> }
> EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>
> +
> +/*
> + * Device resource management
> + */
> +
Should delete blank line.
> +struct extcon_devres {
> + struct extcon_dev *edev;
> +};
> +
> +static void devm_extcon_release(struct device *dev, void *res)
Need to change function name as following to sustain consistency of existing
extcon functions.
- devm_extcon_release -> devm_extcon_dev_release()
> +{
> + struct extcon_devres *dr = (struct extcon_devres *)res;
> +
> + extcon_dev_unregister(dr->edev);
> +}
> +
> +static int devm_extcon_match(struct device *dev, void *res, void *data)
ditto.
- devm_extcon_match -> devm_extcon_dev_match
> +{
> + struct extcon_devres *dr = (struct extcon_devres *)res;
> + struct extcon_devres *match = (struct extcon_devres *)data;
I think that this function don't need explicit casting
because as I knew, casting is automatically about tool-chain.
> +
> + return dr->edev == match->edev;
> +}
> +
> +/**
> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
> + * @dev: device to allocate extcon device
> + * @edev: the new extcon device to register
> + *
> + * Managed extcon_dev_register() function. If extcon device is attached with
> + * this function, that extcon device is automatically unregistered on driver
> + * detach. Internally this function calls extcon_dev_register() function.
> + * To get more information, refer that function.
> + *
> + * If extcon device is registered with this function and the device needs to
> be
> + * unregistered separately, devm_extcon_dev_unregister() should be used.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev *edev)
> +{
> + struct extcon_devres *dr;
To improve readability, I prefer to change 'dr' variable name (e.g., dr
->devres)
> + int rc;
I think 'rc' variable name is ambiguous.
I prefer to change variable name for return value. (rc -> ret)
> +
> + dr = devres_alloc(devm_extcon_release, sizeof(struct extcon_devres),
ditto.
- devm_extcon_release -> devm_extcon_dev_release
We chan modify it as following:
sizeof(struct extcon_devres) -> sizeof(*dr)
> + GFP_KERNEL);
> + if (!dr)
> + return -ENOMEM;
> +
> + rc = extcon_dev_register(edev);
> + if (rc) {
> + devres_free(dr);
> + return rc;
> + }
> +
> + dr->edev = edev;
> + devres_add(dev, dr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
> +
> +/**
> + * devm_extcon_dev_unregister() - Resource-managed extcon_dev_unregister()
> + * @dev: device the extcon belongs to
> + * @edev: the extcon device to unregister
> + *
> + * Unregister extcon device that is registered with
> devm_extcon_dev_register()
> + * function.
> + */
> +void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev *edev)
> +{
> + struct extcon_devres match_dr = { edev };
Should we define 'match_dr' variable? I think it is not necessary.
Maybe it could use 'edev' directly without casting.
> +
> + WARN_ON(devres_destroy(dev, devm_extcon_release,
> + devm_extcon_match, &match_dr));
I think that devres_release() is more proper than devres_destroy.
> +
> + extcon_dev_unregister(edev);
If you use devres_release() instead of devres_destroy(), don't need to call
extcon_dev_unregister() function separately because devres_release() function
would call 'release' function.
> +}
> +EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
> +
> #ifdef CONFIG_OF
> /*
> * extcon_get_edev_by_phandle - Get the extcon device from devicetree
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index f488145..e1e85a1 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev
> *edev);
> extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
>
> /*
> + * Resource-managed extcon device register function.
> + */
> +extern int devm_extcon_dev_register(struct device *dev,
> + struct extcon_dev *edev);
> +extern void devm_extcon_dev_unregister(struct device *dev,
> + struct extcon_dev *edev);
This functions need to consider if CONFIG_OF is disabled.
IF CONFIG_OF is disabled, devm_extcon_dev_register/unregister() function
must need static inline function for dummy function.
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/
