[PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../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

[PATCH 2/3] rtc: max77686: Make wakeup support configurable

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
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

Re: [PATCH 2/3] rtc: max77686: Make wakeup support configurable

[Jon Hunter]

On 17/04/2020 18:08, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Tested-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620

[Thierry Reding]

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

Re: [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620

[Jon Hunter]

On 17/04/2020 18:08, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Tested-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

Re: [PATCH 3/3] rtc: max77686: Use single-byte writes on MAX77620

[Alexandre Belloni]

On 17/04/2020 19:08:25+0200, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  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

Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Rob Herring]

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

Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Alexandre Belloni]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../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

Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Rob Herring]

On Thu, Apr 30, 2020 at 9:15 AM Alexandre Belloni
<alexandre.belloni-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> > > 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  .../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

Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Alexandre Belloni]

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

Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Thierry Reding]

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

Re: [PATCH 1/3] dt-bindings: mfd: Document the RTC present on MAX77620

[Alexandre Belloni]

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