Message-id: <56B1E2F3.9010502@samsung.com>
Date:	Wed, 03 Feb 2016 20:22:27 +0900
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Laxman Dewangan <ldewangan@...dia.com>, lee.jones@...aro.org,
	alexandre.belloni@...e-electrons.com, javier@....samsung.com
Cc:	cw00.choi@...sung.com, linux-kernel@...r.kernel.org,
	rtc-linux@...glegroups.com
Subject: Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap,
 irq chip locally

On 03.02.2016 19:45, Krzysztof Kozlowski wrote:
> On 03.02.2016 18:30, Laxman Dewangan wrote:
>> To make RTC block of MAX77686/MAX77802 as independent driver,
>> move the registration of i2c device, regmap for register access
>> and irq_chip for interrupt support inside the RTC driver.
>> Removed the same initialisation from MFD driver.
>>
>> Having this change will allow to reuse this driver for different
>> PMIC/devices from Maxim Semiconductor if they kept same RTC IP on
>> different PMIC. Some of examples as PMIC MAX77620, MAX20024 where
>> same RTC IP used and hence driver for these chips will use this
>> driver only for RTC support.
>>
>> Suggested-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
>> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>
>> CC: Krzysztof Kozlowski <k.kozlowski@...sung.com>
>> CC: Javier Martinez Canillas <javier@....samsung.com>
>>
>> ---
>> Changes from V1:
>> - Remove changes from Kconfig.
>> - Maintain all register definition in max77686 private header and remove
>>   the movement to rtc driver.
>> - Taken care of all comments on V1 from Krzysztof and Javier.
> 
> Almost everything is good. You skipped one my comment, at the end:
> 
> (...)
> 
>> @@ -659,14 +745,33 @@ static int max77686_rtc_probe(struct platform_device *pdev)
>>  	ret = devm_request_threaded_irq(&pdev->dev, info->virq, NULL,
>>  					max77686_rtc_alarm_irq, 0,
>>  					"rtc-alarm1", info);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>>  			info->virq, ret);
>> +		goto err_rtc;
>> +	}
>> +
>> +	return 0;
>>  
>>  err_rtc:
>> +	if (info->rtc)
>> +		i2c_unregister_device(info->rtc);
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>> +
> 
> You should clean up in reverse order of allocation, so first
> regmap_del_irq_chip then i2c_unregister_device. This
> is a common pattern of cleaning up. Sometimes such order
> is even necessary because of dependencies between
> components... which is not a case here but still the
> natural way is reversing the allocation code.
> 
>>  	return ret;
>>  }
>>  
>> +static int max77686_rtc_remove(struct platform_device *pdev)
>> +{
>> +	struct max77686_rtc_info *info = platform_get_drvdata(pdev);
>> +
>> +	regmap_del_irq_chip(info->rtc_irq, info->rtc_irq_data);
>> +	if (info->rtc)
>> +		i2c_unregister_device(info->rtc);
> 
> Here it is okay.
> 
> Anyway RTC works... but unbinding leads to oops:
> echo max77686-rtc > /sys/bus/platform/drivers/max77686-rtc/unbind
> [   80.505411] Unable to handle kernel paging request at virtual address ffffffff
> [   80.511257] pgd = ec3a0000
> [   80.513870] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   80.520130] Internal error: Oops: 37 [#1] PREEMPT SMP ARM
> [   80.525498] Modules linked in:
> [   80.528551] CPU: 2 PID: 1325 Comm: sleep Tainted: G        W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   80.538867] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   80.544947] task: eeb53440 ti: eeb12000 task.ti: eeb12000
> [   80.550349] PC is at kmem_cache_alloc+0x7c/0x154
> [   80.554933] LR is at kmem_cache_alloc+0x3c/0x154
> [   80.559535] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
> [   80.559535] sp : eeb13e18  ip : 00000001  fp : bee6c4f4
> [   80.570986] r10: 024080c0  r9 : eeb13ed0  r8 : 00033d72
> [   80.576195] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   80.582704] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
> [   80.589217] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   80.596332] Control: 10c5387d  Table: 6c3a004a  DAC: 00000051
> [   80.602061] Process sleep (pid: 1325, stack limit = 0xeeb12210)
> [   80.607963] Stack: (0xeeb13e18 to 0xeeb14000)
> [   80.612306] 3e00:                                                       c0858280 ee4ada00
> [   80.620475] 3e20: c0863e80 eeb13f74 c000fa84 eeb13ed0 00000000 c00deda8 b6f4f000 00000011
> [   80.628632] 3e40: 00000003 eeb13f74 00000001 c00e8ebc 386de9c0 00000000 00000001 00000001
> [   80.636792] 3e60: eeb53440 00000041 eee89514 0000014f 00000054 0000053c eeb6c190 ee4f653c
> [   80.644934] 3e80: 00013000 ee4f6000 00000001 00000000 00000054 024200ca 00000010 b6f4e000
> [   80.653093] 3ea0: ee58e7a8 b6f3e000 00000011 00000003 eeb13f74 00000001 00000005 c000fa84
> [   80.661253] 3ec0: eeb12000 00000000 bee6c4f4 c00eaa6c 00000000 c0018570 eea49700 ee58e160
> [   80.669412] 3ee0: 00000000 00000000 c001921c 00000017 c0018388 b6f4f0dd c085cf34 eeb13fb0
> [   80.677571] 3f00: b6f96b40 7f62b751 00000000 eeb13f10 eeb13f24 ef001b80 00000001 00000001
> [   80.685730] 3f20: 00000003 ee5aac00 ee5aac24 00000020 00080000 c05a0390 00000000 c00f72ec
> [   80.693889] 3f40: eebac000 00000000 eebac000 ffffff9c ffffff9c c000fa84 00000003 eebac000
> [   80.702048] 3f60: ffffff9c c00dc29c eeb53440 00000003 ee5aac00 00000000 eea40000 00000004
> [   80.710208] 3f80: 00000100 00000001 eea49700 00000008 00000000 bee6c534 00000005 c000fa84
> [   80.718367] 3fa0: 00000000 c000f8c0 00000008 00000000 b6f4f0ce 00080000 b6f96958 00000000
> [   80.726526] 3fc0: 00000008 00000000 bee6c534 00000005 00000001 b6f4f0ce 00000000 bee6c4f4
> [   80.734685] 3fe0: b6f95ce0 bee6c4ac b6f6ba14 b6f7f24c 600f0010 b6f4f0ce ffffffff ffffffff
> [   80.742854] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   80.751008] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   80.758731] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   80.766195] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   80.773838] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   80.781559] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   80.787694] ---[ end trace 9502799e3ea05a7e ]---
> [   80.792503] Unable to handle kernel paging request at virtual address ffffffff
> [   80.799457] pgd = ec39c000
> [   80.802140] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   80.808377] Internal error: Oops: 37 [#2] PREEMPT SMP ARM
> [   80.813756] Modules linked in:
> [   80.816798] CPU: 2 PID: 628 Comm: sh Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   80.826776] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   80.832853] task: ec3050c0 ti: ee66e000 task.ti: ee66e000
> [   80.838240] PC is at kmem_cache_alloc+0x7c/0x154
> [   80.842836] LR is at kmem_cache_alloc+0x3c/0x154
> [   80.847437] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a00f0013
> [   80.847437] sp : ee66fe18  ip : 00000001  fp : beaba124
> [   80.858893] r10: 024080c0  r9 : ee66fed0  r8 : 00033d72
> [   80.864100] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   80.870610] r3 : 00000000  r2 : 00033d72  r1 : c070e2ac  r0 : 2ef40000
> [   80.877120] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   80.884237] Control: 10c5387d  Table: 6c39c04a  DAC: 00000051
> [   80.889966] Process sh (pid: 628, stack limit = 0xee66e210)
> [   80.895522] Stack: (0xee66fe18 to 0xee670000)
> [   80.899862] fe00:                                                       c0858280 ec264f80
> [   80.908023] fe20: c0863e80 ee66ff74 c000fa84 ee66fed0 00000000 c00deda8 00000040 ee58e9a0
> [   80.916182] fe40: 00000003 ee66ff74 00000001 c00e8ebc ec39efa8 eeb6cee0 69f907df ef00cc10
> [   80.924341] fe60: c0850200 00000041 00000001 00000195 00000055 00000654 ec39edb8 ee464380
> [   80.932500] fe80: ffffff05 c023bd4c 00000000 00000000 00000000 ee464380 ee58e9a0 c00c0904
> [   80.940660] fea0: ee464380 ee58e9a0 ee464380 00000003 ee66ff74 00000001 00000005 c000fa84
> [   80.948819] fec0: ee66e000 00000000 beaba124 c00eaa6c 00000800 c0018570 ec19fa08 00000000
> [   80.956978] fee0: 00000000 00000000 00000000 00000817 c0018388 b6f95000 c085cf34 ee66ffb0
> [   80.965137] ff00: 00000000 b6f974c0 00000000 ee66ff10 ee66ff24 ef001b80 00000001 00000001
> [   80.973296] ff20: 00000003 ee59ba00 ee405e00 00000100 00080000 c05a0390 00000000 c00f72ec
> [   80.981456] ff40: eebaf000 00000000 eebaf000 ffffff9c ffffff9c c000fa84 00000003 eebaf000
> [   80.989615] ff60: ffffff9c c00dc29c beaba52c c00b0e8c 00000022 00000000 000b0000 00000004
> [   80.997774] ff80: 00000100 00000001 ffffffff 000d6430 00000008 00000008 00000005 c000fa84
> [   81.005933] ffa0: 00000000 c000f8c0 000d6430 00000008 beaba148 00080000 000001b6 000001b6
> [   81.014092] ffc0: 000d6430 00000008 00000008 00000005 00000000 b6f0405c beaba408 beaba124
> [   81.022252] ffe0: 00080000 beaba0c4 b6e3529c b6e8a9bc 200f0010 beaba148 6fffd861 6fffdc61
> [   81.030416] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   81.038572] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   81.046296] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   81.053761] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   81.061401] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   81.069124] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   81.075245] ---[ end trace 9502799e3ea05a7f ]---
> [   88.035087] Unable to handle kernel paging request at virtual address ffffffff
> [   88.040971] pgd = ee604000
> [   88.043617] [ffffffff] *pgd=6fffd861, *pte=00000000, *ppte=00000000
> [   88.049822] Internal error: Oops: 37 [#3] PREEMPT SMP ARM
> [   88.055186] Modules linked in:
> [   88.058245] CPU: 2 PID: 396 Comm: deviced Tainted: G      D W       4.5.0-rc2-next-20160202-00006-gb28bd1d36a08 #62
> [   88.068641] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [   88.074723] task: ec089c80 ti: ec180000 task.ti: ec180000
> [   88.080132] PC is at kmem_cache_alloc+0x7c/0x154
> [   88.084712] LR is at kmem_cache_alloc+0x3c/0x154
> [   88.089313] pc : [<c00d5818>]    lr : [<c00d57d8>]    psr: a0000013
> [   88.089313] sp : ec181e18  ip : 00000001  fp : be9821e4
> [   88.100763] r10: 024080c0  r9 : ec181ed0  r8 : 00033d82
> [   88.105971] r7 : c00deda8  r6 : c08585f4  r5 : ffffffff  r4 : ef001d80
> [   88.112482] r3 : 00000000  r2 : 00033d82  r1 : c070e2ac  r0 : 2ef40000
> [   88.118995] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   88.126095] Control: 10c5387d  Table: 6e60404a  DAC: 00000051
> [   88.131824] Process deviced (pid: 396, stack limit = 0xec180210)
> [   88.137812] Stack: (0xec181e18 to 0xec182000)
> [   88.142153] 1e00:                                                       c0858280 ee7bb900
> [   88.150317] 1e20: c0863e80 ec181f74 c000fa84 ec181ed0 00000000 c00deda8 ec181e50 ec181e54
> [   88.158475] 1e40: 00000016 ec181f74 00000001 c00e8ebc 00000000 00000000 00000000 00000000
> [   88.166635] 1e60: ec181eb8 00000041 ffede000 00000127 00000055 0000049c ee605fd8 00002000
> [   88.174793] 1e80: 00000011 ec181f80 00000044 00000000 00000011 c012ca68 00000000 eb17e260
> [   88.182953] 1ea0: 00000051 00000011 ec181f80 00000016 ec181f74 00000001 00000005 c000fa84
> [   88.191112] 1ec0: ec180000 00000000 be9821e4 c00eaa6c 00000800 c0018570 be981c48 ee51cd24
> [   88.199272] 1ee0: ec089c80 00000008 ee51ccc0 0000081f c0018388 7f727c20 c085cfb4 ec181fb0
> [   88.207430] 1f00: c792168f ffffffff 00000000 ec181f10 ec181f24 ef001b80 00000001 00000001
> [   88.215590] 1f20: 00000016 ee5b1000 ee74d7c0 00000100 00000000 c05a0390 00000000 c00f72ec
> [   88.223749] 1f40: eeba9000 00000000 eeba9000 ffffff9c ffffff9c c000fa84 00000016 eeba9000
> [   88.231908] 1f60: ffffff9c c00dc29c ec180000 be9828a8 00000051 00000000 c0000000 00000004
> [   88.240067] 1f80: 00000100 00000001 00000000 7f721c00 00000008 00000008 00000005 c000fa84
> [   88.248226] 1fa0: 00000000 c000f8c0 7f721c00 00000008 be9822cc 00000000 000001b6 000001b6
> [   88.256386] 1fc0: 7f721c00 00000008 00000008 00000005 7f6de5c0 be9822cc be9822cc be9821e4
> [   88.264544] 1fe0: 00000000 be982180 b6efb4c0 b669aa14 80000010 be9822cc 00000000 00000000
> [   88.272716] [<c00d5818>] (kmem_cache_alloc) from [<c00deda8>] (get_empty_filp+0x70/0x1d4)
> [   88.280868] [<c00deda8>] (get_empty_filp) from [<c00e8ebc>] (path_openat+0x20/0xf1c)
> [   88.288591] [<c00e8ebc>] (path_openat) from [<c00eaa6c>] (do_filp_open+0x60/0xb4)
> [   88.296055] [<c00eaa6c>] (do_filp_open) from [<c00dc29c>] (do_sys_open+0x114/0x1c0)
> [   88.303699] [<c00dc29c>] (do_sys_open) from [<c000f8c0>] (ret_fast_syscall+0x0/0x3c)
> [   88.311419] Code: e7905003 e3550000 0a00001f e5943014 (e7950003)
> [   88.317573] ---[ end trace 9502799e3ea05a80 ]---

However removal of "remove" callback helps... which could be expected...
maybe it is not an error of the driver itself?

Best regards,
Krzysztof

Powered by blists - more mailing lists