On Wed, Jan 27, 2010 at 5:07 AM, Anatolij Gustschin <ag...@denx.de> wrote:
> From: John Rigby <jcri...@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 <jcri...@gmail.com>
> Signed-off-by: Piotr Ziecik <ko...@semihalf.com>
> Signed-off-by: Wolfgang Denk <w...@denx.de>
> Signed-off-by: Anatolij Gustschin <ag...@denx.de>
> Cc: <rtc-li...@googlegroups.com>
> Cc: Grant Likely <grant.lik...@secretlab.ca>
> Cc: John Rigby <jcri...@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(®s->alm_enable, 0);
> + out_8(®s->int_enable, in_8(®s->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 <jcri...@gmail.com>");
> --
> 1.5.6.3
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
