On 17/05/2022 22:09, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> 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?
It's a good thing you asked - I went to double check and these reads are
backwards. /facepalm
A read from the upper register will always be aligned with the last read
of the lower register. Stupid oversight, sorry.
>
>> + 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 ;)
SGTM, including not being around in year 141338...
Thanks,
Conor.
>
>> +
>> + 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
