Re: [PATCHv2 1/5] rtc: s3c: Define s3c_rtc structure to remove global variables.

From: Chanwoo Choi
Date: Thu Aug 28 2014 - 00:49:40 EST

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@xxxxxxxxxxx> wrote:
>> Dear Andrew,
>> On 08/23/2014 05:42 AM, Andrew Morton wrote:
>>> On Tue, 12 Aug 2014 11:01:07 +0900 y@xxxxxxxxxxx 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

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at