Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
@ 2020-04-17 17:08 Thierry Reding
  2020-04-17 17:08 ` [PATCH 2/3] rtc: max77686: Make wakeup support configurable Thierry Reding
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Thierry Reding @ 2020-04-17 17:08 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, Alessandro Zummo, Alexandre Belloni
  Cc: Jon Hunter, devicetree, linux-rtc, linux-tegra, linux-kernel

From: Thierry Reding <treding@nvidia.com>

The RTC present on MAX77620 can be used to generate an alarm at a given
time, which in turn can be used as a wakeup source for the system if it
is properly wired up.

Document how to enable the RTC to act as a wakeup source.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/mfd/max77620.txt          | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max77620.txt b/Documentation/devicetree/bindings/mfd/max77620.txt
index 5a642a51d58e..f05005b0993e 100644
--- a/Documentation/devicetree/bindings/mfd/max77620.txt
+++ b/Documentation/devicetree/bindings/mfd/max77620.txt
@@ -125,6 +125,17 @@ MAX77663 supports 20, 40, 80, 160, 320, 640, 1280 and 2540 microseconds.
 			control) then, GPIO1/nRST_IO goes LOW.
 			this property is valid for max20024 only.
 
+Realtime Clock
+--------------
+The MAX77620 family of power management ICs contain a realtime clock block
+that can be used to keep track of time even when the system is powered off.
+
+The realtime clock can also be programmed to trigger alerts, which can be
+used to wake the system up from sleep. In order to configure the RTC to act
+as a wakeup source, add an "rtc" child node and add the "wakeup-source"
+property.
+
+
 For DT binding details of different sub modules like GPIO, pincontrol,
 regulator, power, please refer respective device-tree binding document
 under their respective sub-system directories.
@@ -159,4 +170,8 @@ max77620@3c {
 			maxim,fps-event-source = <MAX77620_FPS_EVENT_SRC_SW>;
 		};
 	};
+
+	rtc {
+		wakeup-source;
+	};
 };
-- 
2.24.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] rtc: max77686: Make wakeup support configurable
  2020-04-17 17:08 [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620 Thierry Reding
@ 2020-04-17 17:08 ` Thierry Reding
  2020-04-20 14:42   ` Jon Hunter
  2020-04-17 17:08 ` [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620 Thierry Reding
  2020-04-30 14:07 ` [PATCH 1/3] dt-bindings: mfd: Document the RTC present " Rob Herring
  2 siblings, 1 reply; 12+ messages in thread
From: Thierry Reding @ 2020-04-17 17:08 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, Alessandro Zummo, Alexandre Belloni
  Cc: Jon Hunter, devicetree, linux-rtc, linux-tegra, linux-kernel

From: Thierry Reding <treding@nvidia.com>

Use the standard "wakeup-source" device tree property to determine if
the RTC can act as a wakeup source for the system. Note that the driver
by default always assumes that the RTC can act as a wakeup source, but
whether it can really do so or not always depends on how it is hooked
up.

In order to preserve backwards compatibility with older device trees,
only parse the "wakeup-source" property when a device tree node was
associated with the RTC device. This doesn't typically happen because
the top-level MFD driver doesn't list any compatible strings that can
be used to map child nodes to child devices. As a fallback, check if a
child node named "rtc" exists and use that instead.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Note that we could obviously add support to the MFD driver to match
subdevices to their device tree nodes by compatible string, but there
are side-effects, such as the driver core complaining about the lack
of a DMA mask for these devices. That in turn could also be fixed but
it ends up all being rather hacky, so just looking up a child node by
name seems like a good compromise, especially since there are already
such subnodes for some of the other subdevices of this PMIC.
---
 drivers/rtc/rtc-max77686.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index d5a0e27dd0a0..35fd74b83626 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -722,6 +722,8 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 {
 	struct max77686_rtc_info *info;
 	const struct platform_device_id *id = platform_get_device_id(pdev);
+	struct device_node *np = of_node_get(pdev->dev.of_node);
+	bool wakeup = true;
 	int ret;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(struct max77686_rtc_info),
@@ -746,7 +748,21 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 		goto err_rtc;
 	}
 
-	device_init_wakeup(&pdev->dev, 1);
+	/*
+	 * Only check for the wakeup-source property if there's an actual
+	 * device tree node for the RTC. If no device tree node had been
+	 * attached during device instantiation, try looking up the "rtc"
+	 * child node of the parent's device tree node.
+	 */
+	if (!np && pdev->dev.parent->of_node)
+		np = of_get_child_by_name(pdev->dev.parent->of_node, "rtc");
+
+	if (np) {
+		wakeup = of_property_read_bool(np, "wakeup-source");
+		of_node_put(np);
+	}
+
+	device_init_wakeup(&pdev->dev, wakeup);
 
 	info->rtc_dev = devm_rtc_device_register(&pdev->dev, id->name,
 					&max77686_rtc_ops, THIS_MODULE);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620
  2020-04-17 17:08 [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620 Thierry Reding
  2020-04-17 17:08 ` [PATCH 2/3] rtc: max77686: Make wakeup support configurable Thierry Reding
@ 2020-04-17 17:08 ` Thierry Reding
  2020-04-20 14:43   ` Jon Hunter
  2020-05-11 14:27   ` Alexandre Belloni
  2020-04-30 14:07 ` [PATCH 1/3] dt-bindings: mfd: Document the RTC present " Rob Herring
  2 siblings, 2 replies; 12+ messages in thread
From: Thierry Reding @ 2020-04-17 17:08 UTC (permalink / raw)
  To: Lee Jones, Rob Herring, Alessandro Zummo, Alexandre Belloni
  Cc: Jon Hunter, devicetree, linux-rtc, linux-tegra, linux-kernel

From: Thierry Reding <treding@nvidia.com>

The MAX77620 doesn't support bulk writes, so make sure the regmap code
breaks bulk writes into multiple single-byte writes.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/rtc/rtc-max77686.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 35fd74b83626..f53db4d6bead 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -78,6 +78,8 @@ struct max77686_rtc_driver_data {
 	int			alarm_pending_status_reg;
 	/* RTC IRQ CHIP for regmap */
 	const struct regmap_irq_chip *rtc_irq_chip;
+	/* regmap configuration for the chip */
+	const struct regmap_config *regmap_config;
 };
 
 struct max77686_rtc_info {
@@ -182,6 +184,11 @@ static const struct regmap_irq_chip max77686_rtc_irq_chip = {
 	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
 };
 
+static const struct regmap_config max77686_rtc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
 static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.delay = 16000,
 	.mask  = 0x7f,
@@ -191,6 +198,13 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
 	.alarm_pending_status_reg = MAX77686_REG_STATUS2,
 	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
 	.rtc_irq_chip = &max77686_rtc_irq_chip,
+	.regmap_config = &max77686_rtc_regmap_config,
+};
+
+static const struct regmap_config max77620_rtc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.use_single_write = true,
 };
 
 static const struct max77686_rtc_driver_data max77620_drv_data = {
@@ -202,6 +216,7 @@ static const struct max77686_rtc_driver_data max77620_drv_data = {
 	.alarm_pending_status_reg = MAX77686_INVALID_REG,
 	.rtc_i2c_addr = MAX77620_I2C_ADDR_RTC,
 	.rtc_irq_chip = &max77686_rtc_irq_chip,
+	.regmap_config = &max77620_rtc_regmap_config,
 };
 
 static const unsigned int max77802_map[REG_RTC_END] = {
@@ -658,11 +673,6 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
 	return ret;
 }
 
-static const struct regmap_config max77686_rtc_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
-};
-
 static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
 {
 	struct device *parent = info->dev->parent;
@@ -698,7 +708,7 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
 	}
 
 	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
