[PATCH 1/2] Input: lm8323 - rely on device core to create kp_disable attribute

[PATCH 1/2] Input: lm8323 - rely on device core to create kp_disable attribute

[Dmitry Torokhov]

Device core now has facilities to create driver-specific device attributes
as part of driver probing, use them.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/lm8323.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index 3964f6e0f6af..d5195415533a 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -615,6 +615,12 @@ static ssize_t lm8323_set_disable(struct device *dev,
 }
 static DEVICE_ATTR(disable_kp, 0644, lm8323_show_disable, lm8323_set_disable);
 
+static struct attribute *lm8323_attrs[] = {
+	&dev_attr_disable_kp.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(lm8323);
+
 static int lm8323_probe(struct i2c_client *client)
 {
 	struct lm8323_platform_data *pdata = dev_get_platdata(&client->dev);
@@ -696,9 +702,6 @@ static int lm8323_probe(struct i2c_client *client)
 	}
 
 	lm->kp_enabled = true;
-	err = device_create_file(&client->dev, &dev_attr_disable_kp);
-	if (err < 0)
-		goto fail2;
 
 	idev->name = pdata->name ? : "LM8323 keypad";
 	snprintf(lm->phys, sizeof(lm->phys),
@@ -719,14 +722,14 @@ static int lm8323_probe(struct i2c_client *client)
 	err = input_register_device(idev);
 	if (err) {
 		dev_dbg(&client->dev, "error registering input device\n");
-		goto fail3;
+		goto fail2;
 	}
 
 	err = request_threaded_irq(client->irq, NULL, lm8323_irq,
 			  IRQF_TRIGGER_LOW|IRQF_ONESHOT, "lm8323", lm);
 	if (err) {
 		dev_err(&client->dev, "could not get IRQ %d\n", client->irq);
-		goto fail4;
+		goto fail3;
 	}
 
 	i2c_set_clientdata(client, lm);
@@ -736,11 +739,9 @@ static int lm8323_probe(struct i2c_client *client)
 
 	return 0;
 
-fail4:
+fail3:
 	input_unregister_device(idev);
 	idev = NULL;
-fail3:
-	device_remove_file(&client->dev, &dev_attr_disable_kp);
 fail2:
 	while (--pwm >= 0)
 		if (lm->pwm[pwm].enabled)
@@ -761,8 +762,6 @@ static void lm8323_remove(struct i2c_client *client)
 
 	input_unregister_device(lm->idev);
 
-	device_remove_file(&lm->client->dev, &dev_attr_disable_kp);
-
 	for (i = 0; i < 3; i++)
 		if (lm->pwm[i].enabled)
 			led_classdev_unregister(&lm->pwm[i].cdev);
@@ -823,8 +822,9 @@ static const struct i2c_device_id lm8323_id[] = {
 
 static struct i2c_driver lm8323_i2c_driver = {
 	.driver = {
-		.name	= "lm8323",
-		.pm	= pm_sleep_ptr(&lm8323_pm_ops),
+		.name		= "lm8323",
+		.pm		= pm_sleep_ptr(&lm8323_pm_ops),
+		.dev_groups	= lm8323_groups,
 	},
 	.probe		= lm8323_probe,
 	.remove		= lm8323_remove,
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH 2/2] Input: lm8323 - convert to use devm_* api

[Dmitry Torokhov]

From: Yangtao Li <frank.li@vivo.com>

Use devm_* api to simplify code, this makes it unnecessary to explicitly
release resources.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/lm8323.c | 77 +++++++++++----------------------
 1 file changed, 26 insertions(+), 51 deletions(-)

diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index d5195415533a..7bee93e9b0f5 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -556,6 +556,7 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
 		    const char *name)
 {
 	struct lm8323_pwm *pwm;
+	int err;
 
 	BUG_ON(id > 3);
 
@@ -575,9 +576,11 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
 		pwm->cdev.name = name;
 		pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
 		pwm->cdev.groups = lm8323_pwm_groups;
-		if (led_classdev_register(dev, &pwm->cdev) < 0) {
-			dev_err(dev, "couldn't register PWM %d\n", id);
-			return -1;
+
+		err = devm_led_classdev_register(dev, &pwm->cdev);
+		if (err) {
+			dev_err(dev, "couldn't register PWM %d: %d\n", id, err);
+			return err;
 		}
 		pwm->enabled = true;
 	}
@@ -585,8 +588,6 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
 	return 0;
 }
 
-static struct i2c_driver lm8323_i2c_driver;
-
 static ssize_t lm8323_show_disable(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
@@ -648,12 +649,13 @@ static int lm8323_probe(struct i2c_client *client)
 		return -EINVAL;
 	}
 
-	lm = kzalloc(sizeof *lm, GFP_KERNEL);
-	idev = input_allocate_device();
-	if (!lm || !idev) {
-		err = -ENOMEM;
-		goto fail1;
-	}
+	lm = devm_kzalloc(&client->dev, sizeof(*lm), GFP_KERNEL);
+	if (!lm)
+		return -ENOMEM;
+
+	idev = devm_input_allocate_device(&client->dev);
+	if (!idev)
+		return -ENOMEM;
 
 	lm->client = client;
 	lm->idev = idev;
@@ -669,8 +671,10 @@ static int lm8323_probe(struct i2c_client *client)
 
 	lm8323_reset(lm);
 
-	/* Nothing's set up to service the IRQ yet, so just spin for max.
-	 * 100ms until we can configure. */
+	/*
+	 * Nothing's set up to service the IRQ yet, so just spin for max.
+	 * 100ms until we can configure.
+	 */
 	tmo = jiffies + msecs_to_jiffies(100);
 	while (lm8323_read(lm, LM8323_CMD_READ_INT, data, 1) == 1) {
 		if (data[0] & INT_NOINIT)
@@ -690,15 +694,14 @@ static int lm8323_probe(struct i2c_client *client)
 	/* If a true probe check the device */
 	if (lm8323_read_id(lm, data) != 0) {
 		dev_err(&client->dev, "device not found\n");
-		err = -ENODEV;
-		goto fail1;
+		return -ENODEV;
 	}
 
 	for (pwm = 0; pwm < LM8323_NUM_PWMS; pwm++) {
 		err = init_pwm(lm, pwm + 1, &client->dev,
 			       pdata->pwm_names[pwm]);
-		if (err < 0)
-			goto fail2;
+		if (err)
+			return err;
 	}
 
 	lm->kp_enabled = true;
@@ -722,14 +725,16 @@ static int lm8323_probe(struct i2c_client *client)
 	err = input_register_device(idev);
 	if (err) {
 		dev_dbg(&client->dev, "error registering input device\n");
-		goto fail2;
+		return err;
 	}
 
-	err = request_threaded_irq(client->irq, NULL, lm8323_irq,
-			  IRQF_TRIGGER_LOW|IRQF_ONESHOT, "lm8323", lm);
+	err = devm_request_threaded_irq(&client->dev, client->irq,
+					NULL, lm8323_irq,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					"lm8323", lm);
 	if (err) {
 		dev_err(&client->dev, "could not get IRQ %d\n", client->irq);
-		goto fail3;
+		return err;
 	}
 
 	i2c_set_clientdata(client, lm);
@@ -738,35 +743,6 @@ static int lm8323_probe(struct i2c_client *client)
 	enable_irq_wake(client->irq);
 
 	return 0;
-
-fail3:
-	input_unregister_device(idev);
-	idev = NULL;
-fail2:
-	while (--pwm >= 0)
-		if (lm->pwm[pwm].enabled)
-			led_classdev_unregister(&lm->pwm[pwm].cdev);
-fail1:
-	input_free_device(idev);
-	kfree(lm);
-	return err;
-}
-
-static void lm8323_remove(struct i2c_client *client)
-{
-	struct lm8323_chip *lm = i2c_get_clientdata(client);
-	int i;
-
-	disable_irq_wake(client->irq);
-	free_irq(client->irq, lm);
-
-	input_unregister_device(lm->idev);
-
-	for (i = 0; i < 3; i++)
-		if (lm->pwm[i].enabled)
-			led_classdev_unregister(&lm->pwm[i].cdev);
-
-	kfree(lm);
 }
 
 /*
@@ -827,7 +803,6 @@ static struct i2c_driver lm8323_i2c_driver = {
 		.dev_groups	= lm8323_groups,
 	},
 	.probe		= lm8323_probe,
-	.remove		= lm8323_remove,
 	.id_table	= lm8323_id,
 };
 MODULE_DEVICE_TABLE(i2c, lm8323_id);
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH 2/2] Input: lm8323 - convert to use devm_* api

[Christophe JAILLET]

Le 24/07/2023 à 07:29, Dmitry Torokhov a écrit :
> From: Yangtao Li <frank.li@vivo.com>
> 
> Use devm_* api to simplify code, this makes it unnecessary to explicitly
> release resources.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/keyboard/lm8323.c | 77 +++++++++++----------------------
>   1 file changed, 26 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
> index d5195415533a..7bee93e9b0f5 100644
> --- a/drivers/input/keyboard/lm8323.c
> +++ b/drivers/input/keyboard/lm8323.c
> @@ -556,6 +556,7 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
>   		    const char *name)
>   {
>   	struct lm8323_pwm *pwm;
> +	int err;
>   
>   	BUG_ON(id > 3);
>   
> @@ -575,9 +576,11 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
>   		pwm->cdev.name = name;
>   		pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
>   		pwm->cdev.groups = lm8323_pwm_groups;
> -		if (led_classdev_register(dev, &pwm->cdev) < 0) {
> -			dev_err(dev, "couldn't register PWM %d\n", id);
> -			return -1;
> +
> +		err = devm_led_classdev_register(dev, &pwm->cdev);
> +		if (err) {
> +			dev_err(dev, "couldn't register PWM %d: %d\n", id, err);
> +			return err;
>   		}
>   		pwm->enabled = true;
>   	}
> @@ -585,8 +588,6 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
>   	return 0;
>   }
>   
> -static struct i2c_driver lm8323_i2c_driver;
> -
>   static ssize_t lm8323_show_disable(struct device *dev,
>   				   struct device_attribute *attr, char *buf)
>   {
> @@ -648,12 +649,13 @@ static int lm8323_probe(struct i2c_client *client)
>   		return -EINVAL;
>   	}
>   
> -	lm = kzalloc(sizeof *lm, GFP_KERNEL);
> -	idev = input_allocate_device();
> -	if (!lm || !idev) {
> -		err = -ENOMEM;
> -		goto fail1;
> -	}
> +	lm = devm_kzalloc(&client->dev, sizeof(*lm), GFP_KERNEL);
> +	if (!lm)
> +		return -ENOMEM;
> +
> +	idev = devm_input_allocate_device(&client->dev);
> +	if (!idev)
> +		return -ENOMEM;
>   
>   	lm->client = client;
>   	lm->idev = idev;
> @@ -669,8 +671,10 @@ static int lm8323_probe(struct i2c_client *client)
>   
>   	lm8323_reset(lm);
>   
> -	/* Nothing's set up to service the IRQ yet, so just spin for max.
> -	 * 100ms until we can configure. */
> +	/*
> +	 * Nothing's set up to service the IRQ yet, so just spin for max.
> +	 * 100ms until we can configure.
> +	 */
>   	tmo = jiffies + msecs_to_jiffies(100);
>   	while (lm8323_read(lm, LM8323_CMD_READ_INT, data, 1) == 1) {
>   		if (data[0] & INT_NOINIT)
> @@ -690,15 +694,14 @@ static int lm8323_probe(struct i2c_client *client)
>   	/* If a true probe check the device */
>   	if (lm8323_read_id(lm, data) != 0) {
>   		dev_err(&client->dev, "device not found\n");
> -		err = -ENODEV;
> -		goto fail1;
> +		return -ENODEV;
>   	}
>   
>   	for (pwm = 0; pwm < LM8323_NUM_PWMS; pwm++) {
>   		err = init_pwm(lm, pwm + 1, &client->dev,
>   			       pdata->pwm_names[pwm]);
> -		if (err < 0)
> -			goto fail2;
> +		if (err)
> +			return err;
>   	}
>   
>   	lm->kp_enabled = true;
> @@ -722,14 +725,16 @@ static int lm8323_probe(struct i2c_client *client)
>   	err = input_register_device(idev);
>   	if (err) {
>   		dev_dbg(&client->dev, "error registering input device\n");
> -		goto fail2;
> +		return err;
>   	}
>   
> -	err = request_threaded_irq(client->irq, NULL, lm8323_irq,
> -			  IRQF_TRIGGER_LOW|IRQF_ONESHOT, "lm8323", lm);
> +	err = devm_request_threaded_irq(&client->dev, client->irq,
> +					NULL, lm8323_irq,
> +					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					"lm8323", lm);
>   	if (err) {
>   		dev_err(&client->dev, "could not get IRQ %d\n", client->irq);
> -		goto fail3;
> +		return err;
>   	}
>   
>   	i2c_set_clientdata(client, lm);
> @@ -738,35 +743,6 @@ static int lm8323_probe(struct i2c_client *client)
>   	enable_irq_wake(client->irq);
>   
>   	return 0;
> -
> -fail3:
> -	input_unregister_device(idev);
> -	idev = NULL;
> -fail2:
> -	while (--pwm >= 0)
> -		if (lm->pwm[pwm].enabled)
> -			led_classdev_unregister(&lm->pwm[pwm].cdev);

This and...

> -fail1:
> -	input_free_device(idev);
> -	kfree(lm);
> -	return err;
> -}
> -
> -static void lm8323_remove(struct i2c_client *client)
> -{
> -	struct lm8323_chip *lm = i2c_get_clientdata(client);
> -	int i;
> -
> -	disable_irq_wake(client->irq);
> -	free_irq(client->irq, lm);
> -
> -	input_unregister_device(lm->idev);
> -
> -	for (i = 0; i < 3; i++)
> -		if (lm->pwm[i].enabled)
> -			led_classdev_unregister(&lm->pwm[i].cdev);

this look wrong.
What you left for lm8323 looks correct.

CJ

> -
> -	kfree(lm);
>   }
>   
>   /*
> @@ -827,7 +803,6 @@ static struct i2c_driver lm8323_i2c_driver = {
>   		.dev_groups	= lm8323_groups,
>   	},
>   	.probe		= lm8323_probe,
> -	.remove		= lm8323_remove,
>   	.id_table	= lm8323_id,
>   };
>   MODULE_DEVICE_TABLE(i2c, lm8323_id);

Re: [PATCH 2/2] Input: lm8323 - convert to use devm_* api

[Dmitry Torokhov]

On Mon, Jul 24, 2023 at 08:53:11PM +0200, Christophe JAILLET wrote:
> Le 24/07/2023 à 07:29, Dmitry Torokhov a écrit :
> > From: Yangtao Li <frank.li@vivo.com>
> > +
> > +		err = devm_led_classdev_register(dev, &pwm->cdev);

                      ^^^^^^^^^^^^^^

...

> > -
> > -	for (i = 0; i < 3; i++)
> > -		if (lm->pwm[i].enabled)
> > -			led_classdev_unregister(&lm->pwm[i].cdev);
> 
> this look wrong.

Why? It will be cleared up by devm.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: lm8323 - convert to use devm_* api

[Christophe JAILLET]

Le 24/07/2023 à 21:01, Dmitry Torokhov a écrit :
> On Mon, Jul 24, 2023 at 08:53:11PM +0200, Christophe JAILLET wrote:
>> Le 24/07/2023 à 07:29, Dmitry Torokhov a écrit :
>>> From: Yangtao Li <frank.li@vivo.com>
>>> +
>>> +		err = devm_led_classdev_register(dev, &pwm->cdev);
> 
>                        ^^^^^^^^^^^^^^

Oops,
For some reason, I missed this hunk :(.

> 
>>> -
>>> -	for (i = 0; i < 3; i++)
>>> -		if (lm->pwm[i].enabled)
>>> -			led_classdev_unregister(&lm->pwm[i].cdev);
>>
>> this look wrong.
> 
> Why? It will be cleared up by devm.
> 
> Thanks.
> 

Sorry for the noise.

I've been puzzled by [1].

CJ

[1]: https://lore.kernel.org/all/20230714080611.81302-8-frank.li@vivo.com/