diff mbox series

[8/8] rtc: max77686: add MAX77714 support

Message ID 20211011155615.257529-9-luca@lucaceresoli.net
State Not Applicable
Headers show
Series Add MAX77714 PMIC minimal driver (RTC and watchdog only) | expand

Commit Message

Luca Ceresoli Oct. 11, 2021, 8:25 p.m. UTC 
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>
---

*** NOTE ***

This patch didn't reach most recipients having hit a limit in my service
provider (125 e-mails per hour). I'm resending it, as far as possible with
proper message-id etc. Apologies for any duplicate.

 drivers/rtc/Kconfig        |  2 +-
 drivers/rtc/rtc-max77686.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

--
2.25.1

Comments

Krzysztof Kozlowski Oct. 12, 2021, 8:20 a.m. UTC | #1 
On 11/10/2021 18:12, Luca Ceresoli wrote:
> 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:
>> 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.

Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 12, 2021, 8:29 a.m. UTC | #2 
On 11/10/2021 22:25, Luca Ceresoli wrote:
> 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>
> ---
> 
> *** NOTE ***
> 
> This patch didn't reach most recipients having hit a limit in my service
> provider (125 e-mails per hour). I'm resending it, as far as possible with
> proper message-id etc. Apologies for any duplicate.
> 
>  drivers/rtc/Kconfig        |  2 +-
>  drivers/rtc/rtc-max77686.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof
Luca Ceresoli Oct. 15, 2021, 4:46 p.m. UTC | #3 
Hi,

On 12/10/21 10:20, Krzysztof Kozlowski wrote:
> On 11/10/2021 18:12, Luca Ceresoli wrote:
>> 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:
>>> 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.
Alexandre Belloni Oct. 15, 2021, 5:36 p.m. UTC | #4 
On 11/10/2021 22:25:50+0200, Luca Ceresoli wrote:
> 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>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> 
> *** NOTE ***
> 
> This patch didn't reach most recipients having hit a limit in my service
> provider (125 e-mails per hour). I'm resending it, as far as possible with
> proper message-id etc. Apologies for any duplicate.
> 
>  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,
> +	/* On MAX77714 RTCA1 is BIT 1 of RTCINT (0x00). Not supported by this driver. */
> +	.alarm_pending_status_reg = MAX77686_INVALID_REG,
> +	.rtc_i2c_addr = MAX77714_I2C_ADDR_RTC,
> +	.rtc_irq_chip = &max77714_rtc_irq_chip,
> +	.regmap_config = &max77686_rtc_regmap_config,
> +};
> +
>  static const struct regmap_config max77620_rtc_regmap_config = {
>  	.reg_bits = 8,
>  	.val_bits = 8,
> @@ -846,6 +869,7 @@ static const struct platform_device_id rtc_id[] = {
>  	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
>  	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
>  	{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
> +	{ "max77714-rtc", .driver_data = (kernel_ulong_t)&max77714_drv_data, },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(platform, rtc_id);
> --
> 2.25.1
diff mbox series

Patch

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,
+	/* On MAX77714 RTCA1 is BIT 1 of RTCINT (0x00). Not supported by this driver. */
+	.alarm_pending_status_reg = MAX77686_INVALID_REG,
+	.rtc_i2c_addr = MAX77714_I2C_ADDR_RTC,
+	.rtc_irq_chip = &max77714_rtc_irq_chip,
+	.regmap_config = &max77686_rtc_regmap_config,
+};
+
 static const struct regmap_config max77620_rtc_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -846,6 +869,7 @@  static const struct platform_device_id rtc_id[] = {
 	{ "max77686-rtc", .driver_data = (kernel_ulong_t)&max77686_drv_data, },
 	{ "max77802-rtc", .driver_data = (kernel_ulong_t)&max77802_drv_data, },
 	{ "max77620-rtc", .driver_data = (kernel_ulong_t)&max77620_drv_data, },
+	{ "max77714-rtc", .driver_data = (kernel_ulong_t)&max77714_drv_data, },
 	{},
 };
 MODULE_DEVICE_TABLE(platform, rtc_id);