-						&max77686_rtc_regmap_config);
+						info->drv_data->regmap_config);
 	if (IS_ERR(info->rtc_regmap)) {
 		ret = PTR_ERR(info->rtc_regmap);
 		dev_err(info->dev, "Failed to allocate RTC regmap: %d\n", ret);
-- 
2.24.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] rtc: max77686: Make wakeup support configurable
  2020-04-17 17:08 ` [PATCH 2/3] rtc: max77686: Make wakeup support configurable Thierry Reding
@ 2020-04-20 14:42   ` Jon Hunter
  0 siblings, 0 replies; 12+ messages in thread
From: Jon Hunter @ 2020-04-20 14:42 UTC (permalink / raw)
  To: Thierry Reding, Lee Jones, Rob Herring, Alessandro Zummo,
	Alexandre Belloni
  Cc: devicetree, linux-rtc, linux-tegra, linux-kernel


On 17/04/2020 18:08, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Use the standard "wakeup-source" device tree property to determine if
> the RTC can act as a wakeup source for the system. Note that the driver
> by default always assumes that the RTC can act as a wakeup source, but
> whether it can really do so or not always depends on how it is hooked
> up.
> 
> In order to preserve backwards compatibility with older device trees,
> only parse the "wakeup-source" property when a device tree node was
> associated with the RTC device. This doesn't typically happen because
> the top-level MFD driver doesn't list any compatible strings that can
> be used to map child nodes to child devices. As a fallback, check if a
> child node named "rtc" exists and use that instead.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Note that we could obviously add support to the MFD driver to match
> subdevices to their device tree nodes by compatible string, but there
> are side-effects, such as the driver core complaining about the lack
> of a DMA mask for these devices. That in turn could also be fixed but
> it ends up all being rather hacky, so just looking up a child node by
> name seems like a good compromise, especially since there are already
> such subnodes for some of the other subdevices of this PMIC.
> ---
>  drivers/rtc/rtc-max77686.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index d5a0e27dd0a0..35fd74b83626 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -722,6 +722,8 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  {
>  	struct max77686_rtc_info *info;
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	struct device_node *np = of_node_get(pdev->dev.of_node);
> +	bool wakeup = true;
>  	int ret;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(struct max77686_rtc_info),
> @@ -746,7 +748,21 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>  		goto err_rtc;
>  	}
>  
> -	device_init_wakeup(&pdev->dev, 1);
> +	/*
> +	 * Only check for the wakeup-source property if there's an actual
> +	 * device tree node for the RTC. If no device tree node had been
> +	 * attached during device instantiation, try looking up the "rtc"
> +	 * child node of the parent's device tree node.
> +	 */
> +	if (!np && pdev->dev.parent->of_node)
> +		np = of_get_child_by_name(pdev->dev.parent->of_node, "rtc");
> +
> +	if (np) {
> +		wakeup = of_property_read_bool(np, "wakeup-source");
> +		of_node_put(np);
> +	}
> +
> +	device_init_wakeup(&pdev->dev, wakeup);
>  
>  	info->rtc_dev = devm_rtc_device_register(&pdev->dev, id->name,
>  					&max77686_rtc_ops, THIS_MODULE);
> 

Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620
  2020-04-17 17:08 ` [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620 Thierry Reding
@ 2020-04-20 14:43   ` Jon Hunter
  2020-05-11 14:27   ` Alexandre Belloni
  1 sibling, 0 replies; 12+ messages in thread
From: Jon Hunter @ 2020-04-20 14:43 UTC (permalink / raw)
  To: Thierry Reding, Lee Jones, Rob Herring, Alessandro Zummo,
	Alexandre Belloni
  Cc: devicetree, linux-rtc, linux-tegra, linux-kernel


On 17/04/2020 18:08, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The MAX77620 doesn't support bulk writes, so make sure the regmap code
> breaks bulk writes into multiple single-byte writes.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
> index 35fd74b83626..f53db4d6bead 100644
> --- a/drivers/rtc/rtc-max77686.c
> +++ b/drivers/rtc/rtc-max77686.c
> @@ -78,6 +78,8 @@ struct max77686_rtc_driver_data {
>  	int			alarm_pending_status_reg;
>  	/* RTC IRQ CHIP for regmap */
>  	const struct regmap_irq_chip *rtc_irq_chip;
> +	/* regmap configuration for the chip */
> +	const struct regmap_config *regmap_config;
>  };
>  
>  struct max77686_rtc_info {
> @@ -182,6 +184,11 @@ static const struct regmap_irq_chip max77686_rtc_irq_chip = {
>  	.num_irqs	= ARRAY_SIZE(max77686_rtc_irqs),
>  };
>  
> +static const struct regmap_config max77686_rtc_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
>  static const struct max77686_rtc_driver_data max77686_drv_data = {
>  	.delay = 16000,
>  	.mask  = 0x7f,
> @@ -191,6 +198,13 @@ static const struct max77686_rtc_driver_data max77686_drv_data = {
>  	.alarm_pending_status_reg = MAX77686_REG_STATUS2,
>  	.rtc_i2c_addr = MAX77686_I2C_ADDR_RTC,
>  	.rtc_irq_chip = &max77686_rtc_irq_chip,
> +	.regmap_config = &max77686_rtc_regmap_config,
> +};
> +
> +static const struct regmap_config max77620_rtc_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.use_single_write = true,
>  };
>  
>  static const struct max77686_rtc_driver_data max77620_drv_data = {
> @@ -202,6 +216,7 @@ static const struct max77686_rtc_driver_data max77620_drv_data = {
>  	.alarm_pending_status_reg = MAX77686_INVALID_REG,
>  	.rtc_i2c_addr = MAX77620_I2C_ADDR_RTC,
>  	.rtc_irq_chip = &max77686_rtc_irq_chip,
> +	.regmap_config = &max77620_rtc_regmap_config,
>  };
>  
>  static const unsigned int max77802_map[REG_RTC_END] = {
> @@ -658,11 +673,6 @@ static int max77686_rtc_init_reg(struct max77686_rtc_info *info)
>  	return ret;
>  }
>  
> -static const struct regmap_config max77686_rtc_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -};
> -
>  static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>  {
>  	struct device *parent = info->dev->parent;
> @@ -698,7 +708,7 @@ static int max77686_init_rtc_regmap(struct max77686_rtc_info *info)
>  	}
>  
>  	info->rtc_regmap = devm_regmap_init_i2c(info->rtc,
> -						&max77686_rtc_regmap_config);
> +						info->drv_data->regmap_config);
>  	if (IS_ERR(info->rtc_regmap)) {
>  		ret = PTR_ERR(info->rtc_regmap);
>  		dev_err(info->dev, "Failed to allocate RTC regmap: %d\n", ret);
> 


Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
  2020-04-17 17:08 [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620 Thierry Reding
  2020-04-17 17:08 ` [PATCH 2/3] rtc: max77686: Make wakeup support configurable Thierry Reding
  2020-04-17 17:08 ` [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620 Thierry Reding
@ 2020-04-30 14:07 ` Rob Herring
  2020-04-30 14:15   ` Alexandre Belloni
  2 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2020-04-30 14:07 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Lee Jones, Alessandro Zummo, Alexandre Belloni, Jon Hunter,
	devicetree, linux-rtc, linux-tegra, linux-kernel

On Fri, Apr 17, 2020 at 07:08:23PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The RTC present on MAX77620 can be used to generate an alarm at a given
> time, which in turn can be used as a wakeup source for the system if it
> is properly wired up.
> 
> Document how to enable the RTC to act as a wakeup source.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/mfd/max77620.txt          | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max77620.txt b/Documentation/devicetree/bindings/mfd/max77620.txt
> index 5a642a51d58e..f05005b0993e 100644
> --- a/Documentation/devicetree/bindings/mfd/max77620.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77620.txt
> @@ -125,6 +125,17 @@ MAX77663 supports 20, 40, 80, 160, 320, 640, 1280 and 2540 microseconds.
>  			control) then, GPIO1/nRST_IO goes LOW.
>  			this property is valid for max20024 only.
>  
> +Realtime Clock
> +--------------
> +The MAX77620 family of power management ICs contain a realtime clock block
> +that can be used to keep track of time even when the system is powered off.
> +
> +The realtime clock can also be programmed to trigger alerts, which can be
> +used to wake the system up from sleep. In order to configure the RTC to act
> +as a wakeup source, add an "rtc" child node and add the "wakeup-source"
> +property.
> +
> +
>  For DT binding details of different sub modules like GPIO, pincontrol,
>  regulator, power, please refer respective device-tree binding document
>  under their respective sub-system directories.
> @@ -159,4 +170,8 @@ max77620@3c {
>  			maxim,fps-event-source = <MAX77620_FPS_EVENT_SRC_SW>;
>  		};
>  	};
> +
> +	rtc {
> +		wakeup-source;

Is the RTC really the only thing that could wake the system in this 
PMIC?

I don't think it's really valid to have 'wakeup-source' without 
'interrupts' unless the wakeup mechanism is somehow not an interrupt. So 
I think this belongs in the parent node.

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
  2020-04-30 14:07 ` [PATCH 1/3] dt-bindings: mfd: Document the RTC present " Rob Herring
