On 26/04/2019 19:37:15+0000, Howey, Dylan wrote:
> +static int pcf2123_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> + struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> + u8 rxbuf[4];
> + int ret;
> + unsigned int val = 0;
> +
> + ret = regmap_bulk_read(pdata->map, PCF2123_REG_ALRM_MN, rxbuf, 4);
sizeof(rxbuf) instead of 4.
> + if (ret)
> + return ret;
> +
> + alm->time.tm_min = bcd2bin(rxbuf[0] & 0x7F);
> + alm->time.tm_hour = bcd2bin(rxbuf[1] & 0x3F);
> + alm->time.tm_mday = bcd2bin(rxbuf[2] & 0x3F);
> + alm->time.tm_wday = bcd2bin(rxbuf[3] & 0x07);
> +
> + dev_dbg(dev, "%s: alm is secs=%d, mins=%d, hours=%d mday=%d, mon=%d, year=%d, wday=%d\n",
> + __func__,
> + alm->time.tm_sec, alm->time.tm_min, alm->time.tm_hour,
> + alm->time.tm_mday, alm->time.tm_mon, alm->time.tm_year,
> + alm->time.tm_wday);
> +
Please use %ptR.
> + ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> + if (ret)
> + return ret;
> +
> + alm->enabled = !!(val & CTRL2_AIE);
> +
> + return 0;
> +}
> +
> +static int pcf2123_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> + struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> + u8 txbuf[4];
> + int ret;
> + unsigned int val = 0;
> +
> + dev_dbg(dev, "%s: alm is secs=%d, mins=%d, hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
> + __func__,
> + alm->time.tm_sec, alm->time.tm_min, alm->time.tm_hour,
> + alm->time.tm_mday, alm->time.tm_mon, alm->time.tm_year,
> + alm->time.tm_wday);
> +
Ditto
> + /* Disable alarm */
> + ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> + if (ret)
> + return ret;
> +
> + val &= ~(CTRL2_AIE);
> + ret = regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
> +
Please use regmap_update_bits
> + /* Set new alarm */
> + txbuf[0] = bin2bcd(alm->time.tm_min & 0x7F);
> + txbuf[1] = bin2bcd(alm->time.tm_hour & 0x3F);
> + txbuf[2] = bin2bcd(alm->time.tm_mday & 0x3F);
> + txbuf[3] = bin2bcd(alm->time.tm_wday & 0x07);
> +
> + ret = regmap_bulk_write(pdata->map, PCF2123_REG_ALRM_MN, txbuf, 4);
sizeof(txbuf)
> + if (ret)
> + return ret;
> +
> + if (alm->enabled) {
> + ret = regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> + if (ret)
> + return ret;
> +
> + val |= CTRL2_AIE;
> + ret = regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
> + if (ret)
> + return ret;
Use regmap_update_bits
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t pcf2123_rtc_irq(int irq, void *dev)
> +{
> + struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> + struct mutex *lock = &pdata->rtc->ops_lock;
> + unsigned int val = 0;
> + int ret = IRQ_NONE;
> +
> + mutex_lock(lock);
> + regmap_read(pdata->map, PCF2123_REG_CTRL2, &val);
> +
> + /* Alarm? */
> + if (val & CTRL2_AF) {
> + ret = IRQ_HANDLED;
> +
> + val &= ~(CTRL2_AF);
> + regmap_write(pdata->map, PCF2123_REG_CTRL2, val);
> +
> + rtc_update_irq(pdata->rtc, 1, RTC_IRQF | RTC_AF);
> + }
> +
> + mutex_unlock(lock);
> +
> + return ret;
> +}
> +
> static int pcf2123_reset(struct device *dev)
> {
> struct pcf2123_plat_data *pdata = dev_get_platdata(dev);
> @@ -347,6 +448,8 @@ static const struct rtc_class_ops pcf2123_rtc_ops = {
> .set_time = pcf2123_rtc_set_time,
> .read_offset = pcf2123_read_offset,
> .set_offset = pcf2123_set_offset,
> + .read_alarm = pcf2123_rtc_read_alarm,
> + .set_alarm = pcf2123_rtc_set_alarm,
Please keep that struct aligned
>
> };
>
> @@ -391,6 +494,8 @@ static int pcf2123_probe(struct spi_device *spi)
> dev_info(&spi->dev, "spiclk %u KHz.\n",
> (spi->max_speed_hz + 500) / 1000);
>
> + device_init_wakeup(&spi->dev, true);
> +
This should be done only when you have an irq and the irq handler was
properly registered.
> /* Finalize the initialization */
> rtc = devm_rtc_device_register(&spi->dev, pcf2123_driver.driver.name,
> &pcf2123_rtc_ops, THIS_MODULE);
> @@ -418,6 +523,16 @@ static int pcf2123_probe(struct spi_device *spi)
> }
> }
>
> + /* Register alarm irq */
> + if (spi->irq >= 0) {
> + ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
> + pcf2123_rtc_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + pcf2123_driver.driver.name, &spi->dev);
> + if (ret)
> + dev_err(&spi->dev, "could not request irq.\n");
> + }
> +
As this RTC only has a minute granularity, you must set uie_unsupported
to get the UIE emulation working. You should probably run rtctest, this
would have uncovered the issue.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
