Messages in this thread Patch in this message | | | Date | Wed, 7 May 2014 13:01:06 -0700 | From | Andrew Morton <> | Subject | Re: [RESEND] [PATCH V1 2/2] rtc: da9063: RTC driver |
| |
On Wed, 7 May 2014 13:22:52 +0100 "Opensource [Steve Twiss]" <stwiss.opensource@diasemi.com> wrote:
> Add the RTC driver for DA9063.
A few minor things:
> > ... > > +static int da9063_rtc_stop_alarm(struct device *dev) > +{ > + struct da9063_rtc *rtc = dev_get_drvdata(dev); > + return regmap_update_bits(rtc->hw->regmap, DA9063_REG_ALARM_Y, > + DA9063_ALARM_ON, 0); > +} > + > +static int da9063_rtc_start_alarm(struct device *dev) > +{ > + struct da9063_rtc *rtc = dev_get_drvdata(dev); > + return regmap_update_bits(rtc->hw->regmap, DA9063_REG_ALARM_Y, > + DA9063_ALARM_ON, DA9063_ALARM_ON); > +}
It's conventional to put a newline between end-of-locals and start-of-code. My version of checkpatch warns about this and soon everyone's will.
> +static int da9063_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct da9063_rtc *rtc = dev_get_drvdata(dev); > + unsigned long tm_secs; > + unsigned long al_secs; > + u8 data[RTC_DATA_LEN]; > + int ret; > + > + ret = regmap_bulk_read(rtc->hw->regmap, DA9063_REG_COUNT_S, > + data, RTC_DATA_LEN); > + if (ret < 0) { > + dev_err(dev, "Failed to read RTC time data: %d\n", ret); > + return ret; > + } > + > + if (!(data[RTC_SEC] & DA9063_RTC_READ)) { > + dev_dbg(dev, "RTC not yet ready to be read by the host\n"); > + return -EINVAL; > + } > + > + da9063_data_to_tm(data, tm); > + > + (void)rtc_tm_to_time(tm, &tm_secs); > + (void)rtc_tm_to_time(&rtc->alarm_time, &al_secs);
The void casts are a bit useful - they tell the reader that the author deliberately chose to ignore the return value. But kernel code generally doesn't do this trick.
> + /* handle the rtc synchronisation delay */ > + if (rtc->rtc_sync == true && al_secs - tm_secs == 1) > + (void)memcpy((void *)tm, (const void *)&rtc->alarm_time, > + sizeof(struct rtc_time));
And all three casts here are unneeded.
And they're actually a bit harmful. If rtc->alarm_time has type `int' then we really want the warning, but this cast will suppress it.
> + else > + rtc->rtc_sync = false; > + > + return rtc_valid_tm(tm); > +}
So how does this look?
--- a/drivers/rtc/rtc-da9063.c~rtc-da9063-rtc-driver-fix +++ a/drivers/rtc/rtc-da9063.c @@ -82,6 +82,7 @@ static void da9063_tm_to_data(struct rtc static int da9063_rtc_stop_alarm(struct device *dev) { struct da9063_rtc *rtc = dev_get_drvdata(dev); + return regmap_update_bits(rtc->hw->regmap, DA9063_REG_ALARM_Y, DA9063_ALARM_ON, 0); } @@ -89,6 +90,7 @@ static int da9063_rtc_stop_alarm(struct static int da9063_rtc_start_alarm(struct device *dev) { struct da9063_rtc *rtc = dev_get_drvdata(dev); + return regmap_update_bits(rtc->hw->regmap, DA9063_REG_ALARM_Y, DA9063_ALARM_ON, DA9063_ALARM_ON); } @@ -115,13 +117,12 @@ static int da9063_rtc_read_time(struct d da9063_data_to_tm(data, tm); - (void)rtc_tm_to_time(tm, &tm_secs); - (void)rtc_tm_to_time(&rtc->alarm_time, &al_secs); + rtc_tm_to_time(tm, &tm_secs); + rtc_tm_to_time(&rtc->alarm_time, &al_secs); /* handle the rtc synchronisation delay */ if (rtc->rtc_sync == true && al_secs - tm_secs == 1) - (void)memcpy((void *)tm, (const void *)&rtc->alarm_time, - sizeof(struct rtc_time)); + memcpy(tm, &rtc->alarm_time, sizeof(struct rtc_time)); else rtc->rtc_sync = false; _
| |