@ 2020-04-30 14:15   ` Alexandre Belloni
  2020-05-01 13:00     ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandre Belloni @ 2020-04-30 14:15 UTC (permalink / raw)
  To: Rob Herring
  Cc: Thierry Reding, Lee Jones, Alessandro Zummo, Jon Hunter,
	devicetree, linux-rtc, linux-tegra, linux-kernel

On 30/04/2020 09:07:01-0500, Rob Herring wrote:
> On Fri, Apr 17, 2020 at 07:08:23PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The RTC present on MAX77620 can be used to generate an alarm at a given
> > time, which in turn can be used as a wakeup source for the system if it
> > is properly wired up.
> > 
> > Document how to enable the RTC to act as a wakeup source.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../devicetree/bindings/mfd/max77620.txt          | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/max77620.txt b/Documentation/devicetree/bindings/mfd/max77620.txt
> > index 5a642a51d58e..f05005b0993e 100644
> > --- a/Documentation/devicetree/bindings/mfd/max77620.txt
> > +++ b/Documentation/devicetree/bindings/mfd/max77620.txt
> > @@ -125,6 +125,17 @@ MAX77663 supports 20, 40, 80, 160, 320, 640, 1280 and 2540 microseconds.
> >  			control) then, GPIO1/nRST_IO goes LOW.
> >  			this property is valid for max20024 only.
> >  
> > +Realtime Clock
> > +--------------
> > +The MAX77620 family of power management ICs contain a realtime clock block
> > +that can be used to keep track of time even when the system is powered off.
> > +
> > +The realtime clock can also be programmed to trigger alerts, which can be
> > +used to wake the system up from sleep. In order to configure the RTC to act
> > +as a wakeup source, add an "rtc" child node and add the "wakeup-source"
> > +property.
> > +
> > +
> >  For DT binding details of different sub modules like GPIO, pincontrol,
> >  regulator, power, please refer respective device-tree binding document
> >  under their respective sub-system directories.
> > @@ -159,4 +170,8 @@ max77620@3c {
> >  			maxim,fps-event-source = <MAX77620_FPS_EVENT_SRC_SW>;
> >  		};
> >  	};
> > +
> > +	rtc {
> > +		wakeup-source;
> 
> Is the RTC really the only thing that could wake the system in this 
> PMIC?
> 
> I don't think it's really valid to have 'wakeup-source' without 
> 'interrupts' unless the wakeup mechanism is somehow not an interrupt. So 
> I think this belongs in the parent node.
> 

I don't think this is true because in the case of a discrete RTC, its
interrupt pin can be connected directly to a PMIC to power up a board
instead of being connected to the SoC. In that case we don't have an
interrupt property but the RTC is still a wakeup source. This is the
usual use case for wakeup-source in the RTC subsystem. Else, if there is
an interrupt, then we assume the RTC is a wakeup source and there is no
need to have the wakeup-source property.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
  2020-04-30 14:15   ` Alexandre Belloni
