RE: [NEW DRIVER V2 4/7] DA9058 RTC driver

[Venu Byravarasu]

> -----Original Message-----
> From: linux-kernel-owner@xxxxxxxxxxxxxxx [mailto:linux-kernel-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Anthony Olech
> Sent: Monday, August 06, 2012 2:14 AM
> To: Andrew Morton
> Cc: Mark Brown; Paul Gortmaker; Samuel Ortiz; Alessandro Zummo; rtc-
> linux@xxxxxxxxxxxxxxxx; LKML; David Dajun Chen
> Subject: [NEW DRIVER V2 4/7] DA9058 RTC driver
> 
> This is the RTC component driver of the Dialog DA9058 PMIC.
> This driver is just one component of the whole DA9058 PMIC driver.
> It depends on the core DA9058 MFD driver.
> 
> Signed-off-by: Anthony Olech <anthony.olech.opensource@xxxxxxxxxxx>
> Signed-off-by: David Dajun Chen <david.chen@xxxxxxxxxxx>
> ---
> +
> +static int da9058_rtc_check_param(struct rtc_time *rtc_tm)
> +{
> +	if ((rtc_tm->tm_sec > DA9058_RTC_SECONDS_LIMIT) || (rtc_tm-
> >tm_sec < 0))
> +		return -EIO;
> +
> +	if ((rtc_tm->tm_min > DA9058_RTC_MINUTES_LIMIT) || (rtc_tm-
> >tm_min < 0))
> +		return -EIO;
> +
> +	if ((rtc_tm->tm_hour > DA9058_RTC_HOURS_LIMIT) || (rtc_tm-
> >tm_hour < 0))
> +		return -EIO;
> +
> +	if (rtc_tm->tm_mday == 0)
> +		return -EIO;

Why limit check is not done for mday, like it is done for other params? 

> +
> +	if ((rtc_tm->tm_mon > DA9058_RTC_MONTHS_LIMIT) || (rtc_tm-
> >tm_mon <= 0))
> +		return -EIO;
> +
> +	if ((rtc_tm->tm_year > DA9058_RTC_YEARS_LIMIT) || (rtc_tm-
> >tm_year < 0))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int da9058_rtc_readtime(struct device *dev, struct rtc_time *tm)
> +{
> +	struct da9058_rtc *rtc = dev_get_drvdata(dev);
> +	struct da9058 *da9058 = rtc->da9058;
> +	unsigned int rtc_time[6];
> +	int ret;
> +
> +	ret = da9058_bulk_read(da9058, DA9058_COUNTS_REG, rtc_time, 6);
> +	if (ret)
> +		return ret;
> +
> +	tm->tm_sec = rtc_time[0] & DA9058_RTC_SECS_MASK;

Usually most of the PMIC RTCs provide RTC time registers in BCD
Format. Plz make sure that is not the case with your device, 
otherwise these masks may not be valid.

> +	tm->tm_min = rtc_time[1] & DA9058_RTC_MINS_MASK;
> +
> +	tm->tm_hour = rtc_time[2] & DA9058_RTC_HRS_MASK;

 

> +
> +	tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon,
> +						tm->tm_year);
> +	tm->tm_year += 100;
> +	tm->tm_mon -= 1;
> +
> +	return 0;
> +}
> +
> +static int da9058_rtc_settime(struct device *dev, struct rtc_time *tm)
> +{
> +	struct da9058_rtc *rtc = dev_get_drvdata(dev);
> +	struct da9058 *da9058 = rtc->da9058;
> +	unsigned int rtc_ctrl, val, rtc_time[6];
> +	int ret;
> +
> +	tm->tm_year -= 111;

Why 111 is subtracted here and only 100 is added in read_time()
above? If this is what you really wanted to have, it would be
better to add few comments justifying these magic numbers. 

> +	tm->tm_mon += 1;


> +static irqreturn_t da9058_rtc_timer_alarm_handler(int irq, void *data)
> +{
> +	struct da9058_rtc *rtc = data;
> +
> +	da9058_rtc_stop_alarm(rtc);
> +	rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> +

Whether start RTC is not needed here explicitly?

> +	return IRQ_HANDLED;
> +}
> +


> +	rtc->tick_irq = DA9058_IRQ_ETICK;
> +	ret = request_threaded_irq(da9058_to_virt_irq_num(da9058,
> +							rtc->tick_irq),
> +				NULL, da9058_rtc_tick_alarm_handler,
> +				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +				"DA9058 RTC Tick Alarm", rtc);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Failed to get rtc timer alarm IRQ %d: %d\n",
> +			DA9058_IRQ_ETICK, ret);
> +		goto err4;
> +	}
> +
> +	ret = 0;
> +	goto exit;

You can have return 0 here, instead of above two statements.
 
> +
> +err4:
> +	free_irq(da9058_to_virt_irq_num(da9058, rtc->alarm_irq), rtc);
> +err3:
> +err2:
> +	rtc_device_unregister(rtc->rtc_dev);
> +err1:
> +err0:
> +	platform_set_drvdata(pdev, NULL);
> +exit:
> +	return ret;
> +}
> +
> +static int __devexit da9058_rtc_remove(struct platform_device *pdev)
> +{
> +	struct da9058_rtc *rtc = platform_get_drvdata(pdev);
> +	struct da9058 *da9058 = rtc->da9058;
> +
> +	free_irq(da9058_to_virt_irq_num(da9058, rtc->alarm_irq), rtc);
> +	free_irq(da9058_to_virt_irq_num(da9058, rtc->tick_irq), rtc);
> +
> +	rtc_device_unregister(rtc->rtc_dev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver da9058_rtc_driver = {
> +	.probe = da9058_rtc_probe,
> +	.remove = __devexit_p(da9058_rtc_remove),
> +	.driver = {
> +		.name = "da9058-rtc",

Some of the rtc drivers had name as rtc-deviceName and others have deviceName-rtc.
Can some experts comment which one is correct practice?


> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(da9058_rtc_driver);
> +
> +MODULE_DESCRIPTION("Dialog DA9058 PMIC Real Time Clock Driver");
> +MODULE_AUTHOR("Anthony Olech <Anthony.Olech@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:da9058-rtc");
> --
> end-of-patch for NEW DRIVER V2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/