Re: [PATCH 2/2] rtc: rs5c372: let the alarm to be used as wakeup source

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 23/11/2020 11:38:48+0100, Daniel González Cabanelas wrote:
> Currently there is no use for the interrupts on the rs5c372 RTC and the
> wakealarm isn't enabled. There are some devices like NAS which use this
> RTC to wake up from the power off state when the INTR pin is activated by
> the alarm clock.
> 
> Enable the alarm and let to be used as a wakeup source. Tested on a Buffalo
> LS421DE NAS.
> 
> Signed-off-by: Daniel González Cabanelas <dgcbueu@gmail.com>
> ---
>  drivers/rtc/rtc-rs5c372.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
> index 94b778c6e..76775d66e 100644
> --- a/drivers/rtc/rtc-rs5c372.c
> +++ b/drivers/rtc/rtc-rs5c372.c
> @@ -654,6 +654,7 @@ static int rs5c372_probe(struct i2c_client *client,
>  	int err = 0;
>  	int smbus_mode = 0;
>  	struct rs5c372 *rs5c372;
> +	bool rs5c372_can_wakeup_device = false;
>  
>  	dev_dbg(&client->dev, "%s\n", __func__);
>  
> @@ -689,6 +690,12 @@ static int rs5c372_probe(struct i2c_client *client,
>  	else
>  		rs5c372->type = id->driver_data;
>  
> +#ifdef CONFIG_OF

I don't think you need to protect this section

> +	if(of_property_read_bool(client->dev.of_node,

but you'll have to check client->dev.of_node is not null.

> +					      "wakeup-source"))
> +		rs5c372_can_wakeup_device = true;
> +#endif
> +
>  	/* we read registers 0x0f then 0x00-0x0f; skip the first one */
>  	rs5c372->regs = &rs5c372->buf[1];
>  	rs5c372->smbus = smbus_mode;
> @@ -722,6 +729,8 @@ static int rs5c372_probe(struct i2c_client *client,
>  		goto exit;
>  	}
>  
> +	rs5c372->has_irq = 1;
> +
This is an unrelated change but is this actually useful? I guess you
should simply remove has_irq.

>  	/* if the oscillator lost power and no other software (like
>  	 * the bootloader) set it up, do it here.
>  	 *
> @@ -748,6 +757,10 @@ static int rs5c372_probe(struct i2c_client *client,
>  			);
>  
>  	/* REVISIT use client->irq to register alarm irq ... */
> +	if (rs5c372_can_wakeup_device) {
> +		device_init_wakeup(&client->dev, true);
> +	}
> +
>  	rs5c372->rtc = devm_rtc_device_register(&client->dev,
>  					rs5c372_driver.driver.name,
>  					&rs5c372_rtc_ops, THIS_MODULE);
> @@ -761,6 +774,9 @@ static int rs5c372_probe(struct i2c_client *client,
>  	if (err)
>  		goto exit;
>  
> +	/* the rs5c372 alarm only supports a minute accuracy */
> +	rs5c372->rtc->uie_unsupported = 1;
> +

Honestly, the whole driver would need some modernization so if you have
time to test, I can propose a set of patches.


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