>From: Sascha Hauer [mailto:s.hauer@xxxxxxxxxxxxxx] >Sent: Mittwoch, 6. Dezember 2017 09:36 >On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev@xxxxxxxxxxxx >wrote: >> +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata, >> + struct rtc_time *alarm_tm) >> +{ >> + void __iomem *const ioaddr = pdata->ioaddr; >> + unsigned long time; >> + >> + rtc_tm_to_time(alarm_tm, &time); >> + >> + if (time > U32_MAX) { >> + pr_err("Hopefully I am out of service by then :-(\n"); >> + return -EINVAL; >> + } > >This will never happen as on your target hardware unsigned long is a >32bit type. Not sure what is best to do here. Maybe you should test >the return value of rtc_tm_to_time. ATM it returns 0 unconditionally, >but rtc_tm_to_time could detect when the input time doesn't fit into >its return type and return an error in this case. >Also I just realized that it's unsigned and only overflows in the year >2106. I'm most likely dead then so I don't care that much ;) > please see my response to Alexandre's follow up >> +/* This function is the RTC interrupt service routine. */ >> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) >> +{ >> + struct platform_device *pdev = dev_id; >> + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev); >> + void __iomem *ioaddr = pdata->ioaddr; >> + unsigned long flags; >> + u32 events = 0; >> + u32 lp_status; >> + u32 lp_cr; >> + >> + spin_lock_irqsave(&pdata->lock, flags); >> + if (clk_prepare_enable(pdata->clk)) { >> + spin_unlock_irqrestore(&pdata->lock, flags); >> + return IRQ_NONE; >> + } > >You are not allowed to do a clk_prepare under a spinlock. That was the >original reason to split enabling a clk into clk_prepare and clk_enable. >Everything that can block is done in clk_prepare and only non blocking >things are done in clk_enable. >If you want to enable/disable the clock on demand you can clk_prepare() >in probe and clk_enable when you actually need it. > Thanks for clarification. To be honest when I read Lothar's suggestion it was the first time I thought about the idea of keeping the clk disabled most of the time. I am not very experienced with this. But a rtctest loop run for hours so I assume it would be okay to keep the clk disabled until hw access. If there is no objection from somebody who knows the i.MX53 SRTC HW better, I will stick to the clock on demand model and make sure I avoid blocking. >> + >> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) >> +{ >> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); >> + time_t now; >> + int ret = mxc_rtc_lock(pdata); >> + >> + if (ret) >> + return ret; >> + >> + now = readl(pdata->ioaddr + SRTC_LPSCMR); >> + rtc_time_to_tm(now, tm); >> + ret = rtc_valid_tm(tm); >> + mxc_rtc_unlock(pdata); > >I don't think this needs to be locked. I will change this to only enable the clock for the readl() >> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) >> +{ >> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); >> + int ret = mxc_rtc_lock(pdata); >> + >> + if (ret) >> + return ret; >> + >> + ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time); > >Is it worth it to make this a separate function? Maybe not, I think it is an artifact from a refactoring. I will reconsider this for the next version. > >> + if (!ret) { >> + mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled); >> + mxc_rtc_sync_lp_locked(pdata->ioaddr); >> + } >> + mxc_rtc_unlock(pdata); >> + return ret; >> +} >> + >> +static const struct rtc_class_ops mxc_rtc_ops = { >> + .read_time = mxc_rtc_read_time, >> + .set_time = mxc_rtc_set_time, >> + .read_alarm = mxc_rtc_read_alarm, >> + .set_alarm = mxc_rtc_set_alarm, >> + .alarm_irq_enable = mxc_rtc_alarm_irq_enable, >> +}; >> + >> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag) >> +{ >> + unsigned int timeout = REG_READ_TIMEOUT; >> + >> + while (!(readl(ioaddr) & flag)) { >> + if (!--timeout) { >> + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr); > >Please use dev_* functions for printing. In this case the message should >probably be printed from the caller. Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here, because I would have to change the functions signature to get a device. However, I will drop the message and move it to the caller. >> + /* clear lp interrupt status */ >> + writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); >> + >> + /* move out of init state */ >> + writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); >> + xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES); > >If this can fail, shouldn't you test for an error? very probably >> + >> + /* move out of non-valid state */ >> + writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | >> + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); >> + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES); > >dito sure Thanks, Patrick --- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
