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

[Samuel Ortiz <sameo@linux.intel.com>]

Hi Alessandro,

On Mon, Aug 10, 2009 at 11:08:24PM +0200, Alessandro Zummo wrote:
> 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>
Thanks, patch applied.

Cheers,
Samuel.

 
>  
> 
> -- 
> 
>  Best regards,
> 
>  Alessandro Zummo,
>   Tower Technologies - Torino, Italy
> 
>   http://www.towertech.it
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/