* [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks
@ 2017-01-19 19:01 Josef Gajdusek
2017-01-19 22:04 ` Marek Vasut
2017-01-20 2:53 ` Matt Ranostay
0 siblings, 2 replies; 4+ messages in thread
From: Josef Gajdusek @ 2017-01-19 19:01 UTC (permalink / raw)
To: linux-iio
Cc: jic23, knaack.h, lars, pmeerw, maitysanchayan, gregor.boirie,
linux-kernel, rui.zhang, marxin.liska, marex, atx
ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral.
Otherwise, reads just return 0.
Signed-off-by: Josef Gajdusek <atx@atx.name>
---
drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index f0b47c5..43417c9 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -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;
--
2.10.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks
2017-01-19 19:01 [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks Josef Gajdusek
@ 2017-01-19 22:04 ` Marek Vasut
2017-01-20 2:53 ` Matt Ranostay
1 sibling, 0 replies; 4+ messages in thread
From: Marek Vasut @ 2017-01-19 22:04 UTC (permalink / raw)
To: Josef Gajdusek, linux-iio
Cc: jic23, knaack.h, lars, pmeerw, maitysanchayan, gregor.boirie,
linux-kernel, rui.zhang, marxin.liska
On 01/19/2017 08:01 PM, Josef Gajdusek wrote:
> ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral.
> Otherwise, reads just return 0.
>
> Signed-off-by: Josef Gajdusek <atx@atx.name>
Changelog is missing ...
> ---
> drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index f0b47c5..43417c9 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -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));
This doesn't unwind the previous operation. If ALSC fails, won't the
TALS still be enabled ?
> + return -EIO;
Isn't there some way to extract error code from the $status and
propagating it instead of hard-coding EIO here ?
> + }
> +
> + 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"),
I'm no expert on DMI, but is this unique enough ?
> + },
> + .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;
What happens if some entry has quirk == NULL ? :)
> + 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.
> + 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;
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks
2017-01-19 19:01 [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks Josef Gajdusek
2017-01-19 22:04 ` Marek Vasut
@ 2017-01-20 2:53 ` Matt Ranostay
2017-01-20 10:35 ` Marek Vasut
1 sibling, 1 reply; 4+ messages in thread
From: Matt Ranostay @ 2017-01-20 2:53 UTC (permalink / raw)
To: Josef Gajdusek
Cc: linux-iio, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, maitysanchayan, gregor.boirie,
linux-kernel, rui.zhang, marxin.liska, marex
On Thu, Jan 19, 2017 at 11:01 AM, Josef Gajdusek <atx@atx.name> wrote:
> ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral.
> Otherwise, reads just return 0.
>
> Signed-off-by: Josef Gajdusek <atx@atx.name>
> ---
> drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index f0b47c5..43417c9 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -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"
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?
> +
> 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);
Maybe put the defines above in here.
.tals = "\\_SB.PCI0.LPCB.EC0.TALS",
.alsc = ""\\_SB.ATKD.ALSC"
.
> +};
> +
> +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
> + },
> + .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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks
2017-01-20 2:53 ` Matt Ranostay
@ 2017-01-20 10:35 ` Marek Vasut
0 siblings, 0 replies; 4+ messages in thread
From: Marek Vasut @ 2017-01-20 10:35 UTC (permalink / raw)
To: Matt Ranostay, Josef Gajdusek
Cc: linux-iio, Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, maitysanchayan, gregor.boirie,
linux-kernel, rui.zhang, marxin.liska, Corentin Chary,
acpi4asus-user
On 01/20/2017 03:53 AM, Matt Ranostay wrote:
> On Thu, Jan 19, 2017 at 11:01 AM, Josef Gajdusek <atx@atx.name> wrote:
>> ASUS Zenbooks need several special ACPI calls to enable the ALS peripheral.
>> Otherwise, reads just return 0.
>>
>> Signed-off-by: Josef Gajdusek <atx@atx.name>
>> ---
>> drivers/iio/light/acpi-als.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 60 insertions(+)
>>
>> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
>> index f0b47c5..43417c9 100644
>> --- a/drivers/iio/light/acpi-als.c
>> +++ b/drivers/iio/light/acpi-als.c
>> @@ -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"
>
> 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.
>> +
>> 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);
>
> Maybe put the defines above in here.
>
> .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.
>> +};
>> +
>> +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.
Agreed
> Thanks,
>
> Matt
>
>> + },
>> + .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 majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-20 10:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-19 19:01 [PATCH v3] iio: light: acpi-als: Properly enable on ASUS Zenbooks Josef Gajdusek
2017-01-19 22:04 ` Marek Vasut
2017-01-20 2:53 ` Matt Ranostay
2017-01-20 10:35 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.