[PATCH] leds: lp3952: Store leds[LP3952_LED_ALL] in struct lp3952_led_array

[PATCH] leds: lp3952: Store leds[LP3952_LED_ALL] in struct lp3952_led_array

[Axel Lin]

The LP3952_LED_ALL is known at compile time, so we can just store
leds[LP3952_LED_ALL] in struct lp3952_led_array rather than calling
devm_kcalloc() to allocate the memory.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/leds/leds-lp3952.c  | 9 ---------
 include/linux/leds-lp3952.h | 2 +-
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
index f6157c0..a73c8ff 100644
--- a/drivers/leds/leds-lp3952.c
+++ b/drivers/leds/leds-lp3952.c
@@ -215,21 +215,12 @@ static int lp3952_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	int status;
-	struct lp3952_ctrl_hdl *leds;
 	struct lp3952_led_array *priv;
 
 	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	leds = devm_kcalloc(&client->dev, LP3952_LED_ALL, sizeof(*leds),
-			    GFP_KERNEL);
-	if (!leds) {
-		devm_kfree(&client->dev, priv);
-		return -ENOMEM;
-	}
-
-	priv->leds = leds;
 	priv->client = client;
 
 	priv->enable_gpio = devm_gpiod_get(&client->dev, "nrst",
diff --git a/include/linux/leds-lp3952.h b/include/linux/leds-lp3952.h
index edd5ed6..49b37ed 100644
--- a/include/linux/leds-lp3952.h
+++ b/include/linux/leds-lp3952.h
@@ -119,7 +119,7 @@ struct lp3952_led_array {
 	struct regmap *regmap;
 	struct i2c_client *client;
 	struct gpio_desc *enable_gpio;
-	struct lp3952_ctrl_hdl *leds;
+	struct lp3952_ctrl_hdl leds[LP3952_LED_ALL];
 };
 
 #endif /* LEDS_LP3952_H_ */
-- 
2.5.0

Re: [PATCH] leds: lp3952: Store leds[LP3952_LED_ALL] in struct lp3952_led_array

[Jacek Anaszewski]

Hi Axel,

Thanks for catching this. I missed also redundant devm_kfree here.
Would it be OK for you if I merged your cleanup with the original
patch and added your Reviewed-by to the commit message instead?
This driver hasn't been pushed to the mainline yet, so it would be
nice not to introduce its original version with some shortcomings
that need to be immediately fixed.

Best regards,
Jacek Anaszewski

On 07/17/2016 04:18 AM, Axel Lin wrote:
> The LP3952_LED_ALL is known at compile time, so we can just store
> leds[LP3952_LED_ALL] in struct lp3952_led_array rather than calling
> devm_kcalloc() to allocate the memory.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>   drivers/leds/leds-lp3952.c  | 9 ---------
>   include/linux/leds-lp3952.h | 2 +-
>   2 files changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
> index f6157c0..a73c8ff 100644
> --- a/drivers/leds/leds-lp3952.c
> +++ b/drivers/leds/leds-lp3952.c
> @@ -215,21 +215,12 @@ static int lp3952_probe(struct i2c_client *client,
>   			const struct i2c_device_id *id)
>   {
>   	int status;
> -	struct lp3952_ctrl_hdl *leds;
>   	struct lp3952_led_array *priv;
>
>   	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
>   		return -ENOMEM;
>
> -	leds = devm_kcalloc(&client->dev, LP3952_LED_ALL, sizeof(*leds),
> -			    GFP_KERNEL);
> -	if (!leds) {
> -		devm_kfree(&client->dev, priv);
> -		return -ENOMEM;
> -	}
> -
> -	priv->leds = leds;
>   	priv->client = client;
>
>   	priv->enable_gpio = devm_gpiod_get(&client->dev, "nrst",
> diff --git a/include/linux/leds-lp3952.h b/include/linux/leds-lp3952.h
> index edd5ed6..49b37ed 100644
> --- a/include/linux/leds-lp3952.h
> +++ b/include/linux/leds-lp3952.h
> @@ -119,7 +119,7 @@ struct lp3952_led_array {
>   	struct regmap *regmap;
>   	struct i2c_client *client;
>   	struct gpio_desc *enable_gpio;
> -	struct lp3952_ctrl_hdl *leds;
> +	struct lp3952_ctrl_hdl leds[LP3952_LED_ALL];
>   };
>
>   #endif /* LEDS_LP3952_H_ */
>

Re: [PATCH] leds: lp3952: Store leds[LP3952_LED_ALL] in struct lp3952_led_array

[Jacek Anaszewski]

On 07/18/2016 08:53 AM, Axel Lin wrote:
>
>
> 2016-07-18 14:48 GMT+08:00 Jacek Anaszewski <j.anaszewski@samsung.com
> <mailto:j.anaszewski@samsung.com>>:
>
>     Hi Axel,
>
>     Thanks for catching this. I missed also redundant devm_kfree here.
>     Would it be OK for you if I merged your cleanup with the original
>     patch and added your Reviewed-by to the commit message instead?
>
>
> That's fine, you can add Reviewed-by: Axel Lin <axel.lin@ingics.com
> <mailto:axel.lin@ingics.com>>.
>
>     This driver hasn't been pushed to the mainline yet, so it would be
>     nice not to introduce its original version with some shortcomings
>     that need to be immediately fixed.

Fixed up the original patch and updated for-next branch of
linux-leds.git.

-- 
Best regards,
Jacek Anaszewski