Hello,
On 03/10/2018 15:31:54+0200, Joel Stanley wrote:
> +static int aspeed_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct aspeed_rtc *rtc = dev_get_drvdata(dev);
> + unsigned int cent, year, mon, day, hour, min, sec;
> + unsigned long flags;
> + u32 reg1, reg2;
> +
> + spin_lock_irqsave(&rtc->lock, flags);
> +
> + do {
> + reg2 = readl(rtc->base + RTC_YEAR);
> + reg1 = readl(rtc->base + RTC_TIME);
> + } while (reg2 != readl(rtc->base + RTC_YEAR));
> +
> + day = (reg1 >> 24) & 0x1f;
> + hour = (reg1 >> 16) & 0x1f;
> + min = (reg1 >> 8) & 0x3f;
> + sec = (reg1 >> 0) & 0x3f;
> + cent = (reg2 >> 16) & 0x1f;
> + year = (reg2 >> 8) & 0x7f;
> + /*
> + * Month is 1-12 in hardware, and 0-11 in struct rtc_time, however we
> + * are using mktime64 which is 1-12, so no adjustment is necessary
> + */
> + mon = (reg2 >> 0) & 0x0f;
> +
> + rtc_time64_to_tm(mktime64(cent * 100 + year, mon, day, hour, min, sec),
> + tm);
> +
This is quite wasteful. You already have the broken out time. Why don't
you directly fill the tm struct?
> +static int aspeed_rtc_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct aspeed_rtc *rtc;
> +
> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> + if (!rtc)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + rtc->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(rtc->base))
> + return PTR_ERR(rtc->base);
> +
> + platform_set_drvdata(pdev, rtc);
> +
> + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
> + &aspeed_rtc_ops, THIS_MODULE);
> +
Please use devm_rtc_allocate_device to allocate the rtc and then
register it with rtc_register_device. Please also fill
rtc->range_{min,max} before the registration.
> + if (IS_ERR(rtc->rtc_dev))
> + return PTR_ERR(rtc->rtc_dev);
> +
> + spin_lock_init(&rtc->lock);
> +
> + /* Enable RTC and clear the unlock bit */
> + writel(RTC_ENABLE, rtc->base + RTC_CTRL);
> +
Maybe this should only be done in set_time so you can know whether the
time that is read in read_time has a chance to be valid.
For example you could return -EINVAL when RTC_ENABLE is not set if this
bit is readable.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
