[PATCH v2 1/2] leds: lp3952: Remove ACPI support for lp3952

[PATCH v2 1/2] leds: lp3952: Remove ACPI support for lp3952

[Andy Shevchenko]

In ACPI world any ID should be carefully chosen and registered
officially. The discussion [1] as I read it gets to wilful assignment
an ID for non-existing real DSDT example.

Rafael already told [2] how this device would be enumerated using
compatible string. To be more precise look at the possible DSDT excerpt
below:

	Device (LDX0) {
		Name (_HID, "PRP0001")
		Name (_DDN, "TI LP3952 compatible led driver")
		...
	})

	Name (_DSD, Package () {
		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
		Package () {
			Package () {"compatible", "ti,lp3952"},
			...
		}
	})
Based on above, remove non-official ACPI IDs and enumeration from the
driver.

Note: currently driver has no compatible strings at all, to make above
working one should add at least one.

[1] https://e2e.ti.com/support/power_management/led_driver/f/192/t/524926
[2] https://www.spinics.net/lists/linux-acpi/msg67125.html

Cc: Tony Makkiel <tony.makkiel@daqri.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 v2:
 - extend commit message to show how PRP0001 is supposed to work
 - update link to Rafael's review
 - revert wrong ret variable fix
 drivers/leds/leds-lp3952.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
index 4847e89883a7..5184436b8905 100644
--- a/drivers/leds/leds-lp3952.c
+++ b/drivers/leds/leds-lp3952.c
@@ -10,7 +10,6 @@
  *
  */
 
-#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
@@ -276,19 +275,9 @@ static const struct i2c_device_id lp3952_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, lp3952_id);
 
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id lp3952_acpi_match[] = {
-	{"TXNW3952", 0},
-	{}
-};
-
-MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
-#endif
-
 static struct i2c_driver lp3952_i2c_driver = {
 	.driver = {
 			.name = LP3952_NAME,
-			.acpi_match_table = ACPI_PTR(lp3952_acpi_match),
 	},
 	.probe = lp3952_probe,
 	.remove = lp3952_remove,
-- 
2.11.0

[PATCH v2 2/2] leds: lp3952: Use 'if (ret)' pattern

[Andy Shevchenko]

Instead of unusual "if (!ret)" use "if (ret)" in lp3952_get_label().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/leds-lp3952.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
index 5184436b8905..847f7f282126 100644
--- a/drivers/leds/leds-lp3952.c
+++ b/drivers/leds/leds-lp3952.c
@@ -102,10 +102,11 @@ static int lp3952_get_label(struct device *dev, const char *label, char *dest)
 	const char *str;
 
 	ret = device_property_read_string(dev, label, &str);
-	if (!ret)
-		strncpy(dest, str, LP3952_LABEL_MAX_LEN);
+	if (ret)
+		return ret;
 
-	return ret;
+	strncpy(dest, str, LP3952_LABEL_MAX_LEN);
+	return 0;
 }
 
 static int lp3952_register_led_classdev(struct lp3952_led_array *priv)
-- 
2.11.0

Re: [PATCH v2 1/2] leds: lp3952: Remove ACPI support for lp3952

[Jacek Anaszewski]

Hi Andy,

Thanks for the patch.

What about CONFIG_ACPI dependency in drivers/leds/Kconfig?

Best regards,
Jacek Anaszewski

On 03/22/2017 04:00 PM, Andy Shevchenko wrote:
> In ACPI world any ID should be carefully chosen and registered
> officially. The discussion [1] as I read it gets to wilful assignment
> an ID for non-existing real DSDT example.
> 
> Rafael already told [2] how this device would be enumerated using
> compatible string. To be more precise look at the possible DSDT excerpt
> below:
> 
> 	Device (LDX0) {
> 		Name (_HID, "PRP0001")
> 		Name (_DDN, "TI LP3952 compatible led driver")
> 		...
> 	})
> 
> 	Name (_DSD, Package () {
> 		ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> 		Package () {
> 			Package () {"compatible", "ti,lp3952"},
> 			...
> 		}
> 	})
> Based on above, remove non-official ACPI IDs and enumeration from the
> driver.
> 
> Note: currently driver has no compatible strings at all, to make above
> working one should add at least one.
> 
> [1] https://e2e.ti.com/support/power_management/led_driver/f/192/t/524926
> [2] https://www.spinics.net/lists/linux-acpi/msg67125.html
> 
> Cc: Tony Makkiel <tony.makkiel@daqri.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  v2:
>  - extend commit message to show how PRP0001 is supposed to work
>  - update link to Rafael's review
>  - revert wrong ret variable fix
>  drivers/leds/leds-lp3952.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp3952.c b/drivers/leds/leds-lp3952.c
> index 4847e89883a7..5184436b8905 100644
> --- a/drivers/leds/leds-lp3952.c
> +++ b/drivers/leds/leds-lp3952.c
> @@ -10,7 +10,6 @@
>   *
>   */
>  
> -#include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
>  #include <linux/i2c.h>
> @@ -276,19 +275,9 @@ static const struct i2c_device_id lp3952_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, lp3952_id);
>  
> -#ifdef CONFIG_ACPI
> -static const struct acpi_device_id lp3952_acpi_match[] = {
> -	{"TXNW3952", 0},
> -	{}
> -};
> -
> -MODULE_DEVICE_TABLE(acpi, lp3952_acpi_match);
> -#endif
> -
>  static struct i2c_driver lp3952_i2c_driver = {
>  	.driver = {
>  			.name = LP3952_NAME,
> -			.acpi_match_table = ACPI_PTR(lp3952_acpi_match),
>  	},
>  	.probe = lp3952_probe,
>  	.remove = lp3952_remove,
> 

Re: [PATCH v2 1/2] leds: lp3952: Remove ACPI support for lp3952

[Andy Shevchenko]

On Wed, 2017-03-22 at 20:51 +0100, Jacek Anaszewski wrote:
> Hi Andy,
> 
> Thanks for the patch.

> What about CONFIG_ACPI dependency in drivers/leds/Kconfig?

Thanks for pointing this out. From now on it's redundant.

I will fix this in v3, but will wait a bit for other comments / tags if
any.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy