Hello Arnaud,
On 12/16/2014 12:54 AM, Arnaud Ebalard wrote:
>
> This patch adds alarm support to Intersil ISL12057 driver. The chip
> supports two separate alarms:
>
> - Alarm1: works up to one month in the future and accurate to the
> second, associated w/ IRQ#2 pin
> - Alarm2: works up to one month in the future and accurate to the
> minute, associated w/ IRQ#1 or IRQ#2 pin (configuable).
>
> This patch only adds support for Alarm1 which allows to configure
> the chip to generate an interrupt on IRQ#2 pin when current time
> matches the alarm.
>
> This patch was tested on a Netgear ReadyNAS 102 after some soldering
> of the IRQ#2 pin of the RTC chip to a MPP line of the SoC (the one
> used usually handles the reset button). The test was performed using
> a modified .dts file reflecting this change (see below) and rtc-test.c
> program available in Documentation/rtc.txt. This test program ran as
> expected, which validates alarm supports, including interrupt support.
>
> As a side note, the ISL12057 remains in the list of trivial devices,
> i.e. no specific DT binding being added by this patch: i2c core
> automatically handles extraction of IRQ line info from .dts file. For
> instance, if one wants to reference the interrupt line for the
> alarm in its .dts file, adding interrupt and interrupt-parent
> properties works as expected (if the primary function of your
> interrupt pin is not GPIO, you will need some additional pinctrl
> properties):
>
> isl12057: isl12057@68 {
> compatible = "isil,isl12057";
> interrupt-parent = <&gpio0>;
> interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
> reg = <0x68>;
> };
>
> FWIW, if someone is looking for a way to test alarm support this can
> be done in the following way:
>
> # echo `date '+%s' -d '+ 1 minutes'` > /sys/class/rtc/rtc0/wakealarm
> # shutdown -h now
>
> With the commands above, after a minute, the system comes back to life.
This paragraph belongs into the 2nd patch, right?
> diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
> index 6e1fcfb5d7e6..3ec73ad7f2d8 100644
> --- a/drivers/rtc/rtc-isl12057.c
> +++ b/drivers/rtc/rtc-isl12057.c
> @@ -79,8 +79,10 @@
> #define ISL12057_MEM_MAP_LEN 0x10
>
> struct isl12057_rtc_data {
> + struct rtc_device *rtc;
> struct regmap *regmap;
> struct mutex lock;
> + int irq;
interrupts are usually unsigned values. Hmm, I see that it simplifies a
bit here, as the function to determine the irq returns the actual value
or a negative error. As there would be problems anyhow for irq values >
INT_MAX probably this comment isn't that important.
> +/*
> + * Note: as we only read from device and do not perform any update, there is
> + * no need for an equivalent function which would try and get driver's main
> + * lock. Here, it is safe for everyone if we just use regmap internal lock
> + * on the device when reading.
> + */
> +static int _isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
> struct isl12057_rtc_data *data = dev_get_drvdata(dev);
> u8 regs[ISL12057_RTC_SEC_LEN];
> unsigned int sr;
> int ret;
>
> - mutex_lock(&data->lock);
> ret = regmap_read(data->regmap, ISL12057_REG_SR, &sr);
> if (ret) {
> dev_err(dev, "%s: unable to read oscillator status flag (%d)\n",
> @@ -187,8 +222,6 @@ static int isl12057_rtc_read_time(struct device *dev, struct rtc_time *tm)
> __func__, ret);
>
> out:
> - mutex_unlock(&data->lock);
> -
> if (ret)
> return ret;
>
Is this locking update worth a separate change?
Best regards
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
