From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH V3] drivers: rtc: Add support for Qualcomm PMIC8xxx RTC Date: Thu, 26 May 2011 10:37:12 -0700 Message-ID: <4DDE8FC8.3020405@codeaurora.org> References: <1304574535-23710-1-git-send-email-aghayal@codeaurora.org> <1306386935-8806-1-git-send-email-aghayal@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from wolverine01.qualcomm.com ([199.106.114.254]:23359 "EHLO wolverine01.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753892Ab1EZRhO (ORCPT ); Thu, 26 May 2011 13:37:14 -0400 In-Reply-To: <1306386935-8806-1-git-send-email-aghayal@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Anirudh Ghayal Cc: Andrew Morton , rtc-linux@googlegroups.com, Wan ZongShun , linux-arm-msm@vger.kernel.org, Ashay Jaiswal On 5/25/2011 10:15 PM, Anirudh Ghayal wrote: > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c > new file mode 100644 > index 0000000..4694c4e > --- /dev/null > +++ b/drivers/rtc/rtc-pm8xxx.c > + > +/** > + * struct pm8xxx_rtc rtc driver internal structure This is missing a dash '-'. > +static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val, > + int base, int count) > +{ > + int i, rc; > + struct device *parent = rtc_dd->rtc_dev->parent; > + > + for (i = 0; i < count; i++) { > + rc = pm8xxx_readb(parent, base + i, &rtc_val[i]); > + if (rc < 0) { > + dev_err(rtc_dd->rtc_dev, "PMIC read failed\n"); I still think these printks are useless when you already have better printks at the call sites on the error path. > + return rc; > + } > + } > + > + return 0; > +} > + > +static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val, > + int base, int count) > +{ > + int i, rc; > + struct device *parent = rtc_dd->rtc_dev->parent; > + > + for (i = 0; i < count; i++) { > + rc = pm8xxx_writeb(parent, base + i, rtc_val[i]); > + if (rc < 0) { > + dev_err(rtc_dd->rtc_dev, "PMIC write failed\n"); > + return rc; > + } > + } > + > + return 0; > +} > + > +/* > + * Steps to write the RTC registers. > + * 1. Disable alarm if enabled. > + * 2. Write 0x00 to LSB. > + * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0]. > + * 4. Enable alarm if disabled in step 1. > + */ > +static int > +pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + int rc, i; > + unsigned long secs, irq_flags; > + u8 value[NUM_8_BIT_RTC_REGS], reg = 0, alarm_enabled = 0, ctrl_reg; > + struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev); > + > + rtc_tm_to_time(tm, &secs); > + > + for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) { > + value[i] = secs & 0xFF; > + secs >>= 8; > + } > + > + dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs); > + > + spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags); > + ctrl_reg = rtc_dd->ctrl_reg; > + > + if (ctrl_reg & PM8xxx_RTC_ALARM_ENABLE) { > + alarm_enabled = 1; > + ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE; > + rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, > + 1); > + if (rc < 0) { > + dev_err(dev, "Write to RTC control register " > + "failed\n"); > + goto rtc_rw_fail; > + } > + rtc_dd->ctrl_reg = ctrl_reg; > + } else > + spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags); > + > + /* Write 0 to Byte[0] */ > + reg = 0; > + rc = pm8xxx_write_wrapper(rtc_dd, ®, rtc_dd->rtc_write_base, 1); > + if (rc < 0) { > + dev_err(dev, "Write to RTC write data register failed\n"); > + goto rtc_rw_fail; > + } > + > + /* Write Byte[1], Byte[2], Byte[3] */ > + rc = pm8xxx_write_wrapper(rtc_dd, value + 1, > + rtc_dd->rtc_write_base + 1, 3); > + if (rc < 0) { > + dev_err(dev, "Write to RTC write data register failed\n"); > + goto rtc_rw_fail; > + } > + > + /* Write Byte[0] */ > + rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->rtc_write_base, 1); > + if (rc < 0) { > + dev_err(dev, "Write to RTC write data register failed\n"); > + goto rtc_rw_fail; > + } > + > + if (alarm_enabled) { > + ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE; > + rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, > + 1); > + if (rc < 0) { > + dev_err(dev, "Write to RTC control register " > + "failed\n"); > + goto rtc_rw_fail; > + } > + rtc_dd->ctrl_reg = ctrl_reg; > + } > + > +rtc_rw_fail: > + if (alarm_enabled) > + spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags); > + This could be simplified by putting the label inside the if and then putting the spin_unlock at the end of the if. You would save a branch. Not sure it really matters though since the compiler should be doing it anyway. Is it really that bad to take the lock all the time? It would simplify this function a bit if you did. If you fix the kernel-doc error you can add a Reviewed-by: Stephen Boyd -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.