Messages in this thread | | | Date | Mon, 10 Aug 2009 23:08:24 +0200 | From | Alessandro Zummo <> | Subject | Re: [rtc-linux] Re: [PATCH 4/5] RTC: Add support for RTCs on Wolfson WM831x devices |
| |
On Mon, 10 Aug 2009 21:57:13 +0100 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> > > given you have your own include directory undef mfd/ > > you might want to move those #defines there > > Unlike the others these registers can't really be used outside of this > driver - for the other drivers there's potential for at least platform > specific code if not multiple drivers to use some or all of the register > definitions.
ack.
> > > +struct wm831x_rtc { > > > + struct wm831x *wm831x; > > > + struct rtc_device *rtc; > > > + int alarm_enabled; > > > + int per_irq; > > > are those tows int or unsigned int? > > I've just dropped per_irq, it's not needed anyway. > > > or maybe alarm_enabled could be :1 ? > > Done, but it doesn't really buy us much given that there's nothing > else to pack it with.
ack.
> > > +/* > > > + * Set current time and date in RTC > > > + */ > > > +static int wm831x_rtc_settime(struct device *dev, struct rtc_time *tm) > > > isn't rtc_set_mmss more appropriate? > > Hrm, I think so so I've made the change. It's not a particularly > discoverable API - the fact that there's no readback equivalent hides > the fact that it's supposed to be an equivalent for settime. Some > documentation would really help here.
you're right. there's one but it's still in my tree. will push it one day.
> > > > + ret = wm831x_set_bits(wm831x_rtc->wm831x, WM831X_RTC_CONTROL, > > > + WM831X_RTC_ALM_ENA, enable); > > > + if (ret != 0) > > > + dev_err(&pdev->dev, "Failed to update RTC alarm: %d\n", ret); > > > + > > > + return 0; > > > always 0 ? (also below..) > > Failing suspend and resume due to failure to disable the RTC alarm > would be excessive - indeed, I'd expect the overwhelming majority of > systems to function perfectly well with no suspend/resume support in the > driver at all. RTC alarms are infrequent relative to other > suspend/resume events in typical systems but suspend is normally used > fairly heavily to preserve battery (typical applications include things > like MP3 players or phones). Typically the error handling would at best > cause more serious consequences than the original error and there's > little chance the user will be able to even report the error.
ack.
> > > +static int __init wm831x_rtc_init(void) > > > +{ > > > + return platform_driver_register(&wm831x_rtc_driver); > > > can you use platform_driver_probe() ? > > No, this is a MFD accessed over slow buses and we can't guarantee that > the device will be registered. > > Fixed everything else, will repost tomorrow.
here's my acked-by in advance, feel free to push the patch with the series.
Acked-by: Alessandro Zummo <a.zummo@towertech.it>
--
Best regards,
Alessandro Zummo, Tower Technologies - Torino, Italy
http://www.towertech.it
| |