Hi,
The subject doesn't fit the subsystem style, this needs to be changed.
On 28/06/2017 at 17:46:55 +0200, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <aleksandar.markovic@xxxxxxxxxx>
>
> Add device driver for a virtual Goldfish RTC clock.
>
> The driver can be built only if CONFIG_MIPS and CONFIG_GOLDFISH are
> set. The compatible string used by OS for binding the driver is
> defined as "google,goldfish-rtc".
>
Is it really MIPS specific? I would expect the same driver to work on
the ARM based emulator too.
> +config RTC_DRV_GOLDFISH
> + tristate "Goldfish Real Time Clock"
> + depends on MIPS
> + depends on GOLDFISH
This should be made buildable with COMPILE_TEST to extend coverage.
> + help
> + Say yes here to build support for the Goldfish RTC.
Please, don't expect anybody to actually know what is goldfish can you
add a sentence or two?
> +static irqreturn_t goldfish_rtc_interrupt(int irq, void *dev_id)
> +{
> + struct goldfish_rtc *qrtc = dev_id;
> + unsigned long events = 0;
> + void __iomem *base = qrtc->base;
> +
> + writel(1, base + TIMER_CLEAR_INTERRUPT);
> + events = RTC_IRQF | RTC_AF;
> +
> + rtc_update_irq(qrtc->rtc, 1, events);
I'd say that events is not needed you can pass the flags directly to
rtc_update_irq
> +static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> + u64 time;
> + u64 time_low;
> + u64 time_high;
> + u64 time_high_prev;
> +
> + struct goldfish_rtc *qrtc =
> + platform_get_drvdata(to_platform_device(dev));
> + void __iomem *base = qrtc->base;
> +
> + time_high = readl(base + TIMER_TIME_HIGH);
> + do {
> + time_high_prev = time_high;
> + time_low = readl(base + TIMER_TIME_LOW);
> + time_high = readl(base + TIMER_TIME_HIGH);
> + } while (time_high != time_high_prev);
I'm not sure why you need that loop as the comments for TIMER_TIME_LOW
and TIMER_TIME_HIGH indicate that TIMER_TIME_HIGH is latched when
TIMER_TIME_LOW is read. Note that the original driver from google
doesn't do that.
> + time = (time_high << 32) | time_low;
> +
> + do_div(time, NSEC_PER_SEC);
> +
> + rtc_time_to_tm(time, tm);
> +
> + return 0;
> +}
> +
> +static const struct rtc_class_ops goldfish_rtc_ops = {
> + .read_time = goldfish_rtc_read_time,
> +};
> +
> +static int goldfish_rtc_probe(struct platform_device *pdev)
> +{
> + struct resource *r;
> + struct goldfish_rtc *qrtc;
> + unsigned long rtc_dev_len;
> + unsigned long rtc_dev_addr;
> + int err;
> +
> + qrtc = devm_kzalloc(&pdev->dev, sizeof(*qrtc), GFP_KERNEL);
> + if (qrtc == NULL)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, qrtc);
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (r == NULL)
> + return -ENODEV;
> +
> + rtc_dev_addr = r->start;
> + rtc_dev_len = resource_size(r);
> + qrtc->base = devm_ioremap(&pdev->dev, rtc_dev_addr, rtc_dev_len);
devm_ioremap_resource ?
> + if (IS_ERR(qrtc->base))
> + return -ENODEV;
> +
> + qrtc->irq = platform_get_irq(pdev, 0);
> + if (qrtc->irq < 0)
> + return -ENODEV;
> +
Is the irq so important that you have to fail here even if the driver
doesn't support any alarm?
> + qrtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> + &goldfish_rtc_ops, THIS_MODULE);
> + if (IS_ERR(qrtc->rtc))
> + return PTR_ERR(qrtc->rtc);
> +
> + err = devm_request_irq(&pdev->dev, qrtc->irq, goldfish_rtc_interrupt,
> + 0, pdev->name, qrtc);
> + if (err)
> + return err;
Ditto.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
