[PATCH v2 0/3] Adding I2C support to RX6110 RTC

[PATCH v2 0/3] Adding I2C support to RX6110 RTC

[Claudius Heine]

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

-- 
2.20.1

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

[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 <ch@denx.de>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/rtc/Kconfig      |  20 ++---
 drivers/rtc/rtc-rx6110.c | 165 +++++++++++++++++++++++++++++++++------
 2 files changed, 154 insertions(+), 31 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 65ad9d0b47ab..1fe411ffb19c 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -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
diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
index 3a9eb7043f01..1e6e9b19c81c 100644
--- a/drivers/rtc/rtc-rx6110.c
+++ b/drivers/rtc/rtc-rx6110.c
@@ -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 <val.krutov@erd.epson.com>");
 MODULE_DESCRIPTION("RX-6110 SA RTC driver");
-- 
2.20.1

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

[Claudius Heine]

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

On 2020-11-12 14:07, Claudius Heine wrote:
> 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 <ch@denx.de>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>   drivers/rtc/Kconfig      |  20 ++---
>   drivers/rtc/rtc-rx6110.c | 165 +++++++++++++++++++++++++++++++++------
>   2 files changed, 154 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 65ad9d0b47ab..1fe411ffb19c 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -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
> diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
> index 3a9eb7043f01..1e6e9b19c81c 100644
> --- a/drivers/rtc/rtc-rx6110.c
> +++ b/drivers/rtc/rtc-rx6110.c
> @@ -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 <val.krutov@erd.epson.com>");
>   MODULE_DESCRIPTION("RX-6110 SA RTC driver");
> 

-- 
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: ch@denx.de

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

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

[Andy Shevchenko]

On Thu, Nov 12, 2020 at 02:07:32PM +0100, Claudius Heine wrote:
> 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

-- 
With Best Regards,
Andy Shevchenko

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

[Alexandre Belloni]

On 16/11/2020 16:43:43+0200, Andy Shevchenko wrote:
> On Thu, Nov 12, 2020 at 02:07:32PM +0100, Claudius Heine wrote:
> > 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.

> 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
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

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

[Andy Shevchenko]

On Mon, Nov 16, 2020 at 7:38 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 16/11/2020 16:43:43+0200, Andy Shevchenko wrote:
> > On Thu, Nov 12, 2020 at 02:07:32PM +0100, Claudius Heine wrote:
> > > 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.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Claudius Heine]

From: Johannes Hahn <johannes-hahn@siemens.com>

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

Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
Signed-off-by: Claudius Heine <ch@denx.de>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
index 1e6e9b19c81c..2fc9e8c2eeeb 100644
--- a/drivers/rtc/rtc-rx6110.c
+++ b/drivers/rtc/rtc-rx6110.c
@@ -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,
-- 
2.20.1

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

[Andy Shevchenko]

On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote:
> From: Johannes Hahn <johannes-hahn@siemens.com>
> 
> 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Mon, 16 Nov 2020 16:46:31 +0200
schrieb Andy Shevchenko <andriy.shevchenko@intel.com>:

> On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote:
> > From: Johannes Hahn <johannes-hahn@siemens.com>
> > 
> > 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.

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

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

[Andy Shevchenko]

On Mon, Nov 16, 2020 at 04:30:24PM +0100, Henning Schild wrote:
> Am Mon, 16 Nov 2020 16:46:31 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@intel.com>:
> 
> > On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote:
> > > From: Johannes Hahn <johannes-hahn@siemens.com>
> > > 
> > > 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.
> 
> 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.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Nov 16, 2020 at 06:05:09PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 16, 2020 at 04:30:24PM +0100, Henning Schild wrote:
> > Am Mon, 16 Nov 2020 16:46:31 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@intel.com>:
> > 
> > > On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote:
> > > > From: Johannes Hahn <johannes-hahn@siemens.com>
> > > > 
> > > > 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.
> > 
> > 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.

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[johannes-hahn]

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 <henning.schild@siemens.com> 
Gesendet: Montag, 16. November 2020 16:30
An: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: Claudius Heine <ch@denx.de>; Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org; Hahn, Johannes (DI FA CTR PLC PRC3) <johannes-hahn@siemens.com>; Zeh, Werner (DI MC MTS R&D HW 1) <werner.zeh@siemens.com>
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 <andriy.shevchenko@intel.com>:

> On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote:
> > From: Johannes Hahn <johannes-hahn@siemens.com>
> > 
> > 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.

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&amp;data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&amp;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

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

[Andy Shevchenko]

