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

[Alexandre Belloni]

On 22/02/2017 at 02:56:34 +0100, Sebastian Reichel wrote:
> > 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.
> 

That's fine, I was just curious :)

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

OK, I'll take it for 4.12 then

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

OK, my plan was to remove all the RTC_UF users. I'll give it more
thoughts.

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

Then it is fine where it is.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com