On Tuesday 25 January 2011 08:13 PM, Mark Brown wrote:
> 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.
>
Will do the needful
>> +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.
>
Will do the needful
>> +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?
>
Will do the needful
>> + if (alrm->enabled && (rtc->irq != -1)) {
> No need to check the IRQ here either.
>
Will do the needful
>> +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.
>
That's right. Since only a single alarm is being used, rtc->irq is
treated as the controlling element.
>> + rtc->epoch_start = mktime(TPS_EPOCH, 1, 1, 0, 0, 0);
> Should this be configurable in platform data?
>
Will do the needful
>> + 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.
>
Will do the needful
>> +MODULE_DESCRIPTION("TI TPS6586x RTC driver");
>> +MODULE_AUTHOR("NVIDIA Corporation");
>> +MODULE_LICENSE("GPL");
> Should really have a MODULE_ALIAS() too.
Will do the needful
nvpublic
--
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
