Adding I2C support to RX6110 RTC

PATCH Superseded Claudius Heine 9 participants 31 messages Nov 12, 2020 – Mar 17, 2021

Subsystems (2)

ACPI Real time clock (RTC)

lore

Share & tools

lore

Claudius Heine

author

2020-11-12 13:08 UTC #0 lore raw

v2 cover Adding I2C support to RX6110 RTC

Hi,

here is the 2nd version of the patchset that adds I2C support to the RX6110 RTC.

Changes from v1:
- moved common probing code into its own function rx6110_probe
- added a small patch to fix a type in the Kconfig

Claudius Heine (2):
rtc: rx6110: add i2c support
rtc: Kconfig: Fix typo in help message of rx 6110

Johannes Hahn (1):
rtc: rx6110: add ACPI bindings to I2C

 drivers/rtc/Kconfig      |  20 +++--
 drivers/rtc/rtc-rx6110.c | 177 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 166 insertions(+), 31 deletions(-)

Signature · 1 line

2.20.1

Claudius Heine

author

2020-11-12 13:08 UTC #1 lore raw

PATCH v2 1/3 rtc: rx6110: add i2c support

The RX6110 also supports I2C, so this patch adds support for it to the driver.

This also renames the SPI specific functions and variables to include _spi_ in their names.

Signed-off-by Claudius Heine <[email protected]>

Signed-off-by Henning Schild <[email protected]>

---
 drivers/rtc/Kconfig      |  20 ++---
 drivers/rtc/rtc-rx6110.c | 165 +++++++++++++++++++++++++++++++++------
 2 files changed, 154 insertions(+), 31 deletions(-)

drivers/rtc/Kconfig +11 -9

index 65ad9d0b47ab..1fe411ffb19c 100644
@@ -817,15 +817,6 @@ config RTC_DRV_RX4581
 	  This driver can also be built as a module. If so the module
 	  will be called rtc-rx4581.
 
-config RTC_DRV_RX6110
-	tristate "Epson RX-6110"
-	select REGMAP_SPI
-	help
-	  If you say yes here you will get support for the Epson RX-6610.
-
-	  This driver can also be built as a module. If so the module
-	  will be called rtc-rx6110.
-
 config RTC_DRV_RS5C348
 	tristate "Ricoh RS5C348A/B"
 	help

@@ -936,6 +927,17 @@ config RTC_DRV_RV3029_HWMON
 	  Say Y here if you want to expose temperature sensor data on
 	  rtc-rv3029.
 
+config RTC_DRV_RX6110
+	tristate "Epson RX-6110"
+	depends on RTC_I2C_AND_SPI
+	select REGMAP_SPI if SPI_MASTER
+	select REGMAP_I2C if I2C
+	help
+	  If you say yes here you will get support for the Epson RX-6610.
+
+	  This driver can also be built as a module. If so the module
+	  will be called rtc-rx6110.
+
 comment "Platform RTC drivers"
 
 # this 'CMOS' RTC driver is arch dependent because it requires

drivers/rtc/rtc-rx6110.c +143 ~~-22~~

208 lines not on this page · raw message

Signature · 1 line

2.20.1

Claudius Heine

author

2020-11-12 13:19 UTC #2 lore raw

Re: [PATCH v2 1/3] rtc: rx6110: add i2c support

Hi,

Sorry, I forgot to remove some unused variables (both err variables in the spi and i2c probe functions). Will fix that in the next patch version.

Cheers,
Claudius

Quoted 269 lines · Claudius Heine

The RX6110 also supports I2C, so this patch adds support for it to the driver.

This also renames the SPI specific functions and variables to include _spi_ in their names.

Signed-off-by Claudius Heine <[email protected]>

Signed-off-by Henning Schild <[email protected]>

---
  drivers/rtc/Kconfig      |  20 ++---
  drivers/rtc/rtc-rx6110.c | 165 +++++++++++++++++++++++++++++++++------
  2 files changed, 154 insertions(+), 31 deletions(-)

drivers/rtc/Kconfig +11 -9

index 65ad9d0b47ab..1fe411ffb19c 100644
@@ -817,15 +817,6 @@ config RTC_DRV_RX4581
  	  This driver can also be built as a module. If so the module
  	  will be called rtc-rx4581.
 
-config RTC_DRV_RX6110
-	tristate "Epson RX-6110"
-	select REGMAP_SPI
-	help
-	  If you say yes here you will get support for the Epson RX-6610.
-
-	  This driver can also be built as a module. If so the module
-	  will be called rtc-rx6110.
-
  config RTC_DRV_RS5C348
  	tristate "Ricoh RS5C348A/B"
  	help

@@ -936,6 +927,17 @@ config RTC_DRV_RV3029_HWMON
  	  Say Y here if you want to expose temperature sensor data on
  	  rtc-rv3029.
 
+config RTC_DRV_RX6110
+	tristate "Epson RX-6110"
+	depends on RTC_I2C_AND_SPI
+	select REGMAP_SPI if SPI_MASTER
+	select REGMAP_I2C if I2C
+	help
+	  If you say yes here you will get support for the Epson RX-6610.
+
+	  This driver can also be built as a module. If so the module
+	  will be called rtc-rx6110.
+
  comment "Platform RTC drivers"
 
  # this 'CMOS' RTC driver is arch dependent because it requires

drivers/rtc/rtc-rx6110.c +143 ~~-22~~

index 3a9eb7043f01..1e6e9b19c81c 100644
@@ -16,6 +16,7 @@
  #include <linux/of.h>
  #include <linux/of_device.h>
  #include <linux/spi/spi.h>
+#include <linux/i2c.h>
 
  /* RX-6110 Register definitions */
  #define RX6110_REG_SEC		0x10

@@ -310,6 +311,27 @@ static const struct rtc_class_ops rx6110_rtc_ops = {
  	.set_time = rx6110_set_time,
  };
 
