Re: [PATCH V2 5/5] rtc: max77686: move initialisation of rtc regmap, irq chip locally

[Krzysztof Kozlowski]

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@xxxxxxxxxxx>
> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
> CC: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> CC: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> 
> ---
> 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 ]---


Without patch 5 it is ok.

BR,
Krzysztof