On Tue, Nov 17, 2020 at 9:39 AM johannes-hahn@siemens.com
<johannes-hahn@siemens.com> wrote:
>
> 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/20201116142859.31257-1-andriy.shevchenko@linux.intel.com/


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 17, 2020 at 1:10 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Nov 17, 2020 at 9:39 AM johannes-hahn@siemens.com
> <johannes-hahn@siemens.com> wrote:
> >
> > 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/20201116142859.31257-1-andriy.shevchenko@linux.intel.com/

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.

-- 
With Best Regards,
Andy Shevchenko

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

[johannes-hahn]

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

-----Ursprüngliche Nachricht-----
Von: Henning Schild <henning.schild@siemens.com> 
Gesendet: Montag, 16. November 2020 16:30
An: Andy Shevchenko <andriy.shevchenko@intel.com>
Cc: Claudius Heine <ch@denx.de>; Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org; Hahn, Johannes (DI FA CTR PLC PRC3) <johannes-hahn@siemens.com>; Zeh, Werner (DI MC MTS R&D HW 1) <werner.zeh@siemens.com>
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 <andriy.shevchenko@intel.com>:

> On Thu, Nov 12, 2020 at 02:07:33PM +0100, Claudius Heine wrote:
> > From: Johannes Hahn <johannes-hahn@siemens.com>
> > 
> > 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.

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&amp;data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&amp;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

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

[Andy Shevchenko]

On Tue, Nov 17, 2020 at 11:51 AM johannes-hahn@siemens.com
<johannes-hahn@siemens.com> wrote:
>
> 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!

> 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!

> 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.

> 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.

> 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.

