Re: [PATCH] RTC:Add RTC driver for Gemini SoC

[Alexandre Belloni <alexandre.belloni@free-electrons.com>]

Hi,

Please also send to the linux-rtc mailing list:
rtc-linux@googlegroups.com

On 07/05/2015 at 19:32:54 +0200, Hans Ulli Kroll wrote :

Please always include a commit log.

> Signed-off-by: Hans Ulli Kroll <ulli.kroll@googlemail.com>
> ---

[...]

> +static int gemini_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct gemini_rtc *rtc = dev_get_drvdata(dev);
> +
> +	unsigned int  days, hour, min, sec;
> +	unsigned long offset, time;
> +
> +	sec  = ioread32(rtc->rtc_base + GEMINI_RTC_SECOND);
> +	min  = ioread32(rtc->rtc_base + GEMINI_RTC_MINUTE);
> +	hour = ioread32(rtc->rtc_base + GEMINI_RTC_HOUR);
> +	days = ioread32(rtc->rtc_base + GEMINI_RTC_DAYS);
> +	offset = ioread32(rtc->rtc_base + GEMINI_RTC_RECORD);

Can you use readl() here?

> +
> +	time = offset + days * 86400 + hour * 3600 + min * 60 + sec;
> +
> +	rtc_time_to_tm(time, tm);
> +
> +	return 0;
> +}
> +
> +static int gemini_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct gemini_rtc *rtc = dev_get_drvdata(dev);
> +	unsigned int sec, min, hour, day;
> +	unsigned long offset, time;
> +
> +	if (tm->tm_year >= 2148)	/* EPOCH Year + 179 */
> +		return -EINVAL;
> +
> +	rtc_tm_to_time(tm, &time);
> +
> +	sec = ioread32(rtc->rtc_base + GEMINI_RTC_SECOND);
> +	min = ioread32(rtc->rtc_base + GEMINI_RTC_MINUTE);
> +	hour = ioread32(rtc->rtc_base + GEMINI_RTC_HOUR);
> +	day = ioread32(rtc->rtc_base + GEMINI_RTC_DAYS);
> +

ditto.

> +	offset = time - (day*86400 + hour*3600 + min*60 + sec);
> +

If you use spaces around the * in read_time, please do the same here.

> +	iowrite32(offset, rtc->rtc_base + GEMINI_RTC_RECORD);
> +	iowrite32(0x01, rtc->rtc_base + GEMINI_RTC_CR);

writel() ?

> +
> +	return 0;
> +}
> +
> +static struct rtc_class_ops gemini_rtc_ops = {
> +	.read_time     = gemini_rtc_read_time,
> +	.set_time      = gemini_rtc_set_time,
> +};
> +
> +static int gemini_rtc_probe(struct platform_device *pdev)
> +{
> +	struct gemini_rtc *rtc;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +
> +	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
> +	if (unlikely(!rtc))
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, rtc);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!res) {
> +		ret = -ENODEV;
> +		goto err_mem;
> +	}
> +	rtc->rtc_irq = res->start;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		ret = -ENODEV;
> +		goto err_mem;
> +	}
> +	rtc->rtc_base =	devm_ioremap(&pdev->dev, res->start,
> +				res->end - res->start + 1);
> +
> +	ret = request_irq(rtc->rtc_irq, gemini_rtc_interrupt,
> +				IRQF_SHARED, pdev->name, dev);
> +	if (unlikely(ret))
> +		goto err_mem;
> +
> +	rtc->rtc_dev =	rtc_device_register(pdev->name, dev,
> +				&gemini_rtc_ops, THIS_MODULE);

For those 3 functions, the alignment of the wrapped function parameters
should match the open parenthesis.

[...]

> +static int __init gemini_rtc_init(void)
> +{
> +	return platform_driver_register(&gemini_rtc_driver);
> +}
> +
> +static void __exit gemini_rtc_exit(void)
> +{
> +	platform_driver_unregister(&gemini_rtc_driver);
> +}
> +
> +module_init(gemini_rtc_init);
> +module_exit(gemini_rtc_exit);

I'd remove those and use module_platform_driver()

Thanks!

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com