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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html