Hi Uwe,
Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx> writes:
> 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.
It has:
This was tested by putting a device 100 years in the future (using a
specific kernel due to the inability of userland tools such as date or
hwclock to pass year 2038), rebooting on a kernel w/ this patch applied
and verifying the device was still 100 years in the future.
> 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).
will do that next time.
> 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).
I can prepare a v0 patch for a "can-wakeup-machine" property to mark the
device as a wakeup source when the IRQ is absent. Will fix the prefix
in a v1.
>> [...]
>> 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?
It disable the alarm. It's not required.
>> # 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/
useless french letter.
>> + 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.)
I can add that in the documentation.
>> [...]
>> + /*
>> + * 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.
ditto.
>> + */
>> + 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.
Hum, I will take a look at those irq vs wakeup cpabilities when adding
support for "can-wakeup-machine" property to consolidate the behavior.
Cheers,
a+
--
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
