Re: [PATCH RESENT] [RTC] Add Freescale MC13783 RTC driver

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

Hi Alessandro,

On Tue, Nov 10, 2009 at 12:10:01PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 10, 2009 at 10:30:54AM +0100, Alessandro Zummo wrote:
> > On Tue, 10 Nov 2009 09:32:47 +0100
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > 
> > > +	ret = mc13783_irq_request_nounmask(priv->mc13783, MC13783_IRQ_1HZ,
> > > +			mc13783_rtc_update_handler, DRIVER_NAME, priv);
> > > +	if (ret)
> > > +		goto err_update_irq_request;
> > > +
> > > +	mc13783_unlock(priv->mc13783);
> > > +
> > > +	priv->rtc = rtc_device_register(pdev->name,
> > > +			&pdev->dev, &mc13783_rtc_ops, THIS_MODULE);
> > > +
> > 
> >  isn't better to enable irqs after registration.
> IMHO it doesn't make a difference for the 1HZ irq, still more as it
> remains masked.  The reset irq should be requested before registration.
I thought again about the 1HZ irq.  There are two different things to
consider:

 - After rtc_device_register the .update_irq_enable callback could be
   called at once.  At that time the update irq should already be
   registered.  So register irq before rtc_device.
 - The update irq handler calls rtc_update_irq(struct rtc_device *,
   ...) so the rtc_device should already be complete.  That is, register
   before the irq can trigger.

To satisfy both you need to make sure the irq is registered before
registration but masked (either at controller level as in my patch or at
device level (which should be the usual thing but there is no knob in
the mc13783's rtc for that)).

Will you take my patch?

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |