Re: [PATCH 4/5] leds: flash: Add devm_* functions to the flash class

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Dan,

Thank you for the patch. One funny omission caught my
eye here and in led-class.c when making visual comparison.
Please refer below.

On 10/1/19 8:04 PM, Dan Murphy wrote:
> Add the missing device managed API for registration and
> unregistration for the LED flash class.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  drivers/leds/led-class-flash.c  | 50 +++++++++++++++++++++++++++++++++
>  include/linux/led-class-flash.h | 14 +++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
> index 60c3de5c6b9f..c2b4fd02a1bc 100644
> --- a/drivers/leds/led-class-flash.c
> +++ b/drivers/leds/led-class-flash.c
> @@ -327,6 +327,56 @@ void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_classdev_flash_unregister);
>  
> +static void devm_led_classdev_flash_release(struct device *dev, void *res)
> +{
> +	led_classdev_flash_unregister(*(struct led_classdev_flash **)res);
> +}
> +
> +int devm_led_classdev_flash_register_ext(struct device *parent,
> +				     struct led_classdev_flash *fled_cdev,
> +				     struct led_init_data *init_data)
> +{
> +	struct led_classdev_flash **dr;
> +	int ret;
> +
> +	dr = devres_alloc(devm_led_classdev_flash_release, sizeof(*dr),
> +			  GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	ret = led_classdev_flash_register_ext(parent, fled_cdev, init_data);
> +	if (ret) {
> +		devres_free(dr);
> +		return ret;
> +	}
> +
> +	*dr = fled_cdev;
> +	devres_add(parent, dr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_led_classdev_flash_register_ext);
> +
> +static int devm_led_classdev_flash_match(struct device *dev,
> +					      void *res, void *data)
> +{
> +	struct fled_cdev **p = res;

We don't have struct fled_cdev. This name is used for variables
of struct led_clssdev_flash type in drivers.

We don't get even compiler warning here because this is cast
from void pointer.

Funny thing is that you seem to have followed similar flaw in
devm_led_classdev_match() where struct led_cdev has been
introduced.

We need to fix both cases.

> +
> +	if (WARN_ON(!p || !*p))
> +		return 0;
> +
> +	return *p == data;
> +}
> +
> +void devm_led_classdev_flash_unregister(struct device *dev,
> +				  	     struct led_classdev_flash *fled_cdev)
> +{
> +	WARN_ON(devres_release(dev,
> +			       devm_led_classdev_flash_release,
> +			       devm_led_classdev_flash_match, fled_cdev));
> +}
> +EXPORT_SYMBOL_GPL(devm_led_classdev_flash_unregister);
> +
>  static void led_clamp_align(struct led_flash_setting *s)
>  {
>  	u32 v, offset;
> diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
> index 1bd83159fa4c..21a3358a1731 100644
> --- a/include/linux/led-class-flash.h
> +++ b/include/linux/led-class-flash.h
> @@ -113,6 +113,20 @@ static inline int led_classdev_flash_register(struct device *parent,
>   */
>  void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev);
>  
> +int devm_led_classdev_flash_register_ext(struct device *parent,
> +				     struct led_classdev_flash *fled_cdev,
> +				     struct led_init_data *init_data);
> +
> +
> +static inline int devm_led_classdev_flash_register(struct device *parent,
> +				     struct led_classdev_flash *fled_cdev)
> +{
> +	return devm_led_classdev_flash_register_ext(parent, fled_cdev, NULL);
> +}
> +
> +void devm_led_classdev_flash_unregister(struct device *parent,
> +					struct led_classdev_flash *fled_cdev);
> +
>  /**
>   * led_set_flash_strobe - setup flash strobe
>   * @fled_cdev: the flash LED to set strobe on
> 

-- 
Best regards,
Jacek Anaszewski