Hi,
The patch has a few checkpatch issues. Some of those should really be
fixed. Can you do that?
Else, it is mostly fine, a few comments below.
On 21/02/2017 at 07:16:50 +0100, Sebastian Reichel wrote:
> +static int cpcap_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct cpcap_rtc *rtc;
> + struct cpcap_time cpcap_tm;
> + int ret = 0;
> +
> + rtc = dev_get_drvdata(dev);
> +
> + rtc2cpcap_time(&cpcap_tm, tm);
> +
> + if (rtc->alarm_enabled)
> + disable_irq(rtc->alarm_irq);
> + if (rtc->update_enabled)
> + disable_irq(rtc->update_irq);
> +
> + if (rtc->vendor == CPCAP_VENDOR_ST) {
> + /* The TOD1 and TOD2 registers MUST be written in this order
> + * for the change to properly set. */
Does this mean there is a race condition?
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> + TOD1_MASK, cpcap_tm.tod1);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2,
> + TOD2_MASK, cpcap_tm.tod2);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY,
> + DAY_MASK, cpcap_tm.day);
> + } else {
> + /* Clearing the upper lower 8 bits of the TOD guarantees that
> + * the upper half of TOD (TOD2) will not increment for 0xFF RTC
> + * ticks (255 seconds). During this time we can safely write
> + * to DAY, TOD2, then TOD1 (in that order) and expect RTC to be
> + * synchronized to the exact time requested upon the final write
> + * to TOD1. */
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> + TOD1_MASK, 0);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY,
> + DAY_MASK, cpcap_tm.day);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2,
> + TOD2_MASK, cpcap_tm.tod2);
> + ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> + TOD1_MASK, cpcap_tm.tod1);
> + }
> +
> + err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor);
I think this means it depends on the mfd tree.
> + if (err)
> + return err;
> +
> + rtc->alarm_irq= platform_get_irq(pdev, 0);
> + err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL,
> + cpcap_rtc_alarm_irq, IRQ_NONE,
> + "rtc_alarm", rtc);
> + if (err) {
> + dev_err(dev, "Could not request alarm irq: %d\n", err);
> + return err;
> + }
> + disable_irq(rtc->alarm_irq);
> +
> + rtc->update_irq= platform_get_irq(pdev, 1);
> + err = devm_request_threaded_irq(dev, rtc->update_irq, NULL,
> + cpcap_rtc_update_irq, IRQ_NONE,
> + "rtc_1hz", rtc);
I don't think this IRQ is actually useful. It doesn't really harm but
the tests should pass without it.
> + if (err) {
> + dev_err(dev, "Could not request update irq: %d\n", err);
> + return err;
> + }
> + disable_irq(rtc->update_irq);
> +
> + err = device_init_wakeup(dev, 1);
If you use device_init_wakeup, I think it needs to be called before
devm_rtc_device_register() to properly work.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
