Hello,
On 16/05/2022 09:28:38+0100, Conor Dooley wrote:
> +struct mpfs_rtc_dev {
> + struct rtc_device *rtc;
> + void __iomem *base;
> + int wakeup_irq;
I believe this is only used in .probe so you make it local to this
function.
> +};
> +
> +static int mpfs_rtc_readtime(struct device *dev, struct rtc_time *tm)
> +{
> + struct mpfs_rtc_dev *rtcdev = dev_get_drvdata(dev);
> + u64 time;
> +
> + time = ((u64)readl(rtcdev->base + DATETIME_UPPER_REG) & DATETIME_UPPER_MASK) << 32;
> + time |= readl(rtcdev->base + DATETIME_LOWER_REG);
Are the registers properly latched on a DATETIME_UPPER_REG read?
> + rtc_time64_to_tm(time + rtcdev->rtc->range_min, tm);
range_min is never set so it will end up being 0. I guess you can avoid
a bunch of arithmetic in all the driver. Offsetting will happen in the
core which will probably never happen anyway because the max year is
141338. I guess we will all be gone by then ;)
> +
> + return 0;
> +}
> +
> +static int mpfs_rtc_probe(struct platform_device *pdev)
> +{
> + struct mpfs_rtc_dev *rtcdev;
> + struct clk *clk;
> + u32 prescaler;
> + int ret;
> +
> + rtcdev = devm_kzalloc(&pdev->dev, sizeof(struct mpfs_rtc_dev), GFP_KERNEL);
> + if (!rtcdev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, rtcdev);
> +
> + rtcdev->rtc = devm_rtc_allocate_device(&pdev->dev);
> + if (IS_ERR(rtcdev->rtc))
> + return PTR_ERR(rtcdev->rtc);
> +
> + rtcdev->rtc->ops = &mpfs_rtc_ops;
> +
> + /* range is capped by alarm max, lower reg is 31:0 & upper is 10:0 */
> + rtcdev->rtc->range_max = GENMASK_ULL(42, 0);
> +
> + clk = mpfs_rtc_init_clk(&pdev->dev);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + rtcdev->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(rtcdev->base)) {
> + dev_dbg(&pdev->dev, "invalid ioremap resources\n");
> + return PTR_ERR(rtcdev->base);
> + }
> +
> + rtcdev->wakeup_irq = platform_get_irq(pdev, 0);
> + if (rtcdev->wakeup_irq <= 0) {
> + dev_dbg(&pdev->dev, "could not get wakeup irq\n");
> + return rtcdev->wakeup_irq;
> + }
> + ret = devm_request_irq(&pdev->dev, rtcdev->wakeup_irq, mpfs_rtc_wakeup_irq_handler, 0,
> + dev_name(&pdev->dev), rtcdev);
> + if (ret) {
> + dev_dbg(&pdev->dev, "could not request wakeup irq\n");
> + return ret;
> + }
> +
> + /* prescaler hardware adds 1 to reg value */
> + prescaler = clk_get_rate(devm_clk_get(&pdev->dev, "rtcref")) - 1;
> +
> + if (prescaler > MAX_PRESCALER_COUNT) {
> + dev_dbg(&pdev->dev, "invalid prescaler %d\n", prescaler);
> + return -EINVAL;
> + }
> +
> + writel(prescaler, rtcdev->base + PRESCALER_REG);
> + dev_info(&pdev->dev, "prescaler set to: 0x%X \r\n", prescaler);
> +
> + device_init_wakeup(&pdev->dev, true);
> + ret = dev_pm_set_wake_irq(&pdev->dev, rtcdev->wakeup_irq);
> + if (ret)
> + dev_err(&pdev->dev, "failed to enable irq wake\n");
> +
> + return devm_rtc_register_device(rtcdev->rtc);
> +}
> +
> +static int mpfs_rtc_remove(struct platform_device *pdev)
> +{
> + mpfs_rtc_alarm_irq_enable(&pdev->dev, 0);
This is not something you want to do if you want to wake up from
hibernate or any similar sleep state.
> + device_init_wakeup(&pdev->dev, 0);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id mpfs_rtc_of_match[] = {
> + { .compatible = "microchip,mpfs-rtc" },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(of, mpfs_rtc_of_match);
> +
> +static struct platform_driver mpfs_rtc_driver = {
> + .probe = mpfs_rtc_probe,
> + .remove = mpfs_rtc_remove,
> + .driver = {
> + .name = "mpfs_rtc",
> + .of_match_table = mpfs_rtc_of_match,
> + },
> +};
> +
> +module_platform_driver(mpfs_rtc_driver);
> +
> +MODULE_DESCRIPTION("Real time clock for Microchip Polarfire SoC");
> +MODULE_AUTHOR("Daire McNamara <daire.mcnamara@xxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Conor Dooley <conor.dooley@xxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.36.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