> [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

> > 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&amp;data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&amp;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

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

[Andy Shevchenko]

+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.

On Tue, Nov 17, 2020 at 1:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Nov 17, 2020 at 11:51 AM johannes-hahn@siemens.com
> <johannes-hahn@siemens.com> wrote:
> >
> > 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!
>
> > 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!
>
> > 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.
>
> > 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.
>
> > 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.
>
> > [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
>
> > > 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&amp;data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&amp;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



-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

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 <andy.shevchenko@gmail.com>:

> +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.
> 
> On Tue, Nov 17, 2020 at 1:33 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Tue, Nov 17, 2020 at 11:51 AM johannes-hahn@siemens.com
> > <johannes-hahn@siemens.com> wrote:  
> > >
> > > 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!
> >  
> > > 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! 
> > > 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.
> >  
> > > 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.
> >  
> > > 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.
> >  
> > > [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 
> > > > 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&amp;data=04%7C01%7Cjohannes-hahn%40siemens.com%7C21c9e1fe99274df7951a08d88a448af5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637411374276831534%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=7EVdO%2F77LNyvux0y3m9nEf2HZO%2BDm2WkWMfxzaJUoto%3D&amp;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  
> 
> 
> 

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

[werner.zeh]

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 <henning.schild@siemens.com> 
Gesendet: Mittwoch, 18. November 2020 11:04
An: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Hahn, Johannes (DI FA CTR PLC PRC3) <johannes-hahn@siemens.com>; Rafael J. Wysocki <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; Brown, Len <len.brown@intel.com>; Simon Glass <sjg@chromium.org>; Simon Glass <sjg@google.com>; val.krutov@erd.epson.com; Claudius Heine <ch@denx.de>; Alessandro Zummo <a.zummo@towertech.it>; Alexandre Belloni <alexandre.belloni@bootlin.com>; linux-rtc@vger.kernel.org; linux-kernel@vger.kernel.org; Zeh, Werner (DI MC MTS R&D HW 1) <werner.zeh@siemens.com>; Andy Shevchenko <andriy.shevchenko@intel.com>; Mantel, Martin (DI FA CTR PLC PO) <martin.mantel@siemens.com>
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.

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 <andy.shevchenko@gmail.com>:

> +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.
> 
> On Tue, Nov 17, 2020 at 1:33 PM Andy Shevchenko 
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Tue, Nov 17, 2020 at 11:51 AM johannes-hahn@siemens.com 
> > <johannes-hahn@siemens.com> wrote:
> > >
> > > 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!
> >  
> > > 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!
> > > 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.
> >  
> > > 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.
> >  
> > > 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.
> >  
> > > [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&amp;data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=mc2cCsxmX6cjx6HP1yEY7BYd2UmDayTh%2FAqvfA5%2BG20%3D&amp;reserved=0 [3]:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2Fc%2Fcoreboot%2F%2B%2F47235&amp;data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=IPt%2B2j%2Fb7TCIwnQpTh0YQfRhAk3gG34xgs9j5J1p0IA%3D&amp;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&amp;data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e
> > > 4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C
> > > 637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=9DXsd
> > > gRozFKHm%2F3B1i1ls2cwtzip1rsFuJ9qnwFvXOY%3D&amp;reserved=0
> > > [5]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.uefi.org%2FPNP_ACPI_Registry&amp;data=04%7C01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tFOTF40yzDdfVWGKatL56IYo87aCAABHoxPefmLAtPU%3D&amp;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&amp;data=04%7C01%7Cwerner.zeh%40siemens.com%7C57
> > > e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495d55a%
> > > 7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wL
> > > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;s
> > > data=fn6HCTcy1YYOH02dd3uiXymz87jXzTzLO3rWX6S1AeI%3D&amp;reserved=0
> > > [7]: 
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > www.uefi.org%2FPNP_ID_List%3Fsearch%3DSEIKO%2BEPSON&amp;data=04%7C
> > > 01%7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7
> > > C38ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CU
> > > nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I
> > > k1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NDHUqVKgnhNASqTOEydkG%2BocR
> > > eOauHMeo1uCrGCvOyk%3D&amp;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&amp;data=04%7C01%7Cwerner.zeh%40sie
> > mens.com%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab4
> > 2e1495d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJW
> > IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100
> > 0&amp;sdata=y7mB4fH7d98r4VOSeTM2qQ%2BraUvwRrmsznK7qj7W6bM%3D&amp;res
> > erved=0
> > [9]: 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > w.uefi.org%2Fworkinggroups&amp;data=04%7C01%7Cwerner.zeh%40siemens.c
> > om%7C57e192403d8e4648093408d88ba954d5%7C38ae3bcd95794fd4addab42e1495
> > d55a%7C1%7C1%7C637412906669351273%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;
> > sdata=cCwPr1JYgrP7Rou%2FdR0y5DOL%2FUg9MRgnu7QBoOGURZQ%3D&amp;reserve
> > d=0
> > > > 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&amp;data=04%7C01%
> > > 7Cwerner.zeh%40siemens.com%7C57e192403d8e4648093408d88ba954d5%7C38
> > > ae3bcd95794fd4addab42e1495d55a%7C1%7C1%7C637412906669351273%7CUnkn
> > > own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > > aWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=IPt%2B2j%2Fb7TCIwnQpTh0YQfRhAk
> > > 3gG34xgs9j5J1p0IA%3D&amp;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
> 
> 
> 

[PATCH v3 0/1] add ACPI binding to RX6110 driver

[Claudius Heine]

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

-- 
2.30.1

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

[Claudius Heine]

From: Johannes Hahn <johannes-hahn@siemens.com>

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

Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
Co-developed-by: Claudius Heine <ch@denx.de>
Signed-off-by: Claudius Heine <ch@denx.de>
---
 drivers/rtc/rtc-rx6110.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
index 79161d4c6ce4..2a06953c0a84 100644
--- a/drivers/rtc/rtc-rx6110.c
+++ b/drivers/rtc/rtc-rx6110.c
@@ -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,
-- 
2.30.1

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

[Andy Shevchenko]

On Tue, Mar 16, 2021 at 03:48:18PM +0100, Claudius Heine wrote:
> From: Johannes Hahn <johannes-hahn@siemens.com>
> 
> 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 20210316144819.4130622-1-ch@denx.de

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 <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
> Co-developed-by: Claudius Heine <ch@denx.de>
> Signed-off-by: Claudius Heine <ch@denx.de>
> ---
>  drivers/rtc/rtc-rx6110.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
> index 79161d4c6ce4..2a06953c0a84 100644
> --- a/drivers/rtc/rtc-rx6110.c
> +++ b/drivers/rtc/rtc-rx6110.c
> @@ -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),

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

>  	},
>  	.probe		= rx6110_i2c_probe,
>  	.id_table	= rx6110_i2c_id,
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Claudius Heine]

Hi Andy,

On 2021-03-16 17:55, Andy Shevchenko wrote:
> On Tue, Mar 16, 2021 at 03:48:18PM +0100, Claudius Heine wrote:
>> From: Johannes Hahn <johannes-hahn@siemens.com>
>>
>> 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 20210316144819.4130622-1-ch@denx.de
> 
> 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.

> 
> So, repeat again my tag and see one fix to be performed below.
> Reviewed-by: From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Ok, will take care about this in v5.

> 
>> Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
>> Co-developed-by: Claudius Heine <ch@denx.de>
>> Signed-off-by: Claudius Heine <ch@denx.de>
>> ---
>>   drivers/rtc/rtc-rx6110.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
>> index 79161d4c6ce4..2a06953c0a84 100644
>> --- a/drivers/rtc/rtc-rx6110.c
>> +++ b/drivers/rtc/rtc-rx6110.c
>> @@ -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),
> 
> 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

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

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 <lkp@intel.com>

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@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45998 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2296 bytes --]

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 <lkp@intel.com>

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 45998 bytes --]

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