+static int rx6110_probe(struct rx6110_data *rx6110, struct device *dev)
+{
+	int err;
+
+	rx6110->rtc = devm_rtc_device_register(dev,
+					       RX6110_DRIVER_NAME,
+					       &rx6110_rtc_ops, THIS_MODULE);
+
+	if (IS_ERR(rx6110->rtc))
+		return PTR_ERR(rx6110->rtc);
+
+	err = rx6110_init(rx6110);
+	if (err)
+		return err;
+
+	rx6110->rtc->max_user_freq = 1;
+
+	return 0;
+}
+
+#ifdef CONFIG_SPI_MASTER
  static struct regmap_config regmap_spi_config = {
  	.reg_bits = 8,
  	.val_bits = 8,

@@ -318,10 +340,10 @@ static struct regmap_config regmap_spi_config = {
  };
 
  /**
- * rx6110_probe - initialize rtc driver
+ * rx6110_spi_probe - initialize rtc driver
   * @spi: pointer to spi device
   */
-static int rx6110_probe(struct spi_device *spi)
+static int rx6110_spi_probe(struct spi_device *spi)
  {
  	struct rx6110_data *rx6110;
  	int err;

@@ -346,27 +368,14 @@ static int rx6110_probe(struct spi_device *spi)
 
  	spi_set_drvdata(spi, rx6110);
 
-	rx6110->rtc = devm_rtc_device_register(&spi->dev,
-					       RX6110_DRIVER_NAME,
-					       &rx6110_rtc_ops, THIS_MODULE);
-
-	if (IS_ERR(rx6110->rtc))
-		return PTR_ERR(rx6110->rtc);
-
-	err = rx6110_init(rx6110);
-	if (err)
-		return err;
-
-	rx6110->rtc->max_user_freq = 1;
-
-	return 0;
+	return rx6110_probe(rx6110, &spi->dev);
  }
 
-static const struct spi_device_id rx6110_id[] = {
+static const struct spi_device_id rx6110_spi_id[] = {
  	{ "rx6110", 0 },
  	{ }
  };
-MODULE_DEVICE_TABLE(spi, rx6110_id);
+MODULE_DEVICE_TABLE(spi, rx6110_spi_id);
 
  static const struct of_device_id rx6110_spi_of_match[] = {
  	{ .compatible = "epson,rx6110" },

@@ -374,16 +383,128 @@ static const struct of_device_id rx6110_spi_of_match[] = {
  };
  MODULE_DEVICE_TABLE(of, rx6110_spi_of_match);
 
-static struct spi_driver rx6110_driver = {
+static struct spi_driver rx6110_spi_driver = {
  	.driver = {
  		.name = RX6110_DRIVER_NAME,
  		.of_match_table = of_match_ptr(rx6110_spi_of_match),
  	},
-	.probe		= rx6110_probe,
-	.id_table	= rx6110_id,
+	.probe		= rx6110_spi_probe,
+	.id_table	= rx6110_spi_id,
  };
 
-module_spi_driver(rx6110_driver);
+static int rx6110_spi_register(void)
+{
+	return spi_register_driver(&rx6110_spi_driver);
+}
+
+static void rx6110_spi_unregister(void)
+{
+	spi_unregister_driver(&rx6110_spi_driver);
+}
+#else
+static int rx6110_spi_register(void)
+{
+	return 0;
+}
+
+static void rx6110_spi_unregister(void)
+{
+}
+#endif /* CONFIG_SPI_MASTER */
+
+#ifdef CONFIG_I2C
+static struct regmap_config regmap_i2c_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = RX6110_REG_IRQ,
+	.read_flag_mask = 0x80,
+};
+
+static int rx6110_i2c_probe(struct i2c_client *client,
+			    const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct rx6110_data *rx6110;
+	int err;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA
+				| I2C_FUNC_SMBUS_I2C_BLOCK)) {
+		dev_err(&adapter->dev,
+			"doesn't support required functionality\n");
+		return -EIO;
+	}
+
+	rx6110 = devm_kzalloc(&client->dev, sizeof(*rx6110), GFP_KERNEL);
+	if (!rx6110)
+		return -ENOMEM;
+
+	rx6110->regmap = devm_regmap_init_i2c(client, &regmap_i2c_config);
+	if (IS_ERR(rx6110->regmap)) {
+		dev_err(&client->dev, "regmap init failed for rtc rx6110\n");
+		return PTR_ERR(rx6110->regmap);
+	}
+
+	i2c_set_clientdata(client, rx6110);
+
+	return rx6110_probe(rx6110, &client->dev);
+}
+
+static const struct i2c_device_id rx6110_i2c_id[] = {
+	{ "rx6110", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
+
+static struct i2c_driver rx6110_i2c_driver = {
+	.driver = {
+		.name = RX6110_DRIVER_NAME,
+	},
+	.probe		= rx6110_i2c_probe,
+	.id_table	= rx6110_i2c_id,
+};
+
+static int rx6110_i2c_register(void)
+{
+	return i2c_add_driver(&rx6110_i2c_driver);
+}
+
+static void rx6110_i2c_unregister(void)
+{
+	i2c_del_driver(&rx6110_i2c_driver);
+}
+#else
+static int rx6110_i2c_register(void)
+{
+	return 0;
+}
+
+static void rx6110_i2c_unregister(void)
+{
+}
+#endif /* CONFIG_I2C */
+
+static int __init rx6110_module_init(void)
+{
+	int ret;
+
+	ret = rx6110_spi_register();
+	if (ret)
+		return ret;
+
+	ret = rx6110_i2c_register();
+	if (ret)
+		rx6110_spi_unregister();
+
+	return ret;
+}
+module_init(rx6110_module_init);
+
+static void __exit rx6110_module_exit(void)
+{
+	rx6110_spi_unregister();
+	rx6110_i2c_unregister();
+}
+module_exit(rx6110_module_exit);
 
  MODULE_AUTHOR("Val Krutov <[email protected]>");
  MODULE_DESCRIPTION("RX-6110 SA RTC driver");

Signature · 6 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: [email protected]

           PGP key: 6FF2 E59F 00C6 BC28 31D8 64C1 1173 CB19 9808 B153
                             Keyserver: hkp://pool.sks-keyservers.net

Andy Shevchenko

2020-11-16 14:43 UTC #3 lore raw

Re: [PATCH v2 1/3] rtc: rx6110: add i2c support

In reply to Claudius Heine

Quoted 5 lines · Claudius Heine

The RX6110 also supports I2C, so this patch adds support for it to the driver.
This also renames the SPI specific functions and variables to include _spi_ in their names.

As practice shows this is not the best approach. Can you ratqer split it to three modules: core, spi, i2c like it's done in many other cases (esp. IIO)?

In Kconfig you just leave same option with two additional ones like

config ..._SPI
tristate
default SPI_MASTER
depends on SPI_MASTER

config ..._I2C
tristate
default I2C
depends on I2C

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Alexandre Belloni

maintainer

2020-11-16 17:36 UTC #4 lore raw

Re: [PATCH v2 1/3] rtc: rx6110: add i2c support

Quoted 10 lines · Andy Shevchenko

Quoted 5 lines · Claudius Heine
The RX6110 also supports I2C, so this patch adds support for it to the driver.
This also renames the SPI specific functions and variables to include _spi_ in their names.
As practice shows this is not the best approach. Can you ratqer split it to three modules: core, spi, i2c like it's done in many other cases (esp. IIO)?

Actually, I'm fine with having everytihn in the same file because separating everything out means having 3 more files per rtc supporting both busses in an already very crowded folder. And I don't think being able to remove support for one or the other holds any actual value.

Quoted 17 lines

In Kconfig you just leave same option with two additional ones like
config ..._SPI
tristate
default SPI_MASTER
depends on SPI_MASTER
config ..._I2C
tristate
default I2C
depends on I2C
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Signature · 3 lines

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

Andy Shevchenko

2020-11-16 17:57 UTC #5 lore raw

Re: [PATCH v2 1/3] rtc: rx6110: add i2c support

Quoted 8 lines · Alexandre Belloni

Quoted 7 lines · Andy Shevchenko
Quoted 5 lines · Claudius Heine
The RX6110 also supports I2C, so this patch adds support for it to the driver.
This also renames the SPI specific functions and variables to include _spi_ in their names.

Quoted 8 lines

Quoted 3 lines
As practice shows this is not the best approach. Can you ratqer split it to three modules: core, spi, i2c like it's done in many other cases (esp. IIO)?
Actually, I'm fine with having everytihn in the same file because separating everything out means having 3 more files per rtc supporting both busses in an already very crowded folder. And I don't think being able to remove support for one or the other holds any actual value.

Good to know your opinion!
Since you are one who is looking after RTC stuff I'm not insisting on my proposal.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Claudius Heine

author

2020-11-12 13:08 UTC #6 lore raw

PATCH v2 2/3 rtc: rx6110: add ACPI bindings to I2C

In reply to Claudius Heine

From: Johannes Hahn <[email protected]>

This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Signed-off-by Johannes Hahn <[email protected]>

Signed-off-by Claudius Heine <[email protected]>

Signed-off-by Henning Schild <[email protected]>

---
 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/rtc/rtc-rx6110.c +12 -0

index 1e6e9b19c81c..2fc9e8c2eeeb 100644
@@ -13,6 +13,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
+#include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/spi/spi.h>

@@ -449,6 +450,14 @@ static int rx6110_i2c_probe(struct i2c_client *client,
 	return rx6110_probe(rx6110, &client->dev);
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "RX6110SA", },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+#endif
+
 static const struct i2c_device_id rx6110_i2c_id[] = {
 	{ "rx6110", 0 },
 	{ }

@@ -458,6 +467,9 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
 static struct i2c_driver rx6110_i2c_driver = {
 	.driver = {
 		.name = RX6110_DRIVER_NAME,
+#ifdef CONFIG_ACPI
+		.acpi_match_table = ACPI_PTR(rx6110_i2c_acpi_match),
+#endif
 	},
 	.probe		= rx6110_i2c_probe,
 	.id_table	= rx6110_i2c_id,

Signature · 1 line

2.20.1

Andy Shevchenko

2020-11-16 14:46 UTC #7 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Quoted 4 lines · Claudius Heine

From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?

Before that happens, NAK.

P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Henning Schild

2020-11-16 15:31 UTC #8 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Am Mon, 16 Nov 2020 16:46:31 +0200
schrieb Andy Shevchenko <[email protected]>:

Quoted 6 lines · Claudius Heine

Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Quoted 8 lines

Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.

In fact we pushed it in coreboot and Linux at the same time.

https://review.coreboot.org/c/coreboot/+/47235

That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.

Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.

Henning

Andy Shevchenko

2020-11-16 16:04 UTC #9 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Quoted 18 lines · Henning Schild

Am Mon, 16 Nov 2020 16:46:31 +0200
schrieb Andy Shevchenko <[email protected]>:
Quoted 6 lines · Claudius Heine
Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.
Quoted 8 lines
Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.

Quoted 6 lines

In fact we pushed it in coreboot and Linux at the same time.
https://review.coreboot.org/c/coreboot/+/47235
That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.

Yes, first of all you must follow ACPI and PNP registry. You may use your Google vendor ID for that (IIRC you have two of them). Ideally you need to convince Seiko Epson to do the right thing.

Quoted 2 lines

Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.

Please, read ACPI specification. In particular chapters 6.1.2 "_CID (Compatible ID)", 6.1.5 "_HID (Hardware ID)". The latter clarifies the rules used to define an ID. Note, chapter 6.1.2 uses in particular "A valid HID value".

I hope you are using as latest as possible ACPICA compiler (or at least the one which follows the latest changes in it).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2020-11-16 16:07 UTC #10 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Quoted 21 lines · Andy Shevchenko

Quoted 20 lines · Henning Schild
Am Mon, 16 Nov 2020 16:46:31 +0200
schrieb Andy Shevchenko <[email protected]>:
Quoted 6 lines · Claudius Heine
Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.
Quoted 8 lines
Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.
In fact we pushed it in coreboot and Linux at the same time.

Quoted 8 lines

Quoted 4 lines
https://review.coreboot.org/c/coreboot/+/47235
That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.
Yes, first of all you must follow ACPI and PNP registry. You may use your Google vendor ID for that (IIRC you have two of them). Ideally you need to convince Seiko Epson to do the right thing.

JFYI: According to the registry [1] they have their own vendor ID

SEIKO EPSON CORPORATION SEC 11/29/1996

[1]: https://www.uefi.org/pnp_id_list

Quoted 10 lines

Quoted 2 lines
Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.
Please, read ACPI specification. In particular chapters 6.1.2 "_CID (Compatible ID)", 6.1.5 "_HID (Hardware ID)". The latter clarifies the rules used to define an ID. Note, chapter 6.1.2 uses in particular "A valid HID value".
I hope you are using as latest as possible ACPICA compiler (or at least the one which follows the latest changes in it).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

[email protected]

2020-11-17 07:37 UTC #11 lore raw

AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

In reply to Henning Schild

Hello Andy,

when comparing the ACPI IDs used in rtc-ds1307.c, which is already on mainline

https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1141

for example. Every ID listed there is also not formatted the ACPI ID , PNP ID way defined in the ACPI spec.

How about that ?

Best Regards,
Johannes

-----Ursprüngliche Nachricht-----
Von: Henning Schild <[email protected]>
Gesendet: Montag, 16. November 2020 16:30
An: Andy Shevchenko <[email protected]>
Cc: Claudius Heine <[email protected]>; Alessandro Zummo <[email protected]>; Alexandre Belloni <[email protected]>; [email protected]; [email protected]; Hahn, Johannes (DI FA CTR PLC PRC3) <[email protected]>; Zeh, Werner (DI MC MTS R&D HW 1) <[email protected]>
Betreff: Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Am Mon, 16 Nov 2020 16:46:31 +0200
schrieb Andy Shevchenko <[email protected]>:

Quoted 6 lines · Claudius Heine

Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Quoted 8 lines

Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.

In fact we pushed it in coreboot and Linux at the same time.

https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&reserved=0

That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.

Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.

Henning

Andy Shevchenko

2020-11-17 11:11 UTC #12 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Quoted 10 lines · On Tue, Nov 17, 2020 at 9:39 AM [email protected]…

Hello Andy,
when comparing the ACPI IDs used in rtc-ds1307.c, which is already on mainline
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1141
for example. Every ID listed there is also not formatted the ACPI ID , PNP ID way defined in the ACPI spec.
How about that ?

Bad examples should not prevent you from doing the right thing, correct?

JFYI: https://lore.kernel.org/linux-rtc/[email protected]/

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2020-11-17 11:17 UTC #13 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Quoted 10 lines · Andy Shevchenko

Quoted 7 lines · On Tue, Nov 17, 2020 at 9:39 AM [email protected]…
Hello Andy,
when comparing the ACPI IDs used in rtc-ds1307.c, which is already on mainline
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1141

Quoted 7 lines

Quoted 3 lines
for example. Every ID listed there is also not formatted the ACPI ID , PNP ID way defined in the ACPI spec.
How about that ?
Bad examples should not prevent you from doing the right thing, correct?
JFYI: https://lore.kernel.org/linux-rtc/[email protected]/

Moreover, this seems to be the last driver which was broken by the same guy who "invented" ACPI IDs for them. The rest had been reverted already.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

[email protected]

2020-11-17 09:47 UTC #14 lore raw

AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

In reply to Henning Schild

Hello Val,

my name is Johannes Hahn from Siemens AG in Germany.
Our product Open Controller II (OCII)[1] uses the Realtime Clock RX6110SA from SEIKO EPSON.

Currently there is a merge request ongoing for the Linux Kernel master branch[2] which adds I²C and ACPI support to your original driver implementation.

Simultaneously there is an already merged patch-set for coreboot[3] available creating the ACPI (SSDT) table entries for the RX6110SA.

The OCII uses coreboot for firmware initialization.

During the merge request the eligible objection arose that the ACPI ID used in the Linux driver patch is not conforming the ACPI Specification.
Indeed it does not. But when searching for a product identifier of RX6110SA I was not able to find a sufficient one with respect to the ACPI Specification (see [4] chapter 6.1.5 _HID (Hardware ID),[5]). According to the fact that there are other Linux RTC drivers on the Kernel mainline[6] which support ACPI matching that also do not have ACPI Specification compatible IDs we used that as an example for our first patch attempt.

A PNP ID for SEIKO EPSON is already registered at UEFI database[7].

What I kindly ask your for is an ACPI Specification conforming Product Identifier for the RX6110SA RTC ? According to [5] this Product Identifier should be "... always four-character hexadecimal numbers (0-9 and A-F)".

In case you do not know it our can not acquire/create one could you please redirect me to someone from SEIKO EPSON who can help me with that demand ?

[1]: (https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7677-2DB42-0GB0)
[2]: https://lkml.org/lkml/2020/11/12/561
[3]: https://review.coreboot.org/c/coreboot/+/47235
[4]: https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
[5]: https://www.uefi.org/PNP_ACPI_Registry
[6]: https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1142
[7]: https://www.uefi.org/PNP_ID_List?search=SEIKO+EPSON

Best Regards,
Johannes Hahn

Am Mon, 16 Nov 2020 16:46:31 +0200
schrieb Andy Shevchenko <[email protected]>:

Quoted 6 lines · Claudius Heine

Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Quoted 8 lines

Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.

In fact we pushed it in coreboot and Linux at the same time.

That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.

Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.

Henning

Andy Shevchenko

2020-11-17 11:34 UTC #15 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Quoted 5 lines · On Tue, Nov 17, 2020 at 11:51 AM [email protected]…

Hello Val,
my name is Johannes Hahn from Siemens AG in Germany.
Our product Open Controller II (OCII)[1] uses the Realtime Clock RX6110SA from SEIKO EPSON.

Nice to hear from you!

Quoted 3 lines

Currently there is a merge request ongoing for the Linux Kernel master branch[2] which adds I²C and ACPI support to your original driver implementation.
Simultaneously there is an already merged patch-set for coreboot[3] available creating the ACPI (SSDT) table entries for the RX6110SA.

Thanks for pointers, I commented there. The ACPI ID change must be reverted!

Quoted 4 lines

The OCII uses coreboot for firmware initialization.
During the merge request the eligible objection arose that the ACPI ID used in the Linux driver patch is not conforming the ACPI Specification.
Indeed it does not. But when searching for a product identifier of RX6110SA I was not able to find a sufficient one with respect to the ACPI Specification (see [4] chapter 6.1.5 _HID (Hardware ID),[5]).

Unfortunately many vendors, even being registered in the ACPI/PNP registry, are still neglecting the process.

Quoted 1 line

According to the fact that there are other Linux RTC drivers on the Kernel mainline[6] which support ACPI matching that also do not have ACPI Specification compatible IDs we used that as an example for our first patch attempt.

I answered this in previous mail.

Quoted 6 lines

A PNP ID for SEIKO EPSON is already registered at UEFI database[7].
What I kindly ask your for is an ACPI Specification conforming Product Identifier for the RX6110SA RTC ? According to [5] this Product Identifier should be "... always four-character hexadecimal numbers (0-9 and A-F)".
In case you do not know it our can not acquire/create one could you please redirect me to someone from SEIKO EPSON who can help me with that demand ?

So, to be on the constructive page (I thought initially you are from G company, but anyway) you may do the following:

- (for prototyping only) you may use the PRP0001 approach, described in [8]
- you may issue an ID under your (Siemens) vendor ID
- you may insist G company to issue the ID under their vendor space (thru coreboot)
- (the best option) to communicate to Seiko Epson to get official ID from them for this component (and ID mustn't abuse 6.1.5)

Unfortunately I have no contacts there, but I think the best effort is to contact their support and at the same time ask ASWG [9] how to proceed. I Cc'ed this to ACPI people in Linux kernel, maybe they can help.

Of course you have choice to push bad ID forward and use precedence (like many other companies, even Intel in past, do with firmwares and Linux kernel is full of badly formed IDs), but since the change is not existed in read devices I would really like to see proper process to be followed.

In the Linux kernel I'm in principle trying to prevent bad IDs from happening as much as I can.

Quoted 7 lines

[1]: (https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7677-2DB42-0GB0)
[2]: https://lkml.org/lkml/2020/11/12/561
[3]: https://review.coreboot.org/c/coreboot/+/47235
[4]: https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
[5]: https://www.uefi.org/PNP_ACPI_Registry
[6]: https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1142
[7]: https://www.uefi.org/PNP_ID_List?search=SEIKO+EPSON

[8]: https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst
[9]: https://www.uefi.org/workinggroups

Quoted 16 lines

Quoted 8 lines
Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.
In fact we pushed it in coreboot and Linux at the same time.
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&reserved=0
That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.
Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2020-11-17 11:41 UTC #16 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

+Cc: Simon

Simon, this is an issue with ACPI IDs and I think JFYI would be an interesting topic since this may happen in the future in U-Boot or other projects. Also, you may know people from coreboot to figure out what to do with this case and how to prevent something similar from happening in the future.

Quoted 91 lines · Andy Shevchenko

Quoted 5 lines · On Tue, Nov 17, 2020 at 11:51 AM [email protected]…
Hello Val,
my name is Johannes Hahn from Siemens AG in Germany.
Our product Open Controller II (OCII)[1] uses the Realtime Clock RX6110SA from SEIKO EPSON.
Nice to hear from you!
Quoted 3 lines
Currently there is a merge request ongoing for the Linux Kernel master branch[2] which adds I²C and ACPI support to your original driver implementation.
Simultaneously there is an already merged patch-set for coreboot[3] available creating the ACPI (SSDT) table entries for the RX6110SA.
Thanks for pointers, I commented there. The ACPI ID change must be reverted!
Quoted 4 lines
The OCII uses coreboot for firmware initialization.
During the merge request the eligible objection arose that the ACPI ID used in the Linux driver patch is not conforming the ACPI Specification.
Indeed it does not. But when searching for a product identifier of RX6110SA I was not able to find a sufficient one with respect to the ACPI Specification (see [4] chapter 6.1.5 _HID (Hardware ID),[5]).
Unfortunately many vendors, even being registered in the ACPI/PNP registry, are still neglecting the process.
Quoted 1 line
According to the fact that there are other Linux RTC drivers on the Kernel mainline[6] which support ACPI matching that also do not have ACPI Specification compatible IDs we used that as an example for our first patch attempt.
I answered this in previous mail.
Quoted 6 lines
A PNP ID for SEIKO EPSON is already registered at UEFI database[7].
What I kindly ask your for is an ACPI Specification conforming Product Identifier for the RX6110SA RTC ? According to [5] this Product Identifier should be "... always four-character hexadecimal numbers (0-9 and A-F)".
In case you do not know it our can not acquire/create one could you please redirect me to someone from SEIKO EPSON who can help me with that demand ?
So, to be on the constructive page (I thought initially you are from G company, but anyway) you may do the following:
- (for prototyping only) you may use the PRP0001 approach, described in [8]
- you may issue an ID under your (Siemens) vendor ID
- you may insist G company to issue the ID under their vendor space (thru coreboot)
- (the best option) to communicate to Seiko Epson to get official ID from them for this component (and ID mustn't abuse 6.1.5)
Unfortunately I have no contacts there, but I think the best effort is to contact their support and at the same time ask ASWG [9] how to proceed. I Cc'ed this to ACPI people in Linux kernel, maybe they can help.
Of course you have choice to push bad ID forward and use precedence (like many other companies, even Intel in past, do with firmwares and Linux kernel is full of badly formed IDs), but since the change is not existed in read devices I would really like to see proper process to be followed.
In the Linux kernel I'm in principle trying to prevent bad IDs from happening as much as I can.
Quoted 7 lines
[1]: (https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7677-2DB42-0GB0)
[2]: https://lkml.org/lkml/2020/11/12/561
[3]: https://review.coreboot.org/c/coreboot/+/47235
[4]: https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
[5]: https://www.uefi.org/PNP_ACPI_Registry
[6]: https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1142
[7]: https://www.uefi.org/PNP_ID_List?search=SEIKO+EPSON
[8]: https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst
[9]: https://www.uefi.org/workinggroups
Quoted 9 lines
Quoted 8 lines
Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.
Quoted 7 lines
In fact we pushed it in coreboot and Linux at the same time.
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&reserved=0
That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.
Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.
--
With Best Regards,
Andy Shevchenko

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Henning Schild

2020-11-18 10:05 UTC #17 lore raw

Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

We are trying to reach out to Epson to find valid IDs, but will do that off-list for now.

In the meantime we will propose just the I2C patches, without ACPI.

We have Werner Zeh here, who pushed the coreboot change. In coreboot nobody objected, maybe they have not been aware of the processes or it was missed in their review.

Werner i think the coreboot change should be reverted so it does not enter a release and will become legacy that needs to be supported. Let us revisit it once we have the proper IDs. If that fails we have to look into the other options that Andy proposed. There is no point having the coreboot support without the Linux-user ...

regards,
Henning

Am Tue, 17 Nov 2020 13:41:08 +0200
schrieb Andy Shevchenko <[email protected]>:

Quoted 127 lines

+Cc: Simon
Simon, this is an issue with ACPI IDs and I think JFYI would be an interesting topic since this may happen in the future in U-Boot or other projects. Also, you may know people from coreboot to figure out what to do with this case and how to prevent something similar from happening in the future.
Quoted 114 lines · Andy Shevchenko
Quoted 6 lines · On Tue, Nov 17, 2020 at 11:51 AM [email protected]…
Hello Val,
my name is Johannes Hahn from Siemens AG in Germany. Our product Open Controller II (OCII)[1] uses the Realtime Clock RX6110SA from SEIKO EPSON.
Nice to hear from you!
Quoted 7 lines
Currently there is a merge request ongoing for the Linux Kernel master branch[2] which adds I²C and ACPI support to your original driver implementation.
Simultaneously there is an already merged patch-set for coreboot[3] available creating the ACPI (SSDT) table entries for the RX6110SA.
Thanks for pointers, I commented there. The ACPI ID change must be reverted!
Quoted 8 lines
The OCII uses coreboot for firmware initialization.
During the merge request the eligible objection arose that the ACPI ID used in the Linux driver patch is not conforming the ACPI Specification. Indeed it does not. But when searching for a product identifier of RX6110SA I was not able to find a sufficient one with respect to the ACPI Specification (see [4] chapter 6.1.5 _HID (Hardware ID),[5]).
Unfortunately many vendors, even being registered in the ACPI/PNP registry, are still neglecting the process.
Quoted 4 lines
According to the fact that there are other Linux RTC drivers on the Kernel mainline[6] which support ACPI matching that also do not have ACPI Specification compatible IDs we used that as an example for our first patch attempt.
I answered this in previous mail.
Quoted 11 lines
A PNP ID for SEIKO EPSON is already registered at UEFI database[7].
What I kindly ask your for is an ACPI Specification conforming Product Identifier for the RX6110SA RTC ? According to [5] this Product Identifier should be "... always four-character hexadecimal numbers (0-9 and A-F)".
In case you do not know it our can not acquire/create one could you please redirect me to someone from SEIKO EPSON who can help me with that demand ?
So, to be on the constructive page (I thought initially you are from G company, but anyway) you may do the following:
- (for prototyping only) you may use the PRP0001 approach, described in [8]
- you may issue an ID under your (Siemens) vendor ID
- you may insist G company to issue the ID under their vendor space (thru coreboot)
- (the best option) to communicate to Seiko Epson to get official ID from them for this component (and ID mustn't abuse 6.1.5)
Unfortunately I have no contacts there, but I think the best effort is to contact their support and at the same time ask ASWG [9] how to proceed. I Cc'ed this to ACPI people in Linux kernel, maybe they can help.
Of course you have choice to push bad ID forward and use precedence (like many other companies, even Intel in past, do with firmwares and Linux kernel is full of badly formed IDs), but since the change is not existed in read devices I would really like to see proper process to be followed.
In the Linux kernel I'm in principle trying to prevent bad IDs from happening as much as I can.
Quoted 8 lines
[1]:
(https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7677-2DB42-0GB0)
[2]: https://lkml.org/lkml/2020/11/12/561 [3]:
https://review.coreboot.org/c/coreboot/+/47235 [4]:
https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
[5]: https://www.uefi.org/PNP_ACPI_Registry [6]:
https://elixir.bootlin.com/linux/latest/source/drivers/rtc/rtc-ds1307.c#L1142
[7]: https://www.uefi.org/PNP_ID_List?search=SEIKO+EPSON
[8]:
https://elixir.bootlin.com/linux/latest/source/Documentation/firmware-guide/acpi/enumeration.rst
[9]: https://www.uefi.org/workinggroups
Quoted 12 lines
Quoted 9 lines
Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.
In fact we pushed it in coreboot and Linux at the same time.
Quoted 8 lines
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&reserved=0
That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.
Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.
--
With Best Regards,
Andy Shevchenko

[email protected]

2020-11-18 10:59 UTC #18 lore raw

AW: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

Hi Henning.

We had a short discussion with Andy on coreboot Gerrit (https://review.coreboot.org/c/coreboot/+/47235) regarding this issue.
We have agreed that we will give Epson a period of two weeks for reaction. If we will not have a valid HID by then, I will push a patch which will use a non-valid HID instead (something like XXXX0000 as proposed by Andy). I will clarify in the meantime when the next coreboot release will happen and prevent this wrong ID from getting part of the release.

Werner

-----Ursprüngliche Nachricht-----
Von: Henning Schild <[email protected]>
Gesendet: Mittwoch, 18. November 2020 11:04
An: Andy Shevchenko <[email protected]>
Cc: Hahn, Johannes (DI FA CTR PLC PRC3) <[email protected]>; Rafael J. Wysocki <[email protected]>; ACPI Devel Maling List <[email protected]>; Brown, Len <[email protected]>; Simon Glass <[email protected]>; Simon Glass <[email protected]>; [email protected]; Claudius Heine <[email protected]>; Alessandro Zummo <[email protected]>; Alexandre Belloni <[email protected]>; [email protected]; [email protected]; Zeh, Werner (DI MC MTS R&D HW 1) <[email protected]>; Andy Shevchenko <[email protected]>; Mantel, Martin (DI FA CTR PLC PO) <[email protected]>
Betreff: Re: [PATCH v2 2/3] rtc: rx6110: add ACPI bindings to I2C

We are trying to reach out to Epson to find valid IDs, but will do that off-list for now.

In the meantime we will propose just the I2C patches, without ACPI.

We have Werner Zeh here, who pushed the coreboot change. In coreboot nobody objected, maybe they have not been aware of the processes or it was missed in their review.

regards,
Henning

Am Tue, 17 Nov 2020 13:41:08 +0200
schrieb Andy Shevchenko <[email protected]>:

Quoted 167 lines

+Cc: Simon
Simon, this is an issue with ACPI IDs and I think JFYI would be an interesting topic since this may happen in the future in U-Boot or other projects. Also, you may know people from coreboot to figure out what to do with this case and how to prevent something similar from happening in the future.
Quoted 154 lines · Andy Shevchenko
Quoted 6 lines · On Tue, Nov 17, 2020 at 11:51 AM [email protected]…
Hello Val,
my name is Johannes Hahn from Siemens AG in Germany. Our product Open Controller II (OCII)[1] uses the Realtime Clock RX6110SA from SEIKO EPSON.
Nice to hear from you!
Quoted 7 lines
Currently there is a merge request ongoing for the Linux Kernel master branch[2] which adds I²C and ACPI support to your original driver implementation.
Simultaneously there is an already merged patch-set for coreboot[3] available creating the ACPI (SSDT) table entries for the RX6110SA.
Thanks for pointers, I commented there. The ACPI ID change must be reverted!
Quoted 8 lines
The OCII uses coreboot for firmware initialization.
During the merge request the eligible objection arose that the ACPI ID used in the Linux driver patch is not conforming the ACPI Specification. Indeed it does not. But when searching for a product identifier of RX6110SA I was not able to find a sufficient one with respect to the ACPI Specification (see [4] chapter 6.1.5 _HID (Hardware ID),[5]).
Unfortunately many vendors, even being registered in the ACPI/PNP registry, are still neglecting the process.
Quoted 4 lines
According to the fact that there are other Linux RTC drivers on the Kernel mainline[6] which support ACPI matching that also do not have ACPI Specification compatible IDs we used that as an example for our first patch attempt.
I answered this in previous mail.
Quoted 11 lines
A PNP ID for SEIKO EPSON is already registered at UEFI database[7].
What I kindly ask your for is an ACPI Specification conforming Product Identifier for the RX6110SA RTC ? According to [5] this Product Identifier should be "... always four-character hexadecimal numbers (0-9 and A-F)".
In case you do not know it our can not acquire/create one could you please redirect me to someone from SEIKO EPSON who can help me with that demand ?
So, to be on the constructive page (I thought initially you are from G company, but anyway) you may do the following:
- (for prototyping only) you may use the PRP0001 approach, described in [8]
- you may issue an ID under your (Siemens) vendor ID
- you may insist G company to issue the ID under their vendor space (thru coreboot)
- (the best option) to communicate to Seiko Epson to get official ID from them for this component (and ID mustn't abuse 6.1.5)
Unfortunately I have no contacts there, but I think the best effort is to contact their support and at the same time ask ASWG [9] how to proceed. I Cc'ed this to ACPI people in Linux kernel, maybe they can help.
Of course you have choice to push bad ID forward and use precedence (like many other companies, even Intel in past, do with firmwares and Linux kernel is full of badly formed IDs), but since the change is not existed in read devices I would really like to see proper process to be followed.
In the Linux kernel I'm in principle trying to prevent bad IDs from happening as much as I can.
Quoted 20 lines
[1]:
(https://mall.industry.siemens.com/mall/en/WW/Catalog/Product/6ES7
677-2DB42-0GB0)
[2]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F11%2F12%2F561&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=mc2cCsxmX6cjx6HP1yEY7BYd2UmDayTh%2FAqvfA5%2BG20%3D&reserved=0 [3]:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IPt%2B2j%2Fb7TCIwnQpTh0YQfRhAk3gG34xgs9j5J1p0IA%3D&reserved=0 [4]:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
uefi.org%2Fsites%2Fdefault%2Ffiles%2Fresources%2FACPI_6_3_final_Ja
n30.pdf&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e
4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C
637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=9DXsd
gRozFKHm%2F3B1i1ls2cwtzip1rsFuJ9qnwFvXOY%3D&reserved=0
[5]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.uefi.org%2FPNP_ACPI_Registry&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tFOTF40yzDdfVWGKatL56IYo87aCAABHoxPefmLAtPU%3D&reserved=0 [6]:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
elixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Frtc%2Frtc
-ds1307.c%23L1142&data=04%7C01%7Cwerner.zeh%40siemens.com%7C57
e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%
7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&s
data=fn6HCTcy1YYOH02dd3uiXymz87jXzTzLO3rWX6S1AeI%3D&reserved=0
Quoted 8 lines
[7]:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
www.uefi.org%2FPNP_ID_List%3Fsearch%3DSEIKO%2BEPSON&data=04%7C 01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7 C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CU nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I k1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NDHUqVKgnhNASqTOEydkG%2BocR eOauHMeo1uCrGCvOyk%3D&reserved=0
[8]:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fel
ixir.bootlin.com%2Flinux%2Flatest%2Fsource%2FDocumentation%2Ffirmwar e-guide%2Facpi%2Fenumeration.rst&data=04%7C01%7Cwerner.zeh%40sie mens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab4 2e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJW IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100 0&sdata=y7mB4fH7d98r4VOSeTM2qQ%2BraUvwRrmsznK7qj7W6bM%3D&res erved=0
[9]:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
w.uefi.org%2Fworkinggroups&data=04%7C01%7Cwerner.zeh%40siemens.c om%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495 d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000& sdata=cCwPr1JYgrP7Rou%2FdR0y5DOL%2FUg9MRgnu7QBoOGURZQ%3D&reserve d=0
Quoted 20 lines
Quoted 9 lines
Before adding new ACPI ID, can you provide an evidence (either from vendor of the component, or a real snapshot of DSDT from device on market) that this is real ID?
Before that happens, NAK.
P.S. Seems to me that this is kinda cargo cult patch because proposed ID is against ACPI and PNP registry and ACPI specification.
In fact we pushed it in coreboot and Linux at the same time.
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
review.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&data=04%7C01% 7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38 ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnkn own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h aWwiLCJXVCI6Mn0%3D%7C1000&sdata=IPt%2B2j%2Fb7TCIwnQpTh0YQfRhAk 3gG34xgs9j5J1p0IA%3D&reserved=0
Quoted 6 lines
That is the evidence. But in case this is wrong we can probably still change coreboot, even though the patches have been merged there already.
Maybe you can go into detail where you see the violations and maybe even suggest fixes that come to mind.
--
With Best Regards,
Andy Shevchenko

Claudius Heine

author

2021-03-16 10:09 UTC #19 lore raw

v3 cover add ACPI binding to RX6110 driver

Hi,

it took some time, but now we got the official ACPI id for the RX6110 RTC driver from Seiko Epson.

regards,
Claudius

Johannes Hahn (1):
rtc: rx6110: add ACPI bindings to I2C

 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Signature · 1 line

2.30.1

Claudius Heine

author

2021-03-16 14:49 UTC #20 lore raw

PATCH v4 1/1 rtc: rx6110: add ACPI bindings to I2C

From: Johannes Hahn <[email protected]>

This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Signed-off-by Johannes Hahn <[email protected]>

Co-developed-by Claudius Heine <[email protected]>

Signed-off-by Claudius Heine <[email protected]>

---
 drivers/rtc/rtc-rx6110.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/rtc/rtc-rx6110.c +7 -0

index 79161d4c6ce4..2a06953c0a84 100644
@@ -447,6 +447,12 @@ static int rx6110_i2c_probe(struct i2c_client *client,
 	return rx6110_probe(rx6110, &client->dev);
 }
 
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+
 static const struct i2c_device_id rx6110_i2c_id[] = {
 	{ "rx6110", 0 },
 	{ }

@@ -456,6 +462,7 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
 static struct i2c_driver rx6110_i2c_driver = {
 	.driver = {
 		.name = RX6110_DRIVER_NAME,
+		.acpi_match_table = ACPI_PTR(rx6110_i2c_acpi_match),
 	},
 	.probe		= rx6110_i2c_probe,
 	.id_table	= rx6110_i2c_id,

Signature · 1 line

2.30.1

Andy Shevchenko

Reviewed-by on v4 1/1

2021-03-16 16:56 UTC #21 lore raw

Re: [PATCH v4] rtc: rx6110: add ACPI bindings to I2C

Quoted 4 lines · Claudius Heine

From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

You missed given tag, when somebody sends you one, it's usually your responsibility to pick it up. Hint: install b4 tool (likely in your distro, at least Debian, Arch Linux have it) and run it against message ID of the version in question. It will gather all tags. For example, for this case, run

% b4 am [email protected]

It will download mailbox suitable for git am ... you will read on the screen.

Also, when send a new version, don't attach it to the old thread. It will confuse people and maybe even tools (i.o.w. don't supply message ID to be put to In-Reply-To header).

So, repeat again my tag and see one fix to be performed below.

Reviewed-by From: Andy Shevchenko <[email protected]>

Quoted 25 lines

Signed-off-by Johannes Hahn <[email protected]>

Co-developed-by Claudius Heine <[email protected]>

Signed-off-by Claudius Heine <[email protected]>

---
 drivers/rtc/rtc-rx6110.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/rtc/rtc-rx6110.c +6 -0

index 79161d4c6ce4..2a06953c0a84 100644
@@ -447,6 +447,12 @@ static int rx6110_i2c_probe(struct i2c_client *client,
 	return rx6110_probe(rx6110, &client->dev);
 }
 
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+
 static const struct i2c_device_id rx6110_i2c_id[] = {
 	{ "rx6110", 0 },
 	{ }

drivers/rtc/rtc-rx6110.c MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id); · line 465

Quoted 4 lines

drivers/rtc/rtc-rx6110.c +1 -0

@@ -456,6 +462,7 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
 static struct i2c_driver rx6110_i2c_driver = {
 	.driver = {
 		.name = RX6110_DRIVER_NAME,
+		.acpi_match_table = ACPI_PTR(rx6110_i2c_acpi_match),

Since you drop ifdeffery above, you have to drop ACPI_PTR() (besides that ACPI_PTR() requires acpi.h to be included).

Quoted 6 lines

drivers/rtc/rtc-rx6110.c +0 -0
 	},
 	.probe		= rx6110_i2c_probe,
 	.id_table	= rx6110_i2c_id,
Signature · 1 line
2.30.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Claudius Heine

author

2021-03-17 07:54 UTC #22 lore raw

Re: [PATCH v4] rtc: rx6110: add ACPI bindings to I2C

Hi Andy,

Quoted 11 lines · Andy Shevchenko

Quoted 4 lines · Claudius Heine
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.
You missed given tag, when somebody sends you one, it's usually your responsibility to pick it up. Hint: install b4 tool (likely in your distro, at least Debian, Arch Linux have it) and run it against message ID of the version in question. It will gather all tags. For example, for this case, run

Quoted 7 lines

   % b4 am [email protected]
It will download mailbox suitable for git am ... you will read on the screen.
Also, when send a new version, don't attach it to the old thread. It will confuse people and maybe even tools (i.o.w. don't supply message ID to be put to In-Reply-To header).

Ok.

Quoted 3 lines

So, repeat again my tag and see one fix to be performed below.
Reviewed-by From: Andy Shevchenko <[email protected]>

Ok, will take care about this in v5.

Quoted 25 lines

Signed-off-by Johannes Hahn <[email protected]>

Co-developed-by Claudius Heine <[email protected]>

Signed-off-by Claudius Heine <[email protected]>

---
  drivers/rtc/rtc-rx6110.c | 7 +++++++
  1 file changed, 7 insertions(+)

drivers/rtc/rtc-rx6110.c +6 -0

index 79161d4c6ce4..2a06953c0a84 100644
@@ -447,6 +447,12 @@ static int rx6110_i2c_probe(struct i2c_client *client,
  	return rx6110_probe(rx6110, &client->dev);
  }
 
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+
  static const struct i2c_device_id rx6110_i2c_id[] = {
  	{ "rx6110", 0 },
  	{ }

Quoted 8 lines

drivers/rtc/rtc-rx6110.c MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id); · line 465
Quoted 5 lines
drivers/rtc/rtc-rx6110.c +1 -0
@@ -456,6 +462,7 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
  static struct i2c_driver rx6110_i2c_driver = {
  	.driver = {
  		.name = RX6110_DRIVER_NAME,
+		.acpi_match_table = ACPI_PTR(rx6110_i2c_acpi_match),
Since you drop ifdeffery above, you have to drop ACPI_PTR() (besides that ACPI_PTR() requires acpi.h to be included).

Ok, will do that as well.

regards,
Claudius

kernel test robot

build bot

2021-03-16 19:04 UTC #23 lore raw

Re: [PATCH v4] rtc: rx6110: add ACPI bindings to I2C

In reply to Claudius Heine

Thank you for the patch! Perhaps something to improve:

Hi Claudius,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v5.12-rc3 next-20210316]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Claudius-Heine/rtc-rx6110-add-ACPI-bindings-to-I2C/20210316-225026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: powerpc-randconfig-r006-20210316 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 50c7504a93fdb90c26870db8c8ea7add895c7725)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/ec344b93b1b5f4c2c77ce68b7bde7ec380e356a8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Claudius-Heine/rtc-rx6110-add-ACPI-bindings-to-I2C/20210316-225026 git checkout ec344b93b1b5f4c2c77ce68b7bde7ec380e356a8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/rtc/rtc-rx6110.c:450:36: warning: unused variable 'rx6110_i2c_acpi_match' [-Wunused-const-variable]
   static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
                                      ^

1 warning generated.

vim +/rx6110_i2c_acpi_match +450 drivers/rtc/rtc-rx6110.c

449

 > 450	static const struct acpi_device_id rx6110_i2c_acpi_match[] = {

451 { "SECC6110" },
452 { }
453 };
454 MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
455

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Andy Shevchenko

2021-03-16 20:44 UTC #24 lore raw

Re: [PATCH v4] rtc: rx6110: add ACPI bindings to I2C

Quoted 27 lines · kernel test robot

Hi Claudius,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on abelloni/rtc-next]
[also build test WARNING on v5.12-rc3 next-20210316]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:    https://github.com/0day-ci/linux/commits/Claudius-Heine/rtc-rx6110-add-ACPI-bindings-to-I2C/20210316-225026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: powerpc-randconfig-r006-20210316 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 50c7504a93fdb90c26870db8c8ea7add895c7725)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/ec344b93b1b5f4c2c77ce68b7bde7ec380e356a8
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Claudius-Heine/rtc-rx6110-add-ACPI-bindings-to-I2C/20210316-225026 git checkout ec344b93b1b5f4c2c77ce68b7bde7ec380e356a8
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate

Quoted 8 lines

Reported-by kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
Quoted 1 line
Quoted 1 line
drivers/rtc/rtc-rx6110.c:450:36: warning: unused variable 'rx6110_i2c_acpi_match' [-Wunused-const-variable]
   static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
                                      ^
1 warning generated.

Precisely!

This happens due to ACPI_PTR() presence. Either ACPI_PTR() _and_ ifdeffery or none of them should be in the code.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Claudius Heine

author

2021-03-16 10:09 UTC #25 lore raw

PATCH v3 1/1 rtc: rx6110: add ACPI bindings to I2C

In reply to [email protected]

From: Johannes Hahn <[email protected]>

This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Signed-off-by Johannes Hahn <[email protected]>

Signed-off-by Claudius Heine <[email protected]>

Signed-off-by Henning Schild <[email protected]>

---
 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/rtc/rtc-rx6110.c +12 -0

index 79161d4c6ce4..29bd697f82cb 100644
@@ -13,6 +13,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
+#include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/spi/spi.h>

@@ -447,6 +448,14 @@ static int rx6110_i2c_probe(struct i2c_client *client,
 	return rx6110_probe(rx6110, &client->dev);
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110", },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+#endif
+
 static const struct i2c_device_id rx6110_i2c_id[] = {
 	{ "rx6110", 0 },
 	{ }

@@ -456,6 +465,9 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
 static struct i2c_driver rx6110_i2c_driver = {
 	.driver = {
 		.name = RX6110_DRIVER_NAME,
+#ifdef CONFIG_ACPI
+		.acpi_match_table = ACPI_PTR(rx6110_i2c_acpi_match),
+#endif
 	},
 	.probe		= rx6110_i2c_probe,
 	.id_table	= rx6110_i2c_id,

Signature · 1 line

2.30.1

Andy Shevchenko

Reviewed-by on v3 1/1

2021-03-16 11:31 UTC #26 lore raw

Re: [PATCH v3 1/1] rtc: rx6110: add ACPI bindings to I2C

Quoted 4 lines · Claudius Heine

From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.

Thanks for all effort!

Reviewed-by Andy Shevchenko <[email protected]>

after addressing the below comments.

Quoted 4 lines

Signed-off-by Johannes Hahn <[email protected]>
Signed-off-by Claudius Heine <[email protected]>
Signed-off-by Henning Schild <[email protected]>

I think this is somehow confusing. Either you need to add Co-developed-by of the corresponding people, or remove SoB (because of From line), i.o.w seems like Co-dB tag is needed for Johannes or you and something should be done with Henning's SoB.

Quoted 9 lines

---
 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/rtc/rtc-rx6110.c +0 -0

index 79161d4c6ce4..29bd697f82cb 100644

drivers/rtc/rtc-rx6110.c line 17

Quoted 5 lines

drivers/rtc/rtc-rx6110.c +1 -0

@@ -13,6 +13,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
 
+#include <linux/acpi.h>

Usually it's not needed if you leave IDs always to be compiled. Instead mod_devicetable.h is used. But it's all up to you and maintainer.

Quoted 19 lines

drivers/rtc/rtc-rx6110.c +0 -0

 #include <linux/of.h>
 #include <linux/of_device.h>

#include <linux/spi/spi.h>

drivers/rtc/rtc-rx6110.c +8 -0

@@ -447,6 +448,14 @@ static int rx6110_i2c_probe(struct i2c_client *client,
 	return rx6110_probe(rx6110, &client->dev);
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110", },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+#endif
+
 static const struct i2c_device_id rx6110_i2c_id[] = {
 	{ "rx6110", 0 },
 	{ }

drivers/rtc/rtc-rx6110.c MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id); · line 468

Quoted 4 lines

drivers/rtc/rtc-rx6110.c +1 -0

@@ -456,6 +465,9 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
 static struct i2c_driver rx6110_i2c_driver = {
 	.driver = {
 		.name = RX6110_DRIVER_NAME,
+#ifdef CONFIG_ACPI

This is implied by the stub ACPI_PTR() macro for ACPI=n case. I.o.w drop this ugly and redundant ifdeffery.

Quoted 8 lines

drivers/rtc/rtc-rx6110.c +2 -0

+		.acpi_match_table = ACPI_PTR(rx6110_i2c_acpi_match),
+#endif
 	},
 	.probe		= rx6110_i2c_probe,
 	.id_table	= rx6110_i2c_id,

Signature · 1 line

2.30.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2021-03-16 11:33 UTC #27 lore raw

Re: [PATCH v3 1/1] rtc: rx6110: add ACPI bindings to I2C

Quoted 13 lines · Andy Shevchenko

Quoted 4 lines · Claudius Heine
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.
Thanks for all effort!
Reviewed-by Andy Shevchenko <[email protected]>
after addressing the below comments.
Quoted 2 lines
Signed-off-by Johannes Hahn <[email protected]>
Signed-off-by Claudius Heine <[email protected]>

Quoted 6 lines

Quoted 1 line
Signed-off-by Henning Schild <[email protected]>
I think this is somehow confusing. Either you need to add Co-developed-by of the corresponding people, or remove SoB (because of From line), i.o.w seems like Co-dB tag is needed for Johannes or you and something should be done with Henning's SoB.

Since Johannes' name is in the From, then it seems okay for you (either you a co-developer, than a Co-dB tag, or a submitter, than it's okay to have no Co-dB tag).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Henning Schild

2021-03-16 12:03 UTC #28 lore raw

Re: [PATCH v3 1/1] rtc: rx6110: add ACPI bindings to I2C

In reply to Andy Shevchenko

Am Tue, 16 Mar 2021 13:30:36 +0200
schrieb Andy Shevchenko <[email protected]>:

Quoted 14 lines · Claudius Heine

Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.
Thanks for all effort!
Reviewed-by Andy Shevchenko <[email protected]>
after addressing the below comments.
Quoted 4 lines
Signed-off-by Johannes Hahn <[email protected]>
Signed-off-by Claudius Heine <[email protected]>
Signed-off-by Henning Schild <[email protected]>

Claudius, just remove that. I guess just add yours and mention authors in the code if they should receive some recognition.

Henning

Quoted 60 lines

I think this is somehow confusing. Either you need to add Co-developed-by of the corresponding people, or remove SoB (because of From line), i.o.w seems like Co-dB tag is needed for Johannes or you and something should be done with Henning's SoB.
Quoted 9 lines
---
 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/rtc/rtc-rx6110.c +0 -0
index 79161d4c6ce4..29bd697f82cb 100644
drivers/rtc/rtc-rx6110.c line 17
Quoted 5 lines
drivers/rtc/rtc-rx6110.c +1 -0
@@ -13,6 +13,7 @@
 #include <linux/of_gpio.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>  
 
+#include <linux/acpi.h>  
Usually it's not needed if you leave IDs always to be compiled. Instead mod_devicetable.h is used. But it's all up to you and maintainer.
Quoted 19 lines
drivers/rtc/rtc-rx6110.c +0 -0
 #include <linux/of.h>
 #include <linux/of_device.h>
#include <linux/spi/spi.h>
drivers/rtc/rtc-rx6110.c +8 -0
@@ -447,6 +448,14 @@ static int rx6110_i2c_probe(struct i2c_client
 *client, return rx6110_probe(rx6110, &client->dev);
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110", },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+#endif
+
 static const struct i2c_device_id rx6110_i2c_id[] = {
 	{ "rx6110", 0 },
 	{ }
drivers/rtc/rtc-rx6110.c MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id); · line 468
Quoted 4 lines
drivers/rtc/rtc-rx6110.c +1 -0
@@ -456,6 +465,9 @@ MODULE_DEVICE_TABLE(i2c, rx6110_i2c_id);
 static struct i2c_driver rx6110_i2c_driver = {
 	.driver = {
 		.name = RX6110_DRIVER_NAME,
+#ifdef CONFIG_ACPI  
This is implied by the stub ACPI_PTR() macro for ACPI=n case. I.o.w drop this ugly and redundant ifdeffery.
Quoted 8 lines
drivers/rtc/rtc-rx6110.c +1 -0
+		.acpi_match_table =
ACPI_PTR(rx6110_i2c_acpi_match), +#endif
drivers/rtc/rtc-rx6110.c +0 -0
 	},
 	.probe		= rx6110_i2c_probe,
 	.id_table	= rx6110_i2c_id,
Signature · 1 line
2.30.1

Andy Shevchenko

2021-03-16 13:47 UTC #29 lore raw

Re: [PATCH v3 1/1] rtc: rx6110: add ACPI bindings to I2C

Quoted 18 lines · Henning Schild

Am Tue, 16 Mar 2021 13:30:36 +0200
schrieb Andy Shevchenko <[email protected]>:
Quoted 14 lines · Claudius Heine
Quoted 4 lines
From: Johannes Hahn <[email protected]>
This allows the RX6110 driver to be automatically assigned to the right device on the I2C bus.
Thanks for all effort!
Reviewed-by Andy Shevchenko <[email protected]>
after addressing the below comments.
Quoted 4 lines
Signed-off-by Johannes Hahn <[email protected]>
Signed-off-by Claudius Heine <[email protected]>
Signed-off-by Henning Schild <[email protected]>

Quoted 8 lines

Claudius, just remove that. I guess just add yours and mention authors in the code if they should receive some recognition.
Quoted 5 lines
drivers/rtc/rtc-rx6110.c static int rx6110_i2c_probe(struct i2c_client *client, · line 454
Quoted 5 lines
drivers/rtc/rtc-rx6110.c +4 -0
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rx6110_i2c_acpi_match[] = {
+	{ "SECC6110", },
 
+	{ },

Missed one thing, remove comma here. Terminator lines do not need a comma (and it's theoretically may confuse people or scripts while adding a new record to the array).

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
drivers/rtc/rtc-rx6110.c +3 -0
+};
+MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
+#endif

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Claudius Heine

author

2020-11-12 13:08 UTC #30 lore raw

PATCH v2 3/3 rtc: Kconfig: Fix typo in help message of rx 6110

In reply to Claudius Heine

The help message in the Kconfig for the RX-6110 erronously stated RX-6610.

Signed-off-by Claudius Heine <[email protected]>

---
 drivers/rtc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/rtc/Kconfig +1 -1

index 1fe411ffb19c..c7fb749c1c2f 100644
@@ -933,7 +933,7 @@ config RTC_DRV_RX6110
 	select REGMAP_SPI if SPI_MASTER
 	select REGMAP_I2C if I2C
 	help
-	  If you say yes here you will get support for the Epson RX-6610.
+	  If you say yes here you will get support for the Epson RX-6110.
 
 	  This driver can also be built as a module. If so the module
 	  will be called rtc-rx6110.

Signature · 1 line

2.20.1