[PATCH v1 1/3] rtc: ds1307: Remove non-valid ACPI IDs

[PATCH v1 1/3] rtc: ds1307: Remove non-valid ACPI IDs

[Andy Shevchenko]

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(-)

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

[PATCH v1 2/3] rtc: ds1307: Make use of device properties

[Andy Shevchenko]

Device property API allows to gather device resources from different sources,
such as ACPI. Convert the drivers to unleash the power of device property API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/rtc/rtc-ds1307.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index fcb8e281abd5..40300c872337 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -12,7 +12,8 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/rtc/ds1307.h>
 #include <linux/rtc.h>
 #include <linux/slab.h>
@@ -30,6 +31,7 @@
  * That's a natural job for a factory or repair bench.
  */
 enum ds_type {
+	unknown_ds_type, /* always first and 0 */
 	ds_1307,
 	ds_1308,
 	ds_1337,
@@ -1613,8 +1615,11 @@ static struct clk_init_data ds3231_clks_init[] = {
 
 static int ds3231_clks_register(struct ds1307 *ds1307)
 {
-	struct device_node *node = ds1307->dev->of_node;
 	struct clk_onecell_data	*onecell;
+	const char *ds3231_clks_names[] = {
+		[DS3231_CLK_SQW] = ds3231_clks_init[DS3231_CLK_SQW].name,
+		[DS3231_CLK_32KHZ] = ds3231_clks_init[DS3231_CLK_32KHZ].name,
+	};
 	int i;
 
 	onecell = devm_kzalloc(ds1307->dev, sizeof(*onecell), GFP_KERNEL);
@@ -1627,6 +1632,11 @@ static int ds3231_clks_register(struct ds1307 *ds1307)
 	if (!onecell->clks)
 		return -ENOMEM;
 
+	/* optional override of the clockname */
+	device_property_read_string_array(ds1307->dev, "clock-output-names",
+					  ds3231_clks_names,
+					  ARRAY_SIZE(ds3231_clks_names));
+
 	for (i = 0; i < ARRAY_SIZE(ds3231_clks_init); i++) {
 		struct clk_init_data init = ds3231_clks_init[i];
 
@@ -1637,9 +1647,7 @@ static int ds3231_clks_register(struct ds1307 *ds1307)
 		if (i == DS3231_CLK_SQW && test_bit(HAS_ALARM, &ds1307->flags))
 			continue;
 
-		/* optional override of the clockname */
-		of_property_read_string_index(node, "clock-output-names", i,
-					      &init.name);
+		init.name = ds3231_clks_names[i];
 		ds1307->clks[i].init = &init;
 
 		onecell->clks[i] = devm_clk_register(ds1307->dev,
@@ -1648,10 +1656,9 @@ static int ds3231_clks_register(struct ds1307 *ds1307)
 			return PTR_ERR(onecell->clks[i]);
 	}
 
-	if (!node)
-		return 0;
-
-	of_clk_add_provider(node, of_clk_src_onecell_get, onecell);
+	if (ds1307->dev->of_node)
+		of_clk_add_provider(ds1307->dev->of_node,
+				    of_clk_src_onecell_get, onecell);
 
 	return 0;
 }
@@ -1735,6 +1742,7 @@ static int ds1307_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ds1307		*ds1307;
+	const void		*match;
 	int			err = -ENODEV;
 	int			tmp;
 	const struct chip_desc	*chip;
@@ -1760,9 +1768,9 @@ static int ds1307_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ds1307);
 
-	if (client->dev.of_node) {
-		ds1307->type = (enum ds_type)
-			of_device_get_match_data(&client->dev);
+	match = device_get_match_data(&client->dev);
+	if (match) {
+		ds1307->type = (enum ds_type)match;
 		chip = &chips[ds1307->type];
 	} else if (id) {
 		chip = &chips[id->driver_data];
@@ -1786,7 +1794,6 @@ static int ds1307_probe(struct i2c_client *client,
 			     trickle_charger_setup);
 	}
 
-#ifdef CONFIG_OF
 /*
  * For devices with no IRQ directly connected to the SoC, the RTC chip
  * can be forced as a wakeup source by stating that explicitly in
@@ -1795,10 +1802,8 @@ static int ds1307_probe(struct i2c_client *client,
  * This will guarantee the 'wakealarm' sysfs entry is available on the device,
  * if supported by the RTC.
  */
-	if (chip->alarm && of_property_read_bool(client->dev.of_node,
-						 "wakeup-source"))
+	if (chip->alarm && device_property_read_bool(&client->dev, "wakeup-source"))
 		ds1307_can_wakeup_device = true;
-#endif
 
 	switch (ds1307->type) {
 	case ds_1337:
-- 
2.28.0

[PATCH v1 3/3] rtc: ds1307: Drop of_match_ptr and CONFIG_OF protections

[Andy Shevchenko]

These prevent use of this driver with ACPI via PRP0001.
Drop them to remove this restriction.

Also added mod_devicetable.h include given use of struct of_device_id.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/rtc/rtc-ds1307.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 40300c872337..1d8b711ec90e 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -11,8 +11,8 @@
 #include <linux/bcd.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/property.h>
 #include <linux/rtc/ds1307.h>
 #include <linux/rtc.h>
@@ -1091,7 +1091,6 @@ static const struct i2c_device_id ds1307_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ds1307_id);
 
-#ifdef CONFIG_OF
 static const struct of_device_id ds1307_of_match[] = {
 	{
 		.compatible = "dallas,ds1307",
@@ -1168,7 +1167,6 @@ static const struct of_device_id ds1307_of_match[] = {
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ds1307_of_match);
-#endif
 
 /*
  * The ds1337 and ds1339 both have two alarms, but we only use the first
@@ -2036,7 +2034,7 @@ static int ds1307_probe(struct i2c_client *client,
 static struct i2c_driver ds1307_driver = {
 	.driver = {
 		.name	= "rtc-ds1307",
-		.of_match_table = of_match_ptr(ds1307_of_match),
+		.of_match_table = ds1307_of_match,
 	},
 	.probe		= ds1307_probe,
 	.id_table	= ds1307_id,
-- 
2.28.0

Re: [PATCH v1 2/3] rtc: ds1307: Make use of device properties

[Andy Shevchenko]

On Thu, Nov 12, 2020 at 05:57:52PM +0200, Andy Shevchenko wrote:
> Device property API allows to gather device resources from different sources,
> such as ACPI. Convert the drivers to unleash the power of device property API.

> +	const char *ds3231_clks_names[] = {
> +		[DS3231_CLK_SQW] = ds3231_clks_init[DS3231_CLK_SQW].name,
> +		[DS3231_CLK_32KHZ] = ds3231_clks_init[DS3231_CLK_32KHZ].name,
> +	};

I realised I can do this slightly better, i.e. drop the names from the struct
and move them here while declaring latter as static and moving outside of the
function.

In any case I'll wait for comments for the rest and this patch before updating.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] rtc: ds1307: Remove non-valid ACPI IDs

[Alexandre Belloni]

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

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 1/3] rtc: ds1307: Remove non-valid ACPI IDs

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] rtc: ds1307: Remove non-valid ACPI IDs

[Rafael J. Wysocki]

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
>

Re: [PATCH v1 1/3] rtc: ds1307: Remove non-valid ACPI IDs

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko