[PATCH 2/8 v2] rtc: Add MPC5121 Real time clock driver

Grant Likely grant.likely at secretlab.ca
Thu Jan 28 02:58:51 EST 2010


On Wed, Jan 27, 2010 at 5:07 AM, Anatolij Gustschin <agust at denx.de> wrote:
> From: John Rigby <jcrigby at gmail.com>

This is your patch now.  You can claim authorship in the git commit record.

> Based on Domen Puncer's rtc driver for 5200.
> Changes to Domen's original:
>
>    Changed filenames/routine names from mpc5200* to mpc5121*
>    Changed match to only care about compatible and use "fsl,"
>    convention for compatible.
>
>    Make alarms more sane by dealing with lack of second alarm
>    resolution.
>
>    Deal with the fact that most of the 5121 rtc registers are not
>    persistent across a reset even with a battery attached:
>
>        Use actual_time register for time keeping
>        and target_time register as an offset to linux time
>
>        The target_time register would normally be used for hibernation
>        but hibernation does not work on current silicon

This is a description of what has been done, but it is not a
description of what the patch is.  Think about it this way, if someone
looks at your commit in git after it is merged, will the above text
make sense?

>
> Signed-off-by: John Rigby <jcrigby at gmail.com>
> Signed-off-by: Piotr Ziecik <kosmo at semihalf.com>
> Signed-off-by: Wolfgang Denk <wd at denx.de>
> Signed-off-by: Anatolij Gustschin <agust at denx.de>
> Cc: <rtc-linux at googlegroups.com>
> Cc: Grant Likely <grant.likely at secretlab.ca>
> Cc: John Rigby <jcrigby at gmail.com>
> ---
> Changes since v1 (as requested by Alessandro Zummo):
>  - Remove history from the driver file, the same history is in
>   commit message
>  - implement alarm/irq interface using ->ops structure, don't
>   use ops->ioctl() any more
>  - Clean up probe()
>  - replace printk() by dev_*()
>  - add arch dependency in Kconfig
>  - add requested include linux/init.h
>  - move MODULE_XXX to the end
>  - use rtc_valid_tm() when returning 'tm'
>  - use __init/__exit/__exit_p as this is not a hotpluggable device

Dangerous.  You're assuming that drivers won't be able to be
bound/rebound at runtime.  The kernel has no idea you've done this,
and it will cause a bug.  You should always use
__devinit/__devexit/__devexit_p for the probe/remove hooks in
of_platform drivers.

> +static int __init mpc5121_rtc_probe(struct of_device *op,
> +                                       const struct of_device_id *match)

__devinit

> +{
> +       struct mpc5121_rtc_data *rtc;
> +       int err = 0;
> +       u32 ka;
> +
> +       rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> +       if (!rtc)
> +               return -ENOMEM;
> +
> +       rtc->regs = of_iomap(op->node, 0);
> +       if (!rtc->regs) {
> +               dev_err(&op->dev, "%s: couldn't map io space\n", __func__);
> +               err = -ENOSYS;
> +               goto out_free;
> +       }
> +
> +       device_init_wakeup(&op->dev, 1);
> +
> +       rtc->rtc = rtc_device_register("mpc5121-rtc", &op->dev,
> +                                       &mpc5121_rtc_ops, THIS_MODULE);
> +       if (IS_ERR(rtc->rtc)) {
> +               err = PTR_ERR(rtc->rtc);
> +               goto out_unmap;
> +       }

I haven't dug into the rtc layer, but this looks racy.  The device is
getting registered before it is completely set up (irq not mapped,
keepalive register not initialized).  Is this correct?

> +
> +       dev_set_drvdata(&op->dev, rtc);
> +
> +       rtc->irq = irq_of_parse_and_map(op->node, 1);
> +       err = request_irq(rtc->irq, mpc5121_rtc_handler, IRQF_DISABLED,
> +                                               "mpc5121-rtc", &op->dev);
> +       if (err) {
> +               dev_err(&op->dev, "%s: could not request irq: %i\n",
> +                                                       __func__, rtc->irq);
> +               goto out_dispose;
> +       }
> +
> +       rtc->irq_periodic = irq_of_parse_and_map(op->node, 0);
> +       err = request_irq(rtc->irq_periodic, mpc5121_rtc_handler_upd,
> +                               IRQF_DISABLED, "mpc5121-rtc_upd", &op->dev);
> +       if (err) {
> +               dev_err(&op->dev, "%s: could not request irq: %i\n",
> +                                               __func__, rtc->irq_periodic);
> +               goto out_dispose2;
> +       }
> +
> +       ka = in_be32(&rtc->regs->keep_alive);
> +       if (ka & 0x02) {
> +               dev_warn(&op->dev,
> +                       "mpc5121-rtc: Battery or oscillator failure!\n");
> +               out_be32(&rtc->regs->keep_alive, ka);
> +       }
> +
> +       return 0;
> +
> +out_dispose2:
> +       irq_dispose_mapping(rtc->irq_periodic);
> +       free_irq(rtc->irq, &op->dev);
> +out_dispose:
> +       irq_dispose_mapping(rtc->irq);
> +out_unmap:
> +       iounmap(rtc->regs);
> +out_free:
> +       kfree(rtc);
> +
> +       return err;
> +}
> +
> +static int __exit mpc5121_rtc_remove(struct of_device *op)

__devexit

> +{
> +       struct mpc5121_rtc_data *rtc = dev_get_drvdata(&op->dev);
> +       struct mpc5121_rtc_regs __iomem *regs = rtc->regs;
> +
> +       /* disable interrupt, so there are no nasty surprises */
> +       out_8(&regs->alm_enable, 0);
> +       out_8(&regs->int_enable, in_8(&regs->int_enable) & ~0x1);
> +
> +       rtc_device_unregister(rtc->rtc);
> +       iounmap(rtc->regs);
> +       free_irq(rtc->irq, &op->dev);
> +       free_irq(rtc->irq_periodic, &op->dev);
> +       irq_dispose_mapping(rtc->irq);
> +       irq_dispose_mapping(rtc->irq_periodic);
> +       dev_set_drvdata(&op->dev, NULL);
> +       kfree(rtc);
> +
> +       return 0;
> +}
> +
> +static struct of_device_id mpc5121_rtc_match[] = {

Missing __devinitdata.  Should be like this:

static struct of_device_id mpc5121_rtc_match[] __devinitdata = {

> +       { .compatible = "fsl,mpc5121-rtc", },
> +       {},
> +};
> +
> +static struct of_platform_driver mpc5121_rtc_driver = {
> +       .owner = THIS_MODULE,
> +       .name = "mpc5121-rtc",
> +       .match_table = mpc5121_rtc_match,
> +       .probe = mpc5121_rtc_probe,
> +       .remove = __exit_p(mpc5121_rtc_remove),
> +};
> +
> +static int __init mpc5121_rtc_init(void)
> +{
> +       return of_register_platform_driver(&mpc5121_rtc_driver);
> +}
> +module_init(mpc5121_rtc_init);
> +
> +static void __exit mpc5121_rtc_exit(void)
> +{
> +       of_unregister_platform_driver(&mpc5121_rtc_driver);
> +}
> +module_exit(mpc5121_rtc_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("John Rigby <jcrigby at gmail.com>");
> --
> 1.5.6.3
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.


More information about the Linuxppc-dev mailing list