| Message ID | 20240911212202.2892451-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1,1/1] iio: light: ltr501: Drop most likely fake ACPI ID | expand |
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> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> 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(<r501_pm_ops), > - .acpi_match_table = ltr_acpi_match, > }, > .probe = ltr501_probe, > .remove = ltr501_remove,
On Thu, Sep 12, 2024 at 03:51:09PM +0200, 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> > > Thanks, patch looks good to me: Have you grepped over your collection of real DSDTs? > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Thank you!
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(<r501_pm_ops), >> - .acpi_match_table = ltr_acpi_match, >> }, >> .probe = ltr501_probe, >> .remove = ltr501_remove, >
On Fri, 13 Sep 2024 12:31:31 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, Sep 12, 2024 at 03:51:09PM +0200, 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> > > > > Thanks, patch looks good to me: > > Have you grepped over your collection of real DSDTs? > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Thank you! > I'll pick these up in the meantime. Applied to the testing branch of iio.git.
Hi, On 9/14/24 4:25 PM, Jonathan Cameron wrote: > On Fri, 13 Sep 2024 12:31:31 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> On Thu, Sep 12, 2024 at 03:51:09PM +0200, 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> >>> >>> Thanks, patch looks good to me: >> >> Have you grepped over your collection of real DSDTs? >> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> >> Thank you! >> > I'll pick these up in the meantime. Applied to the testing > branch of iio.git. As mentioned earlier today, at least the LTER0301 ACPI Hardware ID is real, so please drop this one. The kmx61 patch is fine to keep. Regards, Hans
On Sat, 14 Sep 2024 16:30:00 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 9/14/24 4:25 PM, Jonathan Cameron wrote: > > On Fri, 13 Sep 2024 12:31:31 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > >> On Thu, Sep 12, 2024 at 03:51:09PM +0200, 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> > >>> > >>> Thanks, patch looks good to me: > >> > >> Have you grepped over your collection of real DSDTs? > >> > >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > >> > >> Thank you! > >> > > I'll pick these up in the meantime. Applied to the testing > > branch of iio.git. > > As mentioned earlier today, at least the LTER0301 ACPI Hardware ID > is real, so please drop this one. The kmx61 patch is fine to keep. Done. Thanks, J > > Regards, > > Hans > > >
On Sat, Sep 14, 2024 at 03:45:58PM +0200, Hans de Goede wrote: > On 9/12/24 3:51 PM, Hans de Goede wrote: > > On 9/11/24 11:22 PM, Andy Shevchenko wrote: > 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. Very good finding! Bookmarked to check any other ACPI ID case with that as well. > 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. I'll update the patch with reference to that catalog. > Note I do not have any hw with a ltr303 light sensor, so > I cannot test this. Neither can I. So, let's drop 'LTER05' and add a comment WRT the 0x01.
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(<r501_pm_ops), - .acpi_match_table = ltr_acpi_match, }, .probe = ltr501_probe, .remove = ltr501_remove,
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> --- drivers/iio/light/ltr501.c | 23 ----------------------- 1 file changed, 23 deletions(-)