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

[Mark Brown]

On Mon, Aug 10, 2009 at 08:55:35PM +0200, Alessandro Zummo wrote:
> Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> 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.

> > +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.

> > +/*
> > + * 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.

> > +	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.

> > +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/