| Message ID | 20201112155753.36834-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1,1/3] rtc: ds1307: Remove non-valid ACPI IDs | expand |
Hi, On 12/11/2020 17:57:51+0200, Andy Shevchenko wrote: > The commit 9c19b8930d2c ("rtc: ds1307: Add ACPI support") added non-valid > ACPI IDs (all of them abusing ACPI specification). Moreover there is > no even a single evidence that vendor registered any of such device. > > Remove broken ACPI IDs from the driver. For prototyping one may use PRP0001 > with device tree defined bindings. The following patches will add support > of that to the driver. I'm intrigued, how does PRP0001 work? Where would the device tree come from? You probably want to have a look at: https://lore.kernel.org/linux-rtc/20201112130734.331094-3-ch@denx.de/T/#u
On Thu, Nov 12, 2020 at 05:36:17PM +0100, Alexandre Belloni wrote: > On 12/11/2020 17:57:51+0200, Andy Shevchenko wrote: > > The commit 9c19b8930d2c ("rtc: ds1307: Add ACPI support") added non-valid > > ACPI IDs (all of them abusing ACPI specification). Moreover there is > > no even a single evidence that vendor registered any of such device. > > > > Remove broken ACPI IDs from the driver. For prototyping one may use PRP0001 > > with device tree defined bindings. The following patches will add support > > of that to the driver. > > I'm intrigued, how does PRP0001 work? Where would the device tree come > from? From nowhere :-) There is no device tree. The properties are coming from _DSD ACPI Method with compatible string provided. ACPI glue layer provides a mechanism to parse that and match against OF ID table (even when CONFIG_OF=n!). More about PRP0001 is in documentation [1]. > You probably want to have a look at: > https://lore.kernel.org/linux-rtc/20201112130734.331094-3-ch@denx.de/T/#u Thanks for heads up! I'll check that thread and answer there. [1]: Documentation/firmware-guide/acpi/enumeration.rst
On Thu, Nov 12, 2020 at 4:58 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > The commit 9c19b8930d2c ("rtc: ds1307: Add ACPI support") added non-valid s/non-valid/invalid/ ? > ACPI IDs (all of them abusing ACPI specification). Moreover there is > no even a single evidence that vendor registered any of such device. "not even" and "devices". > Remove broken ACPI IDs from the driver. For prototyping one may use PRP0001 > with device tree defined bindings. "with device properties adhering to a DT binding". ? > The following patches will add support of that to the driver. > > Link: https://uefi.org/PNP_ACPI_Registry > Cc: Tin Huynh <tnhuynh@apm.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/rtc/rtc-ds1307.c | 36 +----------------------------------- > 1 file changed, 1 insertion(+), 35 deletions(-) > > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c > index 9f5f54ca039d..fcb8e281abd5 100644 > --- a/drivers/rtc/rtc-ds1307.c > +++ b/drivers/rtc/rtc-ds1307.c > @@ -8,7 +8,6 @@ > * Copyright (C) 2012 Bertrand Achard (nvram access fixes) > */ > > -#include <linux/acpi.h> > #include <linux/bcd.h> > #include <linux/i2c.h> > #include <linux/init.h> > @@ -1169,31 +1168,6 @@ static const struct of_device_id ds1307_of_match[] = { > MODULE_DEVICE_TABLE(of, ds1307_of_match); > #endif > > -#ifdef CONFIG_ACPI > -static const struct acpi_device_id ds1307_acpi_ids[] = { > - { .id = "DS1307", .driver_data = ds_1307 }, > - { .id = "DS1308", .driver_data = ds_1308 }, > - { .id = "DS1337", .driver_data = ds_1337 }, > - { .id = "DS1338", .driver_data = ds_1338 }, > - { .id = "DS1339", .driver_data = ds_1339 }, > - { .id = "DS1388", .driver_data = ds_1388 }, > - { .id = "DS1340", .driver_data = ds_1340 }, > - { .id = "DS1341", .driver_data = ds_1341 }, > - { .id = "DS3231", .driver_data = ds_3231 }, > - { .id = "M41T0", .driver_data = m41t0 }, > - { .id = "M41T00", .driver_data = m41t00 }, > - { .id = "M41T11", .driver_data = m41t11 }, > - { .id = "MCP7940X", .driver_data = mcp794xx }, > - { .id = "MCP7941X", .driver_data = mcp794xx }, > - { .id = "PT7C4338", .driver_data = ds_1307 }, > - { .id = "RX8025", .driver_data = rx_8025 }, > - { .id = "ISL12057", .driver_data = ds_1337 }, > - { .id = "RX8130", .driver_data = rx_8130 }, > - { } > -}; > -MODULE_DEVICE_TABLE(acpi, ds1307_acpi_ids); > -#endif > - > /* > * The ds1337 and ds1339 both have two alarms, but we only use the first > * one (with a "seconds" field). For ds1337 we expect nINTA is our alarm > @@ -1794,14 +1768,7 @@ static int ds1307_probe(struct i2c_client *client, > chip = &chips[id->driver_data]; > ds1307->type = id->driver_data; > } else { > - const struct acpi_device_id *acpi_id; > - > - acpi_id = acpi_match_device(ACPI_PTR(ds1307_acpi_ids), > - ds1307->dev); > - if (!acpi_id) > - return -ENODEV; > - chip = &chips[acpi_id->driver_data]; > - ds1307->type = acpi_id->driver_data; > + return -ENODEV; > } > > want_irq = client->irq > 0 && chip->alarm; > @@ -2065,7 +2032,6 @@ static struct i2c_driver ds1307_driver = { > .driver = { > .name = "rtc-ds1307", > .of_match_table = of_match_ptr(ds1307_of_match), > - .acpi_match_table = ACPI_PTR(ds1307_acpi_ids), > }, > .probe = ds1307_probe, > .id_table = ds1307_id, > -- > 2.28.0 >
On Thu, Nov 12, 2020 at 08:01:37PM +0100, Rafael J. Wysocki wrote: > On Thu, Nov 12, 2020 at 4:58 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > The commit 9c19b8930d2c ("rtc: ds1307: Add ACPI support") added non-valid > > s/non-valid/invalid/ ? > > > ACPI IDs (all of them abusing ACPI specification). Moreover there is > > no even a single evidence that vendor registered any of such device. > > "not even" and "devices". > > > Remove broken ACPI IDs from the driver. For prototyping one may use PRP0001 > > with device tree defined bindings. > > "with device properties adhering to a DT binding". ? > > > The following patches will add support of that to the driver. Rafael, thanks for review, I will address them all in v2.
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c index 9f5f54ca039d..fcb8e281abd5 100644 --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -8,7 +8,6 @@ * Copyright (C) 2012 Bertrand Achard (nvram access fixes) */ -#include <linux/acpi.h> #include <linux/bcd.h> #include <linux/i2c.h> #include <linux/init.h> @@ -1169,31 +1168,6 @@ static const struct of_device_id ds1307_of_match[] = { MODULE_DEVICE_TABLE(of, ds1307_of_match); #endif -#ifdef CONFIG_ACPI -static const struct acpi_device_id ds1307_acpi_ids[] = { - { .id = "DS1307", .driver_data = ds_1307 }, - { .id = "DS1308", .driver_data = ds_1308 }, - { .id = "DS1337", .driver_data = ds_1337 }, - { .id = "DS1338", .driver_data = ds_1338 }, - { .id = "DS1339", .driver_data = ds_1339 }, - { .id = "DS1388", .driver_data = ds_1388 }, - { .id = "DS1340", .driver_data = ds_1340 }, - { .id = "DS1341", .driver_data = ds_1341 }, - { .id = "DS3231", .driver_data = ds_3231 }, - { .id = "M41T0", .driver_data = m41t0 }, - { .id = "M41T00", .driver_data = m41t00 }, - { .id = "M41T11", .driver_data = m41t11 }, - { .id = "MCP7940X", .driver_data = mcp794xx }, - { .id = "MCP7941X", .driver_data = mcp794xx }, - { .id = "PT7C4338", .driver_data = ds_1307 }, - { .id = "RX8025", .driver_data = rx_8025 }, - { .id = "ISL12057", .driver_data = ds_1337 }, - { .id = "RX8130", .driver_data = rx_8130 }, - { } -}; -MODULE_DEVICE_TABLE(acpi, ds1307_acpi_ids); -#endif - /* * The ds1337 and ds1339 both have two alarms, but we only use the first * one (with a "seconds" field). For ds1337 we expect nINTA is our alarm @@ -1794,14 +1768,7 @@ static int ds1307_probe(struct i2c_client *client, chip = &chips[id->driver_data]; ds1307->type = id->driver_data; } else { - const struct acpi_device_id *acpi_id; - - acpi_id = acpi_match_device(ACPI_PTR(ds1307_acpi_ids), - ds1307->dev); - if (!acpi_id) - return -ENODEV; - chip = &chips[acpi_id->driver_data]; - ds1307->type = acpi_id->driver_data; + return -ENODEV; } want_irq = client->irq > 0 && chip->alarm; @@ -2065,7 +2032,6 @@ static struct i2c_driver ds1307_driver = { .driver = { .name = "rtc-ds1307", .of_match_table = of_match_ptr(ds1307_of_match), - .acpi_match_table = ACPI_PTR(ds1307_acpi_ids), }, .probe = ds1307_probe, .id_table = ds1307_id,
The commit 9c19b8930d2c ("rtc: ds1307: Add ACPI support") added non-valid ACPI IDs (all of them abusing ACPI specification). Moreover there is no even a single evidence that vendor registered any of such device. Remove broken ACPI IDs from the driver. For prototyping one may use PRP0001 with device tree defined bindings. The following patches will add support of that to the driver. Link: https://uefi.org/PNP_ACPI_Registry Cc: Tin Huynh <tnhuynh@apm.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/rtc/rtc-ds1307.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-)