Hi,

On 12/10/21 10:20, Krzysztof Kozlowski wrote:

On 11/10/2021 18:12, Luca Ceresoli wrote:

quoted

Hi,

see below for the issues with interrupt implementation that I mentioned
in the cover letter.

On 11/10/21 17:56, Luca Ceresoli wrote:

quoted

The RTC included in the MAX77714 PMIC is very similar to the one in the
MAX77686. Reuse the rtc-max77686.c driver with the minimum required changes
for the MAX77714 RTC.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/rtc/Kconfig        |  2 +-
 drivers/rtc/rtc-max77686.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e1bc5214494e..a73591ad292b 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig

@@ -375,7 +375,7 @@ config RTC_DRV_MAX8997
 
 config RTC_DRV_MAX77686
 	tristate "Maxim MAX77686"
-	depends on MFD_MAX77686 || MFD_MAX77620 || COMPILE_TEST
+	depends on MFD_MAX77686 || MFD_MAX77620 || MFD_MAX77714 || COMPILE_TEST
 	help
 	  If you say yes here you will get support for the
 	  RTC of Maxim MAX77686/MAX77620/MAX77802 PMIC.

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 9901c596998a..e6564bc2171e 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c

@@ -19,6 +19,7 @@
 
 #define MAX77686_I2C_ADDR_RTC		(0x0C >> 1)
 #define MAX77620_I2C_ADDR_RTC		0x68
+#define MAX77714_I2C_ADDR_RTC		0x48
 #define MAX77686_INVALID_I2C_ADDR	(-1)
 
 /* Define non existing register */

@@ -203,6 +204,28 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.regmap_config = &max77686_rtc_regmap_config,
 };
 
+static const struct regmap_irq_chip max77714_rtc_irq_chip = {
+	.name		= "max77714-rtc",
+	.status_base	= MAX77686_RTC_INT,
+	.mask_base	= MAX77686_RTC_INTM,
+	.num_regs	= 1,
+	.irqs		= max77686_rtc_irqs,
+	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs) - 1, /* no WTSR on 77714 */
+};
+
+static const struct max77686_rtc_driver_data max77714_drv_data = {
+	.delay = 16000,
+	.mask  = 0x7f,
+	.map   = max77686_map,
+	.alarm_enable_reg = false,
+	.rtc_irq_from_platform = false,

As far as I could understand, rtc_irq_from_platform should be 'true'.
This would trigger the 'if' branch in function
max77686_init_rtc_regmap() [0]:

  if (info->drv_data->rtc_irq_from_platform) {
	struct platform_device *pdev = to_platform_device(info->dev);

	info->rtc_irq = platform_get_irq(pdev, 0);
	if (info->rtc_irq < 0)
		return info->rtc_irq;
  } else {
	info->rtc_irq =  parent_i2c->irq;
  }

Calling platform_get_irq() seems correct for the MAX77714, which can
generate various IRQ events, collecting them in a register, and raise a
single IRQ to the CPU via a physical pin.

However, if I set rtc_irq_from_platform = true, platform_get_irq()
returns IRQ number '1', which ends up in:

  dummy 0-0048: Failed to request IRQ 1 for max77714-rtc: -22
  max77686-rtc max77714-rtc: Failed to add RTC irq chip: -22
  max77686-rtc: probe of max77714-rtc failed with error -22

I compared my code with other MFD drivers and their cell drivers (but
their datasheets is not available so I had to add some guesswork), and
couldn't find out where my code is wrong.

Unfortunately I have no IRQ access on my board (and I don't need them
for my use case). For this reason I initially thought of disabling all
the IRQ code in rtc-max77686.c via a new flag, but it would be quite
invasive and I wouldn't even be able to test that existing hardware
still works. Implementing a new RTC driver for the MAX77714 does not
seem to be a sane option as the hardware is really 99% equal to the
MAX77686 RTC.

I think the flag should be false, not true. The true means you have RTC
device with its own interrupt. For example in DT it could look like:

  pmic@1c {
      compatible = "maxim,max77714";
      reg = <0x1c>;
      interrupt-parent = <&gpio2>;
      interrupts = <3 IRQ_TYPE_LEVEL_LOW>;

      interrupt-controller;
      #interrupt-cells = <2>;
   };

   rtc@48 {
      compatible = "maxim,max77714-rtc";
      reg = <0x48>;
      interrupt-parent = <&gpio2>;
      interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
   };

In your case, the RTC device will not have its own devicetree node and
will be instantiated as MFD child device. The only interrupt line
available is the parents interrupt line - the same as in max77686 and
max77802 setups.

Have in mind that this does not necessarily reflect real HW, but how we
represent it in devicetree and driver model.

Good to know. Thank you for the explanation.

-- 
Luca