Re: [PATCH v1 1/1] iio: light: ltr501: Drop most likely fake ACPI ID

[Hans de Goede <hdegoede@redhat.com>]

Hi Andy,

On 9/12/24 3:51 PM, Hans de Goede wrote:
> Hi,
> 
> On 9/11/24 11:22 PM, Andy Shevchenko wrote:
>> The commit in question does not proove that ACPI ID exists.
>> Quite likely it was a cargo cult addition while doint that
>> for DT-based enumeration.  Drop most likely fake ACPI ID.
>>
>> Googling for LTERxxxx gives no useful results in regard to DSDT.
>> Moreover, there is no "LTER" official vendor ID in the registry.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

On 9/13/24 11:31 AM, Andy Shevchenko wrote:
> Have you grepped over your collection of real DSDTs?

Yes I did, but I just double-checked looking for only LTER and there
are several DSDTs using LTER0303 for an ambient light sensor.

duckduckgo-ing for LTER0303 finds:

https://www.catalog.update.microsoft.com/Search.aspx?q=lter0303

which is actually quite an interesting URL to search for ACPI
HID-s used in any Windows drivers.

Checking for LTER0301:

https://www.catalog.update.microsoft.com/Search.aspx?q=lter0301

Shows that that HID is also actually used, so:

> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Correction, at least the LTER0301 ACPI id seems to actually be real:

https://www.catalog.update.microsoft.com/Search.aspx?q=lter0301

So NACK for dropping all 3 HIDs.

It seems to me that the LTER05xx HIDs can be dropped and
a LTER0303 HID should be added instead of dropping all HIDs.

Note I do not have any hw with a ltr303 light sensor, so
I cannot test this.

Regards,

Hans




>> ---
>>  drivers/iio/light/ltr501.c | 23 -----------------------
>>  1 file changed, 23 deletions(-)
>>
>> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
>> index 8c516ede9116..0e0420573286 100644
>> --- a/drivers/iio/light/ltr501.c
>> +++ b/drivers/iio/light/ltr501.c
>> @@ -15,7 +15,6 @@
>>  #include <linux/err.h>
>>  #include <linux/delay.h>
>>  #include <linux/regmap.h>
>> -#include <linux/acpi.h>
>>  #include <linux/regulator/consumer.h>
>>  
>>  #include <linux/iio/iio.h>
>> @@ -1422,17 +1421,6 @@ static int ltr501_powerdown(struct ltr501_data *data)
>>  				  data->ps_contr & ~LTR501_CONTR_ACTIVE);
>>  }
>>  
>> -static const char *ltr501_match_acpi_device(struct device *dev, int *chip_idx)
>> -{
>> -	const struct acpi_device_id *id;
>> -
>> -	id = acpi_match_device(dev->driver->acpi_match_table, dev);
>> -	if (!id)
>> -		return NULL;
>> -	*chip_idx = id->driver_data;
>> -	return dev_name(dev);
>> -}
>> -
>>  static int ltr501_probe(struct i2c_client *client)
>>  {
>>  	const struct i2c_device_id *id = i2c_client_get_device_id(client);
>> @@ -1523,8 +1511,6 @@ static int ltr501_probe(struct i2c_client *client)
>>  	if (id) {
>>  		name = id->name;
>>  		chip_idx = id->driver_data;
>> -	} else  if (ACPI_HANDLE(&client->dev)) {
>> -		name = ltr501_match_acpi_device(&client->dev, &chip_idx);
>>  	} else {
>>  		return -ENODEV;
>>  	}
>> @@ -1609,14 +1595,6 @@ static int ltr501_resume(struct device *dev)
>>  
>>  static DEFINE_SIMPLE_DEV_PM_OPS(ltr501_pm_ops, ltr501_suspend, ltr501_resume);
>>  
>> -static const struct acpi_device_id ltr_acpi_match[] = {
>> -	{ "LTER0501", ltr501 },
>> -	{ "LTER0559", ltr559 },
>> -	{ "LTER0301", ltr301 },
>> -	{ },
>> -};
>> -MODULE_DEVICE_TABLE(acpi, ltr_acpi_match);
>> -
>>  static const struct i2c_device_id ltr501_id[] = {
>>  	{ "ltr501", ltr501 },
>>  	{ "ltr559", ltr559 },
>> @@ -1640,7 +1618,6 @@ static struct i2c_driver ltr501_driver = {
>>  		.name   = LTR501_DRV_NAME,
>>  		.of_match_table = ltr501_of_match,
>>  		.pm	= pm_sleep_ptr(&ltr501_pm_ops),
>> -		.acpi_match_table = ltr_acpi_match,
>>  	},
>>  	.probe = ltr501_probe,
>>  	.remove	= ltr501_remove,
>