[PATCH v2] iio: light: acpi-als: Properly enable on ASUS Zenbooks

[PATCH v2] iio: light: acpi-als: Properly enable on ASUS Zenbooks

[Josef Gajdusek]

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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
index f0b47c5..ccd5787 100644
--- a/drivers/iio/light/acpi-als.c
+++ b/drivers/iio/light/acpi-als.c
@@ -39,6 +39,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 +173,16 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
 	return IIO_VAL_INT;
 }
 
+static void acpi_als_quirk_asus(struct acpi_als *als)
+{
+	acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
+	acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
+}
+
+static void (*acpi_als_quirks[])(struct acpi_als *als) = {
+	acpi_als_quirk_asus,
+};
+
 static const struct iio_info acpi_als_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= acpi_als_read_raw,
@@ -180,6 +193,7 @@ static int acpi_als_add(struct acpi_device *device)
 	struct acpi_als *als;
 	struct iio_dev *indio_dev;
 	struct iio_buffer *buffer;
+	int i;
 
 	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
 	if (!indio_dev)
@@ -191,6 +205,9 @@ static int acpi_als_add(struct acpi_device *device)
 	als->device = device;
 	mutex_init(&als->lock);
 
+	for (i = 0; i < ARRAY_SIZE(acpi_als_quirks); i++)
+		acpi_als_quirks[i](als);
+
 	indio_dev->name = ACPI_ALS_DEVICE_NAME;
 	indio_dev->dev.parent = &device->dev;
 	indio_dev->info = &acpi_als_info;
-- 
2.10.2

Re: [PATCH v2] iio: light: acpi-als: Properly enable on ASUS Zenbooks

[Marek Vasut]

On 01/19/2017 12:48 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>
> ---
>  drivers/iio/light/acpi-als.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
> index f0b47c5..ccd5787 100644
> --- a/drivers/iio/light/acpi-als.c
> +++ b/drivers/iio/light/acpi-als.c
> @@ -39,6 +39,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 +173,16 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>  	return IIO_VAL_INT;
>  }
>  
> +static void acpi_als_quirk_asus(struct acpi_als *als)
> +{
> +	acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
> +	acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);

This will run on ALL systems, right ? Also, error checking disappeared ?

> +}
> +
> +static void (*acpi_als_quirks[])(struct acpi_als *als) = {
> +	acpi_als_quirk_asus,
> +};
> +
>  static const struct iio_info acpi_als_info = {
>  	.driver_module		= THIS_MODULE,
>  	.read_raw		= acpi_als_read_raw,
> @@ -180,6 +193,7 @@ static int acpi_als_add(struct acpi_device *device)
>  	struct acpi_als *als;
>  	struct iio_dev *indio_dev;
>  	struct iio_buffer *buffer;
> +	int i;
>  
>  	indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
>  	if (!indio_dev)
> @@ -191,6 +205,9 @@ static int acpi_als_add(struct acpi_device *device)
>  	als->device = device;
>  	mutex_init(&als->lock);
>  
> +	for (i = 0; i < ARRAY_SIZE(acpi_als_quirks); i++)
> +		acpi_als_quirks[i](als);
> +
>  	indio_dev->name = ACPI_ALS_DEVICE_NAME;
>  	indio_dev->dev.parent = &device->dev;
>  	indio_dev->info = &acpi_als_info;
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH v2] iio: light: acpi-als: Properly enable on ASUS Zenbooks

[atx]