[Andy Shevchenko]

On Wed, Mar 17, 2021 at 03:04:36AM +0800, kernel test robot wrote:
> 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 <lkp@intel.com>
> 
> 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.

Precisely!

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


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

On Wed, Mar 17, 2021 at 03:04:36AM +0800, kernel test robot wrote:
> 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 <lkp@intel.com>
> 
> 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.

Precisely!

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


-- 
With Best Regards,
Andy Shevchenko

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

[Claudius Heine]

From: Johannes Hahn <johannes-hahn@siemens.com>

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

Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
Signed-off-by: Claudius Heine <ch@denx.de>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
index 79161d4c6ce4..29bd697f82cb 100644
--- a/drivers/rtc/rtc-rx6110.c
+++ b/drivers/rtc/rtc-rx6110.c
@@ -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,
-- 
2.30.1

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

[Andy Shevchenko]

On Tue, Mar 16, 2021 at 11:08:05AM +0100, Claudius Heine wrote:
> From: Johannes Hahn <johannes-hahn@siemens.com>
> 
> 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 <andriy.shevchenko@intel.com>
after addressing the below comments.

> Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
> Signed-off-by: Claudius Heine <ch@denx.de>

> Signed-off-by: Henning Schild <henning.schild@siemens.com>

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.

> ---
>  drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
> index 79161d4c6ce4..29bd697f82cb 100644
> --- a/drivers/rtc/rtc-rx6110.c
> +++ b/drivers/rtc/rtc-rx6110.c
> @@ -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.

>  #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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 16, 2021 at 01:30:36PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 16, 2021 at 11:08:05AM +0100, Claudius Heine wrote:
> > From: Johannes Hahn <johannes-hahn@siemens.com>
> > 
> > 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 <andriy.shevchenko@intel.com>
> after addressing the below comments.
> 
> > Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
> > Signed-off-by: Claudius Heine <ch@denx.de>
> 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> 
> 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).


-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Tue, 16 Mar 2021 13:30:36 +0200
schrieb Andy Shevchenko <andriy.shevchenko@intel.com>:

> On Tue, Mar 16, 2021 at 11:08:05AM +0100, Claudius Heine wrote:
> > From: Johannes Hahn <johannes-hahn@siemens.com>
> > 
> > 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 <andriy.shevchenko@intel.com>
> after addressing the below comments.
> 
> > Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
> > Signed-off-by: Claudius Heine <ch@denx.de>  
> 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>  

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

Henning

> 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.
> 
> > ---
> >  drivers/rtc/rtc-rx6110.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c
> > index 79161d4c6ce4..29bd697f82cb 100644
> > --- a/drivers/rtc/rtc-rx6110.c
> > +++ b/drivers/rtc/rtc-rx6110.c
> > @@ -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.
> 
> >  #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  
> 
> This is implied by the stub ACPI_PTR() macro for ACPI=n case.
> I.o.w drop this ugly and redundant ifdeffery.
> 
> > +		.acpi_match_table =
> > ACPI_PTR(rx6110_i2c_acpi_match), +#endif
> >  	},
> >  	.probe		= rx6110_i2c_probe,
> >  	.id_table	= rx6110_i2c_id,
> > -- 
> > 2.30.1
> >   
> 

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

[Andy Shevchenko]

On Tue, Mar 16, 2021 at 12:52:51PM +0100, Henning Schild wrote:
> Am Tue, 16 Mar 2021 13:30:36 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@intel.com>:
> 
> > On Tue, Mar 16, 2021 at 11:08:05AM +0100, Claudius Heine wrote:
> > > From: Johannes Hahn <johannes-hahn@siemens.com>
> > > 
> > > 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 <andriy.shevchenko@intel.com>
> > after addressing the below comments.
> > 
> > > Signed-off-by: Johannes Hahn <johannes-hahn@siemens.com>
> > > Signed-off-by: Claudius Heine <ch@denx.de>  
> > 
> > > Signed-off-by: Henning Schild <henning.schild@siemens.com>  
> 
> Claudius, just remove that. I guess just add yours and mention authors
> in the code if they should receive some recognition.

> > > +#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).

> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, rx6110_i2c_acpi_match);
> > > +#endif

-- 
With Best Regards,
Andy Shevchenko

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

[Claudius Heine]

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

Signed-off-by: Claudius Heine <ch@denx.de>
---
 drivers/rtc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 1fe411ffb19c..c7fb749c1c2f 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -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.
-- 
2.20.1