[PATCHv1 6/6] rtc: rtc-isl12057: add alarm support to Intersil ISL12057 RTC driver

[uwe@kleine-koenig.org (Uwe Kleine-König)]

Hello,

finally I managed to test this series on my (unmodified) rn104.

For patch 1: Maybe point out that the issue with the century bit isn't
that critical, because this bit is not expected to be set before year 2100.

For patch 3: This patch adds a few dev_err calls that get later amended
in patch 5 to also include an error code. IMHO these should already be
added in patch 3. Patch 5 should only add it to the already existing
strings (if applicable).

For patch 4: Maybe
s/obsolete/for backwards compatibility, don't use in new code/.

Some further comments inline ...

On 11/15/2014 12:07 AM, Arnaud Ebalard wrote:
> The latter is the one found on current 3 kernel users of the chip
> for which support was initially developed (Netgear ReadyNAS 102,
> 104 and 2120 NAS). On those devices, the IRQ#2 pin of the chip is not
> connected to the SoC but to a PMIC. This allows setting an alarm,
> powering off the device and have it wake up when the alarm rings.
> To support that configuration the driver does the following:
> 
>  1. it has alarm_irq_enable() function returns -ENOTTY when no IRQ
>     is passed to the driver.
>  2. it marks the device as a wakeup source in all cases (whether an
>     IRQ is passed to the driver or not) to have 'wakealarm' sysfs
>     entry created.
This is not pretty, but I don't know if there is a nicer alternative.
Maybe this should only be done in the presence of a flag in the device
tree (say: can-wakeup-machine, a prefix would be nice, but "isil," feels
wrong).

> [...]
> FWIW, if someone is looking for a way to test alarm support on a system
> on which the chip IRQ line has the ability to boot the system (e.g.
> ReadyNAS 102, 104, etc):
> 
>     # echo 0 > /sys/class/rtc/rtc0/wakealarm
Why is this needed?

>     # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
>     # shutdown -h now
> 
> With the commandes above, after a minute, the system comes back to life.
s/commandes/commands/

> +	ret = regmap_update_bits(data->regmap, ISL12057_REG_INT,
> +				 ISL12057_REG_INT_A1IE,
> +				 enable ? ISL12057_REG_INT_A1IE : 0);
> +	if (ret)
> +		dev_err(dev, "%s: changing alarm interrupt flag failed (%d)\n",
> +			__func__, ret);
> +
> +	return ret;
> +}
Maybe point out in the commit log that the first alarm (of two) is used,
and the event is signaled on pin $Ididntlookitup.
(IIRC the 2nd alarm register set doesn't support seconds, but in return
has year and month field.)

> [...]
> +	/*
> +	 * This is needed to have 'wakealarm' sysfs entry available. One
> +	 * would expect the device to be marked as a wakeup source only
> +	 * when an IRQ pin of the RTC is routed to an interrupt line of the
> +	 * CPU. In practice, such an IRQ pin can be connected to a PMIC and
> +	 * this allows the device to be powered up when RTC alarm rings.
Maybe add the machines you know of that have this setup to the comment.

> +	 */
> +	device_init_wakeup(dev, true);
> +
> +	data->rtc = devm_rtc_device_register(dev, DRV_NAME, &rtc_ops,
> +					     THIS_MODULE);
> +	ret = PTR_ERR_OR_ZERO(data->rtc);
> +	if (ret) {
> +		dev_err(dev, "%s: unable to register RTC device (%d)\n",
> +			__func__, ret);
> +		goto err;
> +	}
> +
> +	/* We cannot support UIE mode if we do not have an IRQ line */
> +	if (!data->irq)
> +		data->rtc->uie_unsupported = 1;
> +
> +err:
> +	return ret;
>  }
>  
> +static int isl12057_remove(struct i2c_client *client)
> +{
> +	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(&client->dev);
> +
> +	if (rtc_data->irq > 0)
> +		device_init_wakeup(&client->dev, false);
I didn't check, but I wonder it that could be/is done by the device
core already?

> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int isl12057_rtc_suspend(struct device *dev)
> +{
> +	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		return enable_irq_wake(rtc_data->irq);
Does this need a check for data->irq?

> +
> +	return 0;
> +}
> +
> +static int isl12057_rtc_resume(struct device *dev)
> +{
> +	struct isl12057_rtc_data *rtc_data = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		return disable_irq_wake(rtc_data->irq);
ditto.

Thanks for your efforts to improve my NAS :-)
Uwe