iio: light: acpi-als: Properly enable on ASUS Zenbooks

PATCH In review Josef Gajdusek 3 participants 4 messages Jan 19, 2017 – Jan 20, 2017 IIO

lore

Share & tools

lore

Josef Gajdusek

author

2017-01-19 19:16 UTC #0 lore raw

PATCH v3 1/1 iio: light: acpi-als: Properly enable on ASUS Zenbooks

ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral. Otherwise, reads just return 0.

Signed-off-by Josef Gajdusek <[email protected]>

---
 drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

drivers/iio/light/acpi-als.c +60 -0

index f0b47c5..43417c9 100644
@@ -30,6 +30,7 @@
 #include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/dmi.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>

@@ -39,6 +40,9 @@
 #define ACPI_ALS_DEVICE_NAME		"acpi-als"
 #define ACPI_ALS_NOTIFY_ILLUMINANCE	0x80
 
+#define ACPI_ALS_ASUS_TALS		"\\_SB.PCI0.LPCB.EC0.TALS"
+#define ACPI_ALS_ASUS_ALSC		"\\_SB.ATKD.ALSC"
+
 ACPI_MODULE_NAME("acpi-als");
 
 /*

@@ -170,6 +174,46 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
+static int acpi_als_quirk_asus_zenbook_add(struct acpi_als *als)
+{
+	acpi_status status;
+
+	status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+				ACPI_ALS_ASUS_TALS));
+		return -EIO;
+	}
+	status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+				ACPI_ALS_ASUS_ALSC));
+		return -EIO;
+	}
+
+	return 0;
+}
+
+struct quirk_entry {
+	int (*add)(struct acpi_als *als);
+};
+
+static struct quirk_entry acpi_als_quirk_asus_zenbook = {
+	.add = acpi_als_quirk_asus_zenbook_add
+};
+
+static const struct dmi_system_id acpi_als_quirks[] = {
+	{
+		.ident = "ASUSTeK COMPUTER INC. UX",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "UX"),
+		},
+		.driver_data = &acpi_als_quirk_asus_zenbook,
+	},
+	{},
+};
+
 static const struct iio_info acpi_als_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= acpi_als_read_raw,

@@ -180,6 +224,9 @@ static int acpi_als_add(struct acpi_device *device)
 	struct acpi_als *als;
 	struct iio_dev *indio_dev;
 	struct iio_buffer *buffer;
+	const struct dmi_system_id *dmiid;
+	struct quirk_entry *quirk;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
 	if (!indio_dev)

@@ -191,6 +238,19 @@ static int acpi_als_add(struct acpi_device *device)
 	als->device = device;
 	mutex_init(&als->lock);
 
+	dmiid = dmi_first_match(acpi_als_quirks);
+	if (dmiid) {
+		dev_dbg(&device->dev, "Detected quirk for %s\n", dmiid->ident);
+		quirk = dmiid->driver_data;
+		ret = quirk->add(als);
+		if (ret) {
+			dev_err(&device->dev,
+				"Executing quirk for %s failed - %d",
+				dmiid->ident, ret);
+			return ret;
+		}
+	}
+
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
 	indio_dev->dev.parent = &device->dev;
 	indio_dev->info = &acpi_als_info;

Signature · 1 line

2.10.2

Marek Vasut

2017-01-19 22:04 UTC #1 lore raw

Quoted 4 lines · Josef Gajdusek

ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral. Otherwise, reads just return 0.
Signed-off-by Josef Gajdusek <[email protected]>

Changelog is missing ...

Quoted 34 lines

---
 drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

drivers/iio/light/acpi-als.c +8 -0

index f0b47c5..43417c9 100644
@@ -30,6 +30,7 @@
 #include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/dmi.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>

@@ -39,6 +40,9 @@
 #define ACPI_ALS_DEVICE_NAME		"acpi-als"
 #define ACPI_ALS_NOTIFY_ILLUMINANCE	0x80
 
+#define ACPI_ALS_ASUS_TALS		"\\_SB.PCI0.LPCB.EC0.TALS"
+#define ACPI_ALS_ASUS_ALSC		"\\_SB.ATKD.ALSC"
+
 ACPI_MODULE_NAME("acpi-als");
 
 /*

@@ -170,6 +174,46 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
+static int acpi_als_quirk_asus_zenbook_add(struct acpi_als *als)
+{
+	acpi_status status;
+

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 190

Quoted 10 lines

drivers/iio/light/acpi-als.c +10 -0

+	status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+				ACPI_ALS_ASUS_TALS));
+		return -EIO;
+	}
+	status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+				ACPI_ALS_ASUS_ALSC));

This doesn't unwind the previous operation. If ALSC fails, won't the TALS still be enabled ?

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 191

Quoted 1 line

drivers/iio/light/acpi-als.c +1 -0
+		return -EIO;

Isn't there some way to extract error code from the $status and propagating it instead of hard-coding EIO here ?

Quoted 9 lines

drivers/iio/light/acpi-als.c +9 -0

+	}
+
+	return 0;
+}
+
+struct quirk_entry {
+	int (*add)(struct acpi_als *als);
+};
+

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 210

Quoted 10 lines

drivers/iio/light/acpi-als.c +10 -0

+static struct quirk_entry acpi_als_quirk_asus_zenbook = {
+	.add = acpi_als_quirk_asus_zenbook_add
+};
+
+static const struct dmi_system_id acpi_als_quirks[] = {
+	{
+		.ident = "ASUSTeK COMPUTER INC. UX",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "UX"),

I'm no expert on DMI, but is this unique enough ?

Quoted 20 lines

drivers/iio/light/acpi-als.c +9 -0

+		},
+		.driver_data = &acpi_als_quirk_asus_zenbook,
+	},
+	{},
+};
+
 static const struct iio_info acpi_als_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= acpi_als_read_raw,

@@ -180,6 +224,9 @@ static int acpi_als_add(struct acpi_device *device)
 	struct acpi_als *als;
 	struct iio_dev *indio_dev;
 	struct iio_buffer *buffer;
+	const struct dmi_system_id *dmiid;
+	struct quirk_entry *quirk;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
 	if (!indio_dev)

drivers/iio/light/acpi-als.c static int acpi_als_add(struct acpi_device *device) · line 244

Quoted 7 lines

drivers/iio/light/acpi-als.c +4 -0

@@ -191,6 +238,19 @@ static int acpi_als_add(struct acpi_device *device)
 	als->device = device;
 	mutex_init(&als->lock);
 
+	dmiid = dmi_first_match(acpi_als_quirks);
+	if (dmiid) {
+		dev_dbg(&device->dev, "Detected quirk for %s\n", dmiid->ident);
+		quirk = dmiid->driver_data;

What happens if some entry has quirk == NULL ? :)

drivers/iio/light/acpi-als.c static int acpi_als_add(struct acpi_device *device) · line 245

Quoted 1 line

drivers/iio/light/acpi-als.c +1 -0
+		ret = quirk->add(als);

Who will disable the ALS if you rmmod this driver ? Hint: noone, fix it.

btw something tells me that maybe, just maybe, we should use pm_runtime to enable and disable the ALS on demand instead of enabling it and keeping it enabled for the entire duration this driver is in the kernel. This will reduce the power consumption a little.

Quoted 12 lines

drivers/iio/light/acpi-als.c +8 -0

+		if (ret) {
+			dev_err(&device->dev,
+				"Executing quirk for %s failed - %d",
+				dmiid->ident, ret);
+			return ret;
+		}
+	}
+
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
 	indio_dev->dev.parent = &device->dev;
 	indio_dev->info = &acpi_als_info;

Signature · 2 lines

Best regards,
Marek Vasut

Matt Ranostay

2017-01-20 02:53 UTC #2 lore raw

In reply to Josef Gajdusek

Quoted 21 lines · Josef Gajdusek

ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral. Otherwise, reads just return 0.
Signed-off-by Josef Gajdusek <[email protected]>
---
 drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
drivers/iio/light/acpi-als.c +1 -0
index f0b47c5..43417c9 100644
@@ -30,6 +30,7 @@
 #include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/dmi.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>

drivers/iio/light/acpi-als.c line 44

Quoted 5 lines

drivers/iio/light/acpi-als.c +2 -0

@@ -39,6 +40,9 @@
 #define ACPI_ALS_DEVICE_NAME           "acpi-als"
 #define ACPI_ALS_NOTIFY_ILLUMINANCE    0x80
 
+#define ACPI_ALS_ASUS_TALS             "\\_SB.PCI0.LPCB.EC0.TALS"
+#define ACPI_ALS_ASUS_ALSC             "\\_SB.ATKD.ALSC"

Not sure I like these one off defines... this could easily get out of hand. Also too lazy to pop out the ACPI spec what does TALS and ALSC mean?

Quoted 20 lines

drivers/iio/light/acpi-als.c +13 -0

+
 ACPI_MODULE_NAME("acpi-als");
 
 /*

@@ -170,6 +174,46 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
        return IIO_VAL_INT;
 }
 
+static int acpi_als_quirk_asus_zenbook_add(struct acpi_als *als)
+{
+       acpi_status status;
+
+       status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
+       if (ACPI_FAILURE(status)) {
+               ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+                               ACPI_ALS_ASUS_TALS));
+               return -EIO;
+       }
+       status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
+       if (ACPI_FAILURE(status)) {

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 198

Quoted 10 lines

drivers/iio/light/acpi-als.c +10 -0

+               ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+                               ACPI_ALS_ASUS_ALSC));
+               return -EIO;
+       }
+
+       return 0;
+}
+
+struct quirk_entry {
+       int (*add)(struct acpi_als *als);

Maybe put the defines above in here.

     .tals  = "\\_SB.PCI0.LPCB.EC0.TALS",
     .alsc = ""\\_SB.ATKD.ALSC"

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 210

Quoted 12 lines

drivers/iio/light/acpi-als.c +12 -0

+};
+
+static struct quirk_entry acpi_als_quirk_asus_zenbook = {
+       .add = acpi_als_quirk_asus_zenbook_add
+};
+
+static const struct dmi_system_id acpi_als_quirks[] = {
+       {
+               .ident = "ASUSTeK COMPUTER INC. UX",
+               .matches = {
+                       DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+                       DMI_MATCH(DMI_PRODUCT_NAME, "UX"),

Really is the DMI_PRODUCT_NAME only two characters? You are likely going to need another DMI property to match on.

Thanks,

Matt

Quoted 46 lines

drivers/iio/light/acpi-als.c +22 -1

+               },
+               .driver_data = &acpi_als_quirk_asus_zenbook,
+       },
+       {},
+};
+
 static const struct iio_info acpi_als_info = {
        .driver_module          = THIS_MODULE,
        .read_raw               = acpi_als_read_raw,

@@ -180,6 +224,9 @@ static int acpi_als_add(struct acpi_device *device)
        struct acpi_als *als;
        struct iio_dev *indio_dev;
        struct iio_buffer *buffer;
+       const struct dmi_system_id *dmiid;
+       struct quirk_entry *quirk;
+       int ret;
 
        indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
        if (!indio_dev)

@@ -191,6 +238,19 @@ static int acpi_als_add(struct acpi_device *device)
        als->device = device;
        mutex_init(&als->lock);
 
+       dmiid = dmi_first_match(acpi_als_quirks);
+       if (dmiid) {
+               dev_dbg(&device->dev, "Detected quirk for %s\n", dmiid->ident);
+               quirk = dmiid->driver_data;
+               ret = quirk->add(als);
+               if (ret) {
+                       dev_err(&device->dev,
+                               "Executing quirk for %s failed - %d",
+                               dmiid->ident, ret);
+                       return ret;
+               }
+       }
+
        indio_dev->name = ACPI_ALS_DEVICE_NAME;
        indio_dev->dev.parent = &device->dev;
        indio_dev->info = &acpi_als_info;
--

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marek Vasut

2017-01-20 10:35 UTC #3 lore raw

Quoted 25 lines · Matt Ranostay

Quoted 24 lines · Josef Gajdusek

ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral. Otherwise, reads just return 0.

Signed-off-by Josef Gajdusek <[email protected]>

---
 drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

drivers/iio/light/acpi-als.c +1 -0

index f0b47c5..43417c9 100644
@@ -30,6 +30,7 @@
 #include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/dmi.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>

@@ -39,6 +40,9 @@
 #define ACPI_ALS_DEVICE_NAME           "acpi-als"
 #define ACPI_ALS_NOTIFY_ILLUMINANCE    0x80

Quoted 5 lines

drivers/iio/light/acpi-als.c line 44
Quoted 2 lines
drivers/iio/light/acpi-als.c +2 -0
+#define ACPI_ALS_ASUS_TALS             "\\_SB.PCI0.LPCB.EC0.TALS"
+#define ACPI_ALS_ASUS_ALSC             "\\_SB.ATKD.ALSC"
Not sure I like these one off defines... this could easily get out of hand. Also too lazy to pop out the ACPI spec what does TALS and ALSC mean?

Well it is better than hard-coding those directly into the code, on the other hand, it seems like these are unrelated to ALS and maybe should go into some ASUS ACPI driver instead or something ... maybe into drivers/platform/x86/asus-laptop.c ?

CCing the ASUS folks.

Quoted 33 lines

Quoted 20 lines

drivers/iio/light/acpi-als.c +13 -0

+
 ACPI_MODULE_NAME("acpi-als");
 
 /*

@@ -170,6 +174,46 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
        return IIO_VAL_INT;
 }
 
+static int acpi_als_quirk_asus_zenbook_add(struct acpi_als *als)
+{
+       acpi_status status;
+
+       status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
+       if (ACPI_FAILURE(status)) {
+               ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+                               ACPI_ALS_ASUS_TALS));
+               return -EIO;
+       }
+       status = acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
+       if (ACPI_FAILURE(status)) {

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 198

Quoted 10 lines

drivers/iio/light/acpi-als.c +10 -0

+               ACPI_EXCEPTION((AE_INFO, status, "Error enabling ALS by %s",
+                               ACPI_ALS_ASUS_ALSC));
+               return -EIO;
+       }
+
+       return 0;
+}
+
+struct quirk_entry {
+       int (*add)(struct acpi_als *als);

Maybe put the defines above in here.

Quoted 3 lines

     .tals  = "\\_SB.PCI0.LPCB.EC0.TALS",
     .alsc = ""\\_SB.ATKD.ALSC"

I don't think this'd make much good as they are not standard ACPI functions IMO, the next platform will have different ad-hoc ones.

Quoted 7 lines

drivers/iio/light/acpi-als.c +7 -0

+};
+
+static struct quirk_entry acpi_als_quirk_asus_zenbook = {
+       .add = acpi_als_quirk_asus_zenbook_add
+};
+
+static const struct dmi_system_id acpi_als_quirks[] = {

Quoted 8 lines

drivers/iio/light/acpi-als.c static int acpi_als_read_raw(struct iio_dev *indio_dev, · line 210
Quoted 5 lines
drivers/iio/light/acpi-als.c +5 -0
+       {
+               .ident = "ASUSTeK COMPUTER INC. UX",
+               .matches = {
+                       DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+                       DMI_MATCH(DMI_PRODUCT_NAME, "UX"),
Really is the DMI_PRODUCT_NAME only two characters? You are likely going to need another DMI property to match on.

Agreed

Quoted 50 lines

Thanks,

Matt

Quoted 46 lines

drivers/iio/light/acpi-als.c +22 -1

+               },
+               .driver_data = &acpi_als_quirk_asus_zenbook,
+       },
+       {},
+};
+
 static const struct iio_info acpi_als_info = {
        .driver_module          = THIS_MODULE,
        .read_raw               = acpi_als_read_raw,

@@ -180,6 +224,9 @@ static int acpi_als_add(struct acpi_device *device)
        struct acpi_als *als;
        struct iio_dev *indio_dev;
        struct iio_buffer *buffer;
+       const struct dmi_system_id *dmiid;
+       struct quirk_entry *quirk;
+       int ret;
 
        indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
        if (!indio_dev)

@@ -191,6 +238,19 @@ static int acpi_als_add(struct acpi_device *device)
        als->device = device;
        mutex_init(&als->lock);
 
+       dmiid = dmi_first_match(acpi_als_quirks);
+       if (dmiid) {
+               dev_dbg(&device->dev, "Detected quirk for %s\n", dmiid->ident);
+               quirk = dmiid->driver_data;
+               ret = quirk->add(als);
+               if (ret) {
+                       dev_err(&device->dev,
+                               "Executing quirk for %s failed - %d",
+                               dmiid->ident, ret);
+                       return ret;
+               }
+       }
+
        indio_dev->name = ACPI_ALS_DEVICE_NAME;
        indio_dev->dev.parent = &device->dev;
        indio_dev->info = &acpi_als_info;
--

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Best regards,
Marek Vasut