Re: [rtc-linux] Re: [PATCH 4/5] RTC: Add support for RTCs on Wolfson WM831x devices

Alessandro Zummo
2009-08-10T21:08:26Z

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