@ 2020-05-01 13:00     ` Rob Herring
  2020-05-01 13:53       ` Alexandre Belloni
  0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2020-05-01 13:00 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Thierry Reding, Lee Jones, Alessandro Zummo, Jon Hunter,
	devicetree, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	linux-tegra, linux-kernel

On Thu, Apr 30, 2020 at 9:15 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 30/04/2020 09:07:01-0500, Rob Herring wrote:
> > On Fri, Apr 17, 2020 at 07:08:23PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > The RTC present on MAX77620 can be used to generate an alarm at a given
> > > time, which in turn can be used as a wakeup source for the system if it
> > > is properly wired up.
> > >
> > > Document how to enable the RTC to act as a wakeup source.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  .../devicetree/bindings/mfd/max77620.txt          | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/max77620.txt b/Documentation/devicetree/bindings/mfd/max77620.txt
> > > index 5a642a51d58e..f05005b0993e 100644
> > > --- a/Documentation/devicetree/bindings/mfd/max77620.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/max77620.txt
> > > @@ -125,6 +125,17 @@ MAX77663 supports 20, 40, 80, 160, 320, 640, 1280 and 2540 microseconds.
> > >                     control) then, GPIO1/nRST_IO goes LOW.
> > >                     this property is valid for max20024 only.
> > >
> > > +Realtime Clock
> > > +--------------
> > > +The MAX77620 family of power management ICs contain a realtime clock block
> > > +that can be used to keep track of time even when the system is powered off.
> > > +
> > > +The realtime clock can also be programmed to trigger alerts, which can be
> > > +used to wake the system up from sleep. In order to configure the RTC to act
> > > +as a wakeup source, add an "rtc" child node and add the "wakeup-source"
> > > +property.
> > > +
> > > +
> > >  For DT binding details of different sub modules like GPIO, pincontrol,
> > >  regulator, power, please refer respective device-tree binding document
> > >  under their respective sub-system directories.
> > > @@ -159,4 +170,8 @@ max77620@3c {
> > >                     maxim,fps-event-source = <MAX77620_FPS_EVENT_SRC_SW>;
> > >             };
> > >     };
> > > +
> > > +   rtc {
> > > +           wakeup-source;
> >
> > Is the RTC really the only thing that could wake the system in this
> > PMIC?
> >
> > I don't think it's really valid to have 'wakeup-source' without
> > 'interrupts' unless the wakeup mechanism is somehow not an interrupt. So
> > I think this belongs in the parent node.
> >
>
> I don't think this is true because in the case of a discrete RTC, its
> interrupt pin can be connected directly to a PMIC to power up a board
> instead of being connected to the SoC. In that case we don't have an
> interrupt property but the RTC is still a wakeup source. This is the
> usual use case for wakeup-source in the RTC subsystem. Else, if there is
> an interrupt, then we assume the RTC is a wakeup source and there is no
> need to have the wakeup-source property.

Yes, that would be an example of "unless the wakeup mechanism is
somehow not an interrupt". I guess I should add not an interrupt from
the perspective of the OS.

So if the wakeup is self contained within the PMIC, why do we need a
DT property? The capability is always there and enabling/disabling
wakeup from it is userspace policy.

Rob

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
  2020-05-01 13:00     ` Rob Herring
@ 2020-05-01 13:53       ` Alexandre Belloni
  2020-05-08 11:02         ` Thierry Reding
  0 siblings, 1 reply; 12+ messages in thread
From: Alexandre Belloni @ 2020-05-01 13:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Thierry Reding, Lee Jones, Alessandro Zummo, Jon Hunter,
	devicetree, open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
	linux-tegra, linux-kernel

On 01/05/2020 08:00:11-0500, Rob Herring wrote:
> > I don't think this is true because in the case of a discrete RTC, its
> > interrupt pin can be connected directly to a PMIC to power up a board
> > instead of being connected to the SoC. In that case we don't have an
> > interrupt property but the RTC is still a wakeup source. This is the
> > usual use case for wakeup-source in the RTC subsystem. Else, if there is
> > an interrupt, then we assume the RTC is a wakeup source and there is no
> > need to have the wakeup-source property.
> 
> Yes, that would be an example of "unless the wakeup mechanism is
> somehow not an interrupt". I guess I should add not an interrupt from
> the perspective of the OS.
> 
> So if the wakeup is self contained within the PMIC, why do we need a
> DT property? The capability is always there and enabling/disabling
> wakeup from it is userspace policy.
> 

Yes, for this particular case, I'm not sure wakeup-source is actually
necessary. If the interrupt line is used to wakeup the SoC, then the
presence of the interrupts property is enough to enable wakeup.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
  2020-05-01 13:53       ` Alexandre Belloni
@ 2020-05-08 11:02         ` Thierry Reding
  2020-05-11 14:28           ` Alexandre Belloni
  0 siblings, 1 reply; 12+ messages in thread
From: Thierry Reding @ 2020-05-08 11:02 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Rob Herring, Lee Jones, Alessandro Zummo, Jon Hunter, devicetree,
	open list:REAL TIME CLOCK (RTC) SUBSYSTEM, linux-tegra,
	linux-kernel


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

On Fri, May 01, 2020 at 03:53:09PM +0200, Alexandre Belloni wrote:
> On 01/05/2020 08:00:11-0500, Rob Herring wrote:
> > > I don't think this is true because in the case of a discrete RTC, its
> > > interrupt pin can be connected directly to a PMIC to power up a board
> > > instead of being connected to the SoC. In that case we don't have an
> > > interrupt property but the RTC is still a wakeup source. This is the
> > > usual use case for wakeup-source in the RTC subsystem. Else, if there is
> > > an interrupt, then we assume the RTC is a wakeup source and there is no
> > > need to have the wakeup-source property.
> > 
> > Yes, that would be an example of "unless the wakeup mechanism is
> > somehow not an interrupt". I guess I should add not an interrupt from
> > the perspective of the OS.
> > 
> > So if the wakeup is self contained within the PMIC, why do we need a
> > DT property? The capability is always there and enabling/disabling
> > wakeup from it is userspace policy.
> > 
> 
> Yes, for this particular case, I'm not sure wakeup-source is actually
> necessary. If the interrupt line is used to wakeup the SoC, then the
> presence of the interrupts property is enough to enable wakeup.

So yes, the wakeup-source property isn't necessary. The goal of patches
1 and 2 was to allow the RTC to be actually disabled as a wakeup-source
in case it didn't work as intended. But since the RTC is enabled as a
wakeup source on these PMICs by default, the idea was to add a new sub-
node for the RTC and required the wakeup-source in that subnode if that
subnode was present.

That said, patch 3 actually does make the RTC work as a wakeup source
on the particular board that I tested this, so patches 1 and 2 are no
longer really required from my point of view.

Do you want me to send patch 3/3 again separately or can you pick it up
from this series?

Thanks,
Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620
  2020-04-17 17:08 ` [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620 Thierry Reding
  2020-04-20 14:43   ` Jon Hunter
@ 2020-05-11 14:27   ` Alexandre Belloni
  1 sibling, 0 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-05-11 14:27 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Lee Jones, Rob Herring, Alessandro Zummo, Jon Hunter, devicetree,
	linux-rtc, linux-tegra, linux-kernel

On 17/04/2020 19:08:25+0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The MAX77620 doesn't support bulk writes, so make sure the regmap code
> breaks bulk writes into multiple single-byte writes.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/rtc/rtc-max77686.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
Applied, thanks.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620
  2020-05-08 11:02         ` Thierry Reding
@ 2020-05-11 14:28           ` Alexandre Belloni
  0 siblings, 0 replies; 12+ messages in thread
From: Alexandre Belloni @ 2020-05-11 14:28 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rob Herring, Lee Jones, Alessandro Zummo, Jon Hunter, devicetree,
	open list:REAL TIME CLOCK (RTC) SUBSYSTEM, linux-tegra,
	linux-kernel

Hi,

On 08/05/2020 13:02:26+0200, Thierry Reding wrote:
> On Fri, May 01, 2020 at 03:53:09PM +0200, Alexandre Belloni wrote:
> > On 01/05/2020 08:00:11-0500, Rob Herring wrote:
> > > > I don't think this is true because in the case of a discrete RTC, its
> > > > interrupt pin can be connected directly to a PMIC to power up a board
> > > > instead of being connected to the SoC. In that case we don't have an
> > > > interrupt property but the RTC is still a wakeup source. This is the
> > > > usual use case for wakeup-source in the RTC subsystem. Else, if there is
> > > > an interrupt, then we assume the RTC is a wakeup source and there is no
> > > > need to have the wakeup-source property.
> > > 
> > > Yes, that would be an example of "unless the wakeup mechanism is
> > > somehow not an interrupt". I guess I should add not an interrupt from
> > > the perspective of the OS.
> > > 
> > > So if the wakeup is self contained within the PMIC, why do we need a
> > > DT property? The capability is always there and enabling/disabling
> > > wakeup from it is userspace policy.
> > > 
> > 
> > Yes, for this particular case, I'm not sure wakeup-source is actually
> > necessary. If the interrupt line is used to wakeup the SoC, then the
> > presence of the interrupts property is enough to enable wakeup.
> 
> So yes, the wakeup-source property isn't necessary. The goal of patches
> 1 and 2 was to allow the RTC to be actually disabled as a wakeup-source
> in case it didn't work as intended. But since the RTC is enabled as a
> wakeup source on these PMICs by default, the idea was to add a new sub-
> node for the RTC and required the wakeup-source in that subnode if that
> subnode was present.
> 
> That said, patch 3 actually does make the RTC work as a wakeup source
> on the particular board that I tested this, so patches 1 and 2 are no
> longer really required from my point of view.
> 
> Do you want me to send patch 3/3 again separately or can you pick it up
> from this series?
> 

I applied it.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 17:08 [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620 Thierry Reding
2020-04-17 17:08 ` [PATCH 2/3] rtc: max77686: Make wakeup support configurable Thierry Reding
2020-04-20 14:42   ` Jon Hunter
2020-04-17 17:08 ` [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620 Thierry Reding
2020-04-20 14:43   ` Jon Hunter
2020-05-11 14:27   ` Alexandre Belloni
2020-04-30 14:07 ` [PATCH 1/3] dt-bindings: mfd: Document the RTC present " Rob Herring
2020-04-30 14:15   ` Alexandre Belloni
2020-05-01 13:00     ` Rob Herring
2020-05-01 13:53       ` Alexandre Belloni
2020-05-08 11:02         ` Thierry Reding
2020-05-11 14:28           ` Alexandre Belloni

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git