Messages in this thread | | | Date | Thu, 28 Aug 2014 13:49:25 +0900 | From | Chanwoo Choi <> | Subject | Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables. |
| |
Dear Andrew,
On 08/27/2014 06:31 AM, Andrew Morton wrote: > On Mon, 25 Aug 2014 09:57:33 +0900 Chanwoo Choi <cw00.choi@samsung.com> wrote: > >> Dear Andrew, >> >> On 08/23/2014 05:42 AM, Andrew Morton wrote: >>> On Tue, 12 Aug 2014 11:01:07 +0900 y@samsung.com wrote: >>> >>>> This patch define s3c_rtc structure including necessary variables for S3C RTC >>>> device instead of global variables. This patch improves the readability by >>>> removing global variables. >>> >>> Below is the v1->v2 delta. >>> >>> Why were all those tests of info->base added? Can it really be zero? >>> I don't see how. >> >> If some functions (e.g., s3c_rtc_settime) accesses the rtc register >> by using info->base before the initialization of info->base in s3c_rtc_probe, >> I thought that null pointer error would happen. > > probe() should be called before anything else. If we're somehow > calling s3c_rtc_settime() before probe() has completed then something > very bad is happening - for example, the device may have been > registered far too early. But I don't think that's the case here.
I means that existing rtc-s3c.c driver executed s3c_rtc_settime() in s3c_rtc_probe() before initialization of info->base. But, It is my mistake. so, I modified it just as following:
- s3c_rtc_settime(NULL, &rtc_tm); + s3c_rtc_settime(&pdev->dev, &rtc_tm);
> > That being said, it does seem strange that s3c_rtc_probe() calls > devm_rtc_device_register() *before* trying to request its IRQs. So if > IRQ requesting fails, we go and immediately unregister the device. > Some other drivers do it this way, others do not. Wouldn't it be > better to defer registration until we know that all the probe() setup > operations have succeeded?
You're right. I missed this point. If rtc-s3c.c driver completed the probe function, info->base has always right address.
+ if (!info->base) + return -EINVAL; +
As you said, checking state of 'info-base' is un-needed. I'll send new patchset(v3) to fix it.
> >> But, I missed one point which info->base might have the garbate data instead of NULL. >> I'll add the initialization code for info->base. >> info->base = NULL; >> >> If you don't agree it, I'll drop this code checking the state of info->base on next patchset(v3). > > Well, we should have those checks in there unless we know they're > needed. And if they *are* needed, we should have a good understanding > of why they're needed, and we should be sure that we're not just > working around some underlying problem.
You are right. Thanks for your comment and advice.
Best Regards, Chanwoo Choi
| |