Re: [PATCHv4 1/8] extcon: Add resource-managed extcon register function

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi,

This patch has some bug and need code clean.
I'll fix it and resend only this patch on v5.

Thanks,
Chanwoo Choi

On 04/21/2014 07:10 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.woo@samsung.com>
> ---
>  drivers/extcon/extcon-class.c |   64 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/extcon.h        |   17 +++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index 7ab21aa..645b02b 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -819,6 +819,70 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  }
>  EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>  
> +static void devm_extcon_dev_release(struct device *dev, void *res)
> +{
> +	extcon_dev_unregister((struct extcon_dev *)res);

'res' is double pointer. this line have to modify as following:
	extcon_dev_unregister(*(struct extcon_dev **)res);

> +}
> +
> +static int devm_extcon_dev_match(struct device *dev, void *res, void *data)
> +{
> +	return res == data;

'res' is double pointer so this line have to modify as following:
	return *res == data;

and additionally add error exception code

	struct extcon_dev **r = res;

	if (!r || !*r) {
		WARN_ON(!r || !*r);
		return 0;
	}

	return *r == data;

> +}
> +
> +/**
> + * 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_dev **ptr;
> +	int ret;
> +
> +	ptr = devres_alloc(devm_extcon_dev_release, sizeof(*ptr),
> +			GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = extcon_dev_register(edev);
> +	if (ret) {
> +		devres_free(ptr);
> +		return ret;
> +	}
> +
> +	*ptr = edev;
> +	devres_add(dev, ptr);
> +
> +	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)
> +{
> +	WARN_ON(devres_release(dev, devm_extcon_dev_release,
> +			devm_extcon_dev_match, edev));

Need to keep the indentation for readability.

> +}
> +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..6a17f69 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);
> +
> +/*
>   * get/set/update_state access the 32b encoded state value, which represents
>   * states of all possible cables of the multistate port. For example, if one
>   * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
> @@ -254,6 +262,15 @@ static inline int extcon_dev_register(struct extcon_dev *edev)
>  
>  static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
>  
> +static inline devm_extcon_dev_register(struct device *dev,

Missing the vairable type of return value.

> +				       struct extcon_dev *edev)
> +{
> +	return 0;
> +}
> +
> +static inline devm_extcon_dev_unregister(struct device *dev,

Missing the vairable type of return value.

> +					 struct extcon_dev *edev) { }
> +
>  static inline u32 extcon_get_state(struct extcon_dev *edev)
>  {
>  	return 0;
>