Re: [PATCHv2] rtc: cpcap: new rtc driver

[Alexandre Belloni]

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