January 19 2017 4:49 PM, "Marek Vasut" <marex@denx.de> wrote:
> On 01/19/2017 12:48 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>
>> ---
>> drivers/iio/light/acpi-als.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>> 
>> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
>> index f0b47c5..ccd5787 100644
>> --- a/drivers/iio/light/acpi-als.c
>> +++ b/drivers/iio/light/acpi-als.c
>> @@ -39,6 +39,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 +173,16 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>> return IIO_VAL_INT;
>> }
>> 
>> +static void acpi_als_quirk_asus(struct acpi_als *als)
>> +{
>> + acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
>> + acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
> 
> This will run on ALL systems, right ? Also, error checking disappeared ?
> 

Yes, that's intended. Should I add some DMI checks to only run on
ASUS/ASUS Zenbooks?

As for error checking, this code behaves exactly like v1, except
that the xor warning is removed, which means that it is not needed
to explicitly get the handles. The acpi_exceute_simple_method call just
fails if it can't find the object.

Distinguishing between the "object not found" and "failed to call method" scenarios
seemed a bit overly verbose to me especially as it would probably be overkill to abort
the driver initialization because of that.

>> +}
>> +
>> +static void (*acpi_als_quirks[])(struct acpi_als *als) = {
>> + acpi_als_quirk_asus,
>> +};
>> +
>> static const struct iio_info acpi_als_info = {
>> .driver_module = THIS_MODULE,
>> .read_raw = acpi_als_read_raw,
>> @@ -180,6 +193,7 @@ static int acpi_als_add(struct acpi_device *device)
>> struct acpi_als *als;
>> struct iio_dev *indio_dev;
>> struct iio_buffer *buffer;
>> + int i;
>> 
>> indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
>> if (!indio_dev)
>> @@ -191,6 +205,9 @@ static int acpi_als_add(struct acpi_device *device)
>> als->device = device;
>> mutex_init(&als->lock);
>> 
>> + for (i = 0; i < ARRAY_SIZE(acpi_als_quirks); i++)
>> + acpi_als_quirks[i](als);
>> +
>> indio_dev->name = ACPI_ALS_DEVICE_NAME;
>> indio_dev->dev.parent = &device->dev;
>> indio_dev->info = &acpi_als_info;
> 
> --
> Best regards,
> Marek Vasut

Re: [PATCH v2] iio: light: acpi-als: Properly enable on ASUS Zenbooks

[Marek Vasut]

On 01/19/2017 05:19 PM, atx@atx.name wrote:
> January 19 2017 4:49 PM, "Marek Vasut" <marex@denx.de> wrote:
>> On 01/19/2017 12:48 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>
>>> ---
>>> drivers/iio/light/acpi-als.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/iio/light/acpi-als.c b/drivers/iio/light/acpi-als.c
>>> index f0b47c5..ccd5787 100644
>>> --- a/drivers/iio/light/acpi-als.c
>>> +++ b/drivers/iio/light/acpi-als.c
>>> @@ -39,6 +39,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 +173,16 @@ static int acpi_als_read_raw(struct iio_dev *indio_dev,
>>> return IIO_VAL_INT;
>>> }
>>>
>>> +static void acpi_als_quirk_asus(struct acpi_als *als)
>>> +{
>>> + acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_TALS, 1);
>>> + acpi_execute_simple_method(NULL, ACPI_ALS_ASUS_ALSC, 1);
>>
>> This will run on ALL systems, right ? Also, error checking disappeared ?
>>
> 
> Yes, that's intended. Should I add some DMI checks to only run on
> ASUS/ASUS Zenbooks?

Definitely, it doesn't seem right to execute random stuff on hardware
where such stuff is undefined.

> As for error checking, this code behaves exactly like v1, except
> that the xor warning is removed, which means that it is not needed
> to explicitly get the handles. The acpi_exceute_simple_method call just
> fails if it can't find the object.

But the driver will register even though the call fails , that's wrong.
The probe must stop if there is a failure.

> Distinguishing between the "object not found" and "failed to call method" scenarios
> seemed a bit overly verbose to me especially as it would probably be overkill to abort
> the driver initialization because of that.

If something which should be present (like the acpi method) is not
present and/or fails, the driver probe should abort.

>>> +}
>>> +
>>> +static void (*acpi_als_quirks[])(struct acpi_als *als) = {
>>> + acpi_als_quirk_asus,
>>> +};
>>> +
>>> static const struct iio_info acpi_als_info = {
>>> .driver_module = THIS_MODULE,
>>> .read_raw = acpi_als_read_raw,
>>> @@ -180,6 +193,7 @@ static int acpi_als_add(struct acpi_device *device)
>>> struct acpi_als *als;
>>> struct iio_dev *indio_dev;
>>> struct iio_buffer *buffer;
>>> + int i;
>>>
>>> indio_dev = devm_iio_device_alloc(&device->dev, sizeof(*als));
>>> if (!indio_dev)
>>> @@ -191,6 +205,9 @@ static int acpi_als_add(struct acpi_device *device)
>>> als->device = device;
>>> mutex_init(&als->lock);
>>>
>>> + for (i = 0; i < ARRAY_SIZE(acpi_als_quirks); i++)
>>> + acpi_als_quirks[i](als);
>>> +
>>> indio_dev->name = ACPI_ALS_DEVICE_NAME;
>>> indio_dev->dev.parent = &device->dev;
>>> indio_dev->info = &acpi_als_info;
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut