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

[Sebastian Reichel]

Hi Alexandre,

On Wed, Feb 22, 2017 at 12:52:12AM +0100, Alexandre Belloni wrote:
> The patch has a few checkpatch issues. Some of those should really be
> fixed. Can you do that?

of course.

> 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?

The logic (incl. comments) in this section are from the vendor
kernel driver and there is no documentation for CPCAP as far as
I know. I don't know if the hardware has logic to prevent a race
condition for the cpcap_tm.tod1 == 255 case.

> > +		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.

Yes, but cpcap_get_vendor should get into mainline with the
4.11 mfd pull request. So if you base your 4.12 for-next tree
on 4.11-rc1 everything should be fine.

> > +	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.

Yes. RTC works perfectly fine with just the alarm irq. It also
works perfectly fine with just the 1 Hz irq (except for wakeup).

I would like to keep the irq in the driver, so that it's explicitly
disabled. On Droid 4 mainline kernel is booted via kexec from
Android (AKA bootloader) and Motorola's Android kernel uses the
1 Hz IRQ for some proprietary "secure clock daemon".

I will add a comment.

> > +	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.

I successfully tested wakeup before sending this. But in case your 
prefer it to be called before registering the RTC I can move the
call accordingly.

-- Sebastian

Attachment: signature.asc
Description: PGP signature