On Tue, Jan 25, 2011 at 12:19:35PM +0530, Varun Wadekar wrote:
> drivers/rtc/rtc-tps6586x.c | 325
> ++++++++++++++++++++++++++++++++++++++++++
It looks like you still have the line wrapping issues.
> +config RTC_DRV_TPS6586X
> + tristate "TI TPS6586X RTC"
> + depends on I2C && MFD_TPS6586X
No need to depend on I2C, the MFD for the device deals with that for
you.
> +struct tps6586x_rtc {
> + unsigned long epoch_start;
> + int irq;
All the indentation in the driver is broken - you should be using tabs
for indentation. This may well be the same issue with your MUA that is
causing the line wrapping.
> +static int tps6586x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> +{
> + if (rtc->irq == -1)
> + return -EIO;
> + if (rtc->irq_en && rtc->irq_en && (rtc->irq != -1)) {
> + disable_irq(rtc->irq);
> + rtc->irq_en = false;
You're already checking rtc->irq further up, the check for rtc->irq_en
is duplicated. Probably just need to replace the if with a single check
for rtc->irq_en?
> + if (alrm->enabled && (rtc->irq != -1)) {
No need to check the IRQ here either.
> +static int tps6586x_rtc_update_irq_enable(struct device *dev,
> + unsigned int enabled)
> +{
> + struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
> +
> + if (rtc->irq == -1)
> + return -EIO;
> +
> + enabled = !!enabled;
> + if (enabled == rtc->irq_en)
> + return 0;
> +
> + if (enabled)
> + enable_irq(rtc->irq);
> + else
> + disable_irq(rtc->irq);
This looks to be the same IRQ as is used for alarms above - how do these
two bits of code play together? The IRQ hander...
> +static irqreturn_t tps6586x_rtc_irq(int irq, void *data)
> +{
> + struct device *dev = data;
> + struct tps6586x_rtc *rtc = dev_get_drvdata(dev);
> +
> + rtc_update_irq(rtc->rtc, 1, RTC_IRQF | RTC_AF);
> + return IRQ_HANDLED;
...only appears to handle alarm interrupts.
> + rtc->epoch_start = mktime(TPS_EPOCH, 1, 1, 0, 0, 0);
Should this be configurable in platform data?
> + if (pdata && (pdata->irq >= 0)) {
> + rtc->irq = pdata->irq;
> + err = request_threaded_irq(pdata->irq, NULL, tps6586x_rtc_irq,
> + IRQF_ONESHOT, "tps6586x-rtc",
> + &pdev->dev);
> + if (err) {
> + dev_warn(&pdev->dev, "unable to request IRQ\n");
> + rtc->irq = -1;
It'd be nice to print out the interrupt number here in case the issue is
someone misspecified it.
> +MODULE_DESCRIPTION("TI TPS6586x RTC driver");
> +MODULE_AUTHOR("NVIDIA Corporation");
> +MODULE_LICENSE("GPL");
Should really have a MODULE_ALIAS() too.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
