Add Richtek RT8555 Backlight Support for samsung-gtelwifiue #217

person4268 · 2022-01-16T09:29:15Z

The Richtek RT8555 is used as a backlight controller in samsung-gtelwifiue, and this adds support for it. It has support for both 8 bit and 10 bits of brightness levels, and this patch uses the 10 bit brightness mode.

TravMurav · 2022-01-16T09:41:03Z

drivers/video/backlight/rt8555-backlight.c

+		return -ENODEV;
+	}
+
+	ret = device_property_read_u32(&client->dev, "max-brightness", &brightness);


there should be a commit for dt bindings that would describe those

drivers/video/backlight/rt8555-backlight.c

person4268 · 2022-07-03T10:09:48Z

I've just finished rewriting this with actually proper IC initialization support. It contains commits from #241 because it'll probably get merged first and it creates a merge conflict, so I'd rather just rebase this PR on master once it gets merged.

There's probably some code quality issues, but it does work properly, and it also adds the backlight to the panel in the dtb, so brightness control properly works in userspace.

drivers/video/backlight/rt8555-backlight.c

stephan-gh · 2022-07-03T10:24:47Z

drivers/video/backlight/rt8555-backlight.c

+	priv->dev = &client->dev;
+
+	priv->enable =
+		devm_gpiod_get_optional(&client->dev, NULL, GPIOD_OUT_HIGH);


Is it really worth making this GPIO optional? All the error handling required makes me think it's not.

I personally thought that it's possible that a device -might- not have enable wired to a gpio for whatever reason, and it isn't really required for core functionality. I'll remove it if you want, though.

drivers/video/backlight/rt8555-backlight.c

arch/arm64/boot/dts/qcom/apq8016-samsung-gtelwifiue.dts

person4268 · 2022-07-03T11:34:00Z

I'd also like to ask whether it'd be worth (or even a good idea to do) implementing a simple brightness -> perceived brightness conversion inside the driver, since currently the relation between the set brightness and the perceived brightness doesn't seem even remotely linear (the last half of the brightness values have much less of a difference in brightness than the first half). Other mainline drivers don't appear to do it, but the downstream does (though a massive lookup table). There's various functions that can estimate this (though I haven't tried), such as `pow(max_brightness, brightness/max_brightness), and the inverse of this one could be easily calculated.

stephan-gh · 2022-07-03T11:56:34Z

There is something in pwm_bl for this, see the cie1931 function. I don't know if this is worth it or not or if userspace should be responsible for this.

person4268 · 2022-07-03T22:42:00Z

I don't really think there'd be a clean way to implement this in userspace, personally. But then again, I'd also probably have to create the inverse of the function for the get_brightness function (or create a table like they do), which seems like a lot of effort. I'd personally rather leave such functionality out for now.

person4268 · 2022-07-07T04:23:57Z

Rebased on master since #241 was merged. Is this ready for merging now?

The Richtek RT8555 is a 6 channel LED driver that supports up to 35mA/LED. This is a backlight driver that drives ILED1. Signed-off-by: Michael Abood <[email protected]>

This adds the RT8555 backlight driver to the device tree. Using i2c-gpio here is necessary, as the backlight ic is not on any i2c pins. Signed-off-by: Michael Abood <[email protected]>

stephan-gh · 2022-07-07T17:51:26Z

Is this ready for merging now?

The comment with the dt-bindings and the one potentially making the GPIO optional is still open.

Also, as per the contribution guidelines I would appreciate if you could send this driver to the upstream mailing lists. We do not have the capacity to keep additional drivers up to date. Feel free to ask any questions about the upstreaming process. :)

person4268 · 2022-07-07T23:29:02Z

Personally, I'm against the making the enable GPIO non-optional, simply because other drivers leave it optional and it's not integral to the functionality of the backlight, and doesn't make the logic that much more complex, and @stephan-gh hasn't replied, so I think we can consider that closed.

Do I need to add myself to the MAINTAINERS file also? It only has some individual drivers, so I'm not sure.

For sending patches to the mailing list, who would I end up addressing them to, and which fields would I use? scripts/get_maintainer.pl gives me 7 email addresses. Here's the output:

% scripts/get_maintainer.pl 0001-video-backlight-Add-support-for-Richtek-RT8555-Backl.patch
Lee Jones <[email protected]> (maintainer:BACKLIGHT CLASS/SUBSYSTEM)
Daniel Thompson <[email protected]> (maintainer:BACKLIGHT CLASS/SUBSYSTEM)
Jingoo Han <[email protected]> (maintainer:BACKLIGHT CLASS/SUBSYSTEM)
Helge Deller <[email protected]> (maintainer:FRAMEBUFFER LAYER)
[email protected] (open list)
[email protected] (open list:BACKLIGHT CLASS/SUBSYSTEM)
[email protected] (open list:FRAMEBUFFER LAYER)

I'd assume I'd want to mail to most of the mailing lists, except for maybe fbdev, and then CC all the maintainers? Then I imagine I would have to email mainly just the devicetree mailing list for the currently nonexistent dt-bindings commit.

Anyways, I'll start work on the dt-bindings now.

stephan-gh · 2022-07-08T10:00:01Z

Personally, I'm against the making the enable GPIO non-optional, simply because other drivers leave it optional and it's not integral to the functionality of the backlight, and doesn't make the logic that much more complex, and @stephan-gh hasn't replied, so I think we can consider that closed.

My comment is not marked as "resolved" like the others and you have not replied, so it was your turn, not mine. :)

If you want to keep the GPIO optional you need to fix the error handling. The IS_ERR check for the GPIO must be in the probe function to detect errors. Then it can only be NULL or valid later. Functions like gpiod_set_value() have internal NULL checks so you can likely drop most of the if statements.

Do I need to add myself to the MAINTAINERS file also? It only has some individual drivers, so I'm not sure.

Up to you. You can but you don't have to (not sure if it's worth it for such a small driver).

For sending patches to the mailing list, who would I end up addressing them to, and which fields would I use? scripts/get_maintainer.pl gives me 7 email addresses. Here's the output:

I would use --to for the BACKLIGHT CLASS/SUBSYSTEM maintainers (in other words: the recipients that you would expect to apply the patch), and --cc everyone else, including the mailing lists. However, there is no strict rule for that, AFAIK.

I'd assume I'd want to mail to most of the mailing lists, except for maybe fbdev, and then CC all the maintainers? Then I imagine I would have to email mainly just the devicetree mailing list for the currently nonexistent dt-bindings commit.

Please send the entire series (the two patches) to the same recipients. So you'd get the DT mailing lists and the two DT maintainers as additional Cc recipients.

person4268 · 2022-07-08T18:24:51Z

My comment is not marked as "resolved" like the others and you have not replied, so it was your turn, not mine. :)
I did reply, though?

It's probably the pending tag I guess? I replied to a lot of your other comments and they had the tag, so I'm guessing you can't see those either? Will note the rest of your suggestions/advice, though, and simplify the error checking.

person4268

I think I have to leave this message so that you can see the pending comments? I didn't understand Github's review system, I guess.

I'd check some of the ones I've resolved for comments

person4268 · 2022-07-03T10:14:31Z

drivers/video/backlight/rt8555-backlight.c

+	if (ret)
+		return ret;
+
+	/* "Change Duty" for Mixed Mode */


I'm honestly not sure what to call this. Datasheet calls it "Mixed Mode Change Duty" but I couldn't completely tell what it does exactly. I assume duty implies a duty cycle, so maybe something like "change-duty-cycle"? I left out mixed since I always have mixed mode on, also, not sure if that's the best idea but it seemed like the alternative used the PWM pin which I didn't want to support.

drivers/video/backlight/rt8555-backlight.c

person4268 · 2022-07-03T10:41:01Z

drivers/video/backlight/rt8555-backlight.c

+	priv->dev = &client->dev;
+
+	priv->enable =
+		devm_gpiod_get_optional(&client->dev, NULL, GPIOD_OUT_HIGH);


I personally thought that it's possible that a device -might- not have enable wired to a gpio for whatever reason, and it isn't really required for core functionality. I'll remove it if you want, though.

drivers/video/backlight/rt8555-backlight.c

…kprobe_event_gen_test_exit() When trace_get_event_file() failed, gen_kretprobe_test will be assigned as the error code. If module kprobe_event_gen_test is removed now, the null pointer dereference will happen in kprobe_event_gen_test_exit(). Check if gen_kprobe_test or gen_kretprobe_test is error code or NULL before dereference them. BUG: kernel NULL pointer dereference, address: 0000000000000012 PGD 0 P4D 0 Oops: 0000 [#1] SMP PTI CPU: 3 PID: 2210 Comm: modprobe Not tainted 6.1.0-rc1-00171-g2159299a3b74-dirty #217 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 RIP: 0010:kprobe_event_gen_test_exit+0x1c/0xb5 [kprobe_event_gen_test] Code: Unable to access opcode bytes at 0xffffffff9ffffff2. RSP: 0018:ffffc900015bfeb8 EFLAGS: 00010246 RAX: ffffffffffffffea RBX: ffffffffa0002080 RCX: 0000000000000000 RDX: ffffffffa0001054 RSI: ffffffffa0001064 RDI: ffffffffdfc6349c RBP: ffffffffa0000000 R08: 0000000000000004 R09: 00000000001e95c0 R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000800 R13: ffffffffa0002420 R14: 0000000000000000 R15: 0000000000000000 FS: 00007f56b75be540(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffff9ffffff2 CR3: 000000010874a006 CR4: 0000000000330ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> __x64_sys_delete_module+0x206/0x380 ? lockdep_hardirqs_on_prepare+0xd8/0x190 ? syscall_enter_from_user_mode+0x1c/0x50 do_syscall_64+0x3f/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd Link: https://lore.kernel.org/all/[email protected]/ Fixes: 6483624 ("tracing: Add kprobe event command generation test module") Signed-off-by: Shang XiaoJing <[email protected]> Acked-by: Masami Hiramatsu (Google) <[email protected]> Cc: [email protected] Signed-off-by: Masami Hiramatsu (Google) <[email protected]>

…kprobe_event_gen_test_exit() commit e0d7526 upstream. When trace_get_event_file() failed, gen_kretprobe_test will be assigned as the error code. If module kprobe_event_gen_test is removed now, the null pointer dereference will happen in kprobe_event_gen_test_exit(). Check if gen_kprobe_test or gen_kretprobe_test is error code or NULL before dereference them. BUG: kernel NULL pointer dereference, address: 0000000000000012 PGD 0 P4D 0 Oops: 0000 [msm8953-mainline#1] SMP PTI CPU: 3 PID: 2210 Comm: modprobe Not tainted 6.1.0-rc1-00171-g2159299a3b74-dirty msm8953-mainline#217 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 RIP: 0010:kprobe_event_gen_test_exit+0x1c/0xb5 [kprobe_event_gen_test] Code: Unable to access opcode bytes at 0xffffffff9ffffff2. RSP: 0018:ffffc900015bfeb8 EFLAGS: 00010246 RAX: ffffffffffffffea RBX: ffffffffa0002080 RCX: 0000000000000000 RDX: ffffffffa0001054 RSI: ffffffffa0001064 RDI: ffffffffdfc6349c RBP: ffffffffa0000000 R08: 0000000000000004 R09: 00000000001e95c0 R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000800 R13: ffffffffa0002420 R14: 0000000000000000 R15: 0000000000000000 FS: 00007f56b75be540(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: ffffffff9ffffff2 CR3: 000000010874a006 CR4: 0000000000330ee0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> __x64_sys_delete_module+0x206/0x380 ? lockdep_hardirqs_on_prepare+0xd8/0x190 ? syscall_enter_from_user_mode+0x1c/0x50 do_syscall_64+0x3f/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd Link: https://lore.kernel.org/all/[email protected]/ Fixes: 6483624 ("tracing: Add kprobe event command generation test module") Signed-off-by: Shang XiaoJing <[email protected]> Acked-by: Masami Hiramatsu (Google) <[email protected]> Cc: [email protected] Signed-off-by: Masami Hiramatsu (Google) <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>

If a kernel thread is created by a user thread, it may carry FPU/SIMD thread info flags (TIF_USEDFPU, TIF_USEDSIMD, etc.). Then it will be considered as a fpu owner and kernel try to save its FPU/SIMD context and cause such errors: [ 41.518931] do_fpu invoked from kernel context![#1]: [ 41.523933] CPU: 1 PID: 395 Comm: iou-wrk-394 Not tainted 6.1.0-rc5+ #217 [ 41.530757] Hardware name: Loongson Loongson-3A5000-7A1000-1w-CRB/Loongson-LS3A5000-7A1000-1w-CRB, BIOS vUDK2018-LoongArch-V2.0.pre-beta8 08/18/2022 [ 41.544064] $ 0 : 0000000000000000 90000000011e9468 9000000106c7c000 9000000106c7fcf0 [ 41.552101] $ 4 : 9000000106305d40 9000000106689800 9000000106c7fd08 0000000003995818 [ 41.560138] $ 8 : 0000000000000001 90000000009a72e4 0000000000000020 fffffffffffffffc [ 41.568174] $12 : 0000000000000000 0000000000000000 0000000000000020 00000009aab7e130 [ 41.576211] $16 : 00000000000001ff 0000000000000407 0000000000000001 0000000000000000 [ 41.584247] $20 : 0000000000000000 0000000000000001 9000000106c7fd70 90000001002f0400 [ 41.592284] $24 : 0000000000000000 900000000178f740 90000000011e9834 90000001063057c0 [ 41.600320] $28 : 0000000000000000 0000000000000001 9000000006826b40 9000000106305140 [ 41.608356] era : 9000000000228848 _save_fp+0x0/0xd8 [ 41.613542] ra : 90000000011e9468 __schedule+0x568/0x8d0 [ 41.619160] CSR crmd: 000000b0 [ 41.619163] CSR prmd: 00000000 [ 41.622359] CSR euen: 00000000 [ 41.625558] CSR ecfg: 00071c1c [ 41.628756] CSR estat: 000f0000 [ 41.635239] ExcCode : f (SubCode 0) [ 41.638783] PrId : 0014c010 (Loongson-64bit) [ 41.643191] Modules linked in: acpi_ipmi vfat fat ipmi_si ipmi_devintf cfg80211 ipmi_msghandler rfkill fuse efivarfs [ 41.653734] Process iou-wrk-394 (pid: 395, threadinfo=0000000004ebe913, task=00000000636fa1be) [ 41.662375] Stack : 00000000ffff0875 9000000006800ec0 9000000006800ec0 90000000002d57e0 [ 41.670412] 0000000000000001 0000000000000000 9000000106535880 0000000000000001 [ 41.678450] 9000000105291800 0000000000000000 9000000105291838 900000000178e000 [ 41.686487] 9000000106c7fd90 9000000106305140 0000000000000001 90000000011e9834 [ 41.694523] 00000000ffff0875 90000000011f034c 9000000105291838 9000000105291830 [ 41.702561] 0000000000000000 9000000006801440 00000000ffff0875 90000000002d48c0 [ 41.710597] 9000000128800001 9000000106305140 9000000105291838 9000000105291838 [ 41.718634] 9000000105291830 9000000107811740 9000000105291848 90000000009bf1e0 [ 41.726672] 9000000105291830 9000000107811748 2d6b72772d756f69 0000000000343933 [ 41.734708] 0000000000000000 0000000000000000 0000000000000000 0000000000000000 [ 41.742745] ... [ 41.745252] Call Trace: [ 42.197868] [<9000000000228848>] _save_fp+0x0/0xd8 [ 42.205214] [<90000000011ed468>] __schedule+0x568/0x8d0 [ 42.210485] [<90000000011ed834>] schedule+0x64/0xd4 [ 42.215411] [<90000000011f434c>] schedule_timeout+0x88/0x188 [ 42.221115] [<90000000009c36d0>] io_wqe_worker+0x184/0x350 [ 42.226645] [<9000000000221cf0>] ret_from_kernel_thread+0xc/0x9c This can be easily triggered by ltp testcase syscalls/io_uring02 and it can also be easily fixed by clearing the FPU/SIMD thread info flags for kernel threads in copy_thread(). Cc: [email protected] Reported-by: Qi Hu <[email protected]> Signed-off-by: Huacai Chen <[email protected]>

RDS5 · 2024-07-21T20:21:24Z

is anyone going to upstream this? since the backlight driver (unlike the broken panel driver) is working perfectly fine under mainline

person4268 · 2024-07-21T20:38:05Z

Honestly, it's been in a perpetual state of "I want to upstream this" but I've never gotten around to actually doing it, half due to me being unsure on some properties when writing the documentation yaml (writing a description is hard when the datasheet's unclear as to what some fields mean), and half due to how completely unfamiliar I am with the LKML submission process.

I have some uncommitted WIP files I could dig out, plus I think an updated version of the driver that builds on newer kernels that I can put up in this PR. I also wouldn't be completely opposed to someone else continuing the PR and submitting it to the LKML in my place (though I will likely be looking into it oncemore in the coming week or so).

Also, the panel driver's broken? That's news to me. I compiled this patch against latest the pmOS kernel about 5 months ago and it was fine minus some graphical glitchiness in Plasma Mobile that wasn't there before.

TravMurav reviewed Jan 16, 2022

View reviewed changes

person4268 force-pushed the add-backlight branch 2 times, most recently from ae53db0 to 8678ba7 Compare January 17, 2022 09:31

person4268 marked this pull request as draft January 17, 2022 20:30

stephan-gh changed the base branch from master to backup/5.15/please-rebase March 14, 2022 08:39

person4268 force-pushed the add-backlight branch from 8678ba7 to 4d95eec Compare July 3, 2022 10:04

person4268 changed the base branch from backup/5.15/please-rebase to master July 3, 2022 10:05

person4268 force-pushed the add-backlight branch from 4d95eec to 83bc280 Compare July 3, 2022 10:20

person4268 marked this pull request as ready for review July 3, 2022 10:26

stephan-gh reviewed Jul 3, 2022

View reviewed changes

arch/arm64/boot/dts/qcom/apq8016-samsung-gtelwifiue.dts Outdated Show resolved Hide resolved

arch/arm64/boot/dts/qcom/apq8016-samsung-gtelwifiue.dts Outdated Show resolved Hide resolved

arch/arm64/boot/dts/qcom/apq8016-samsung-gtelwifiue.dts Outdated Show resolved Hide resolved

person4268 force-pushed the add-backlight branch from 83bc280 to ad295e9 Compare July 3, 2022 11:19

person4268 force-pushed the add-backlight branch from ad295e9 to bcaf8d3 Compare July 3, 2022 11:55

person4268 force-pushed the add-backlight branch 2 times, most recently from 5f53c62 to e485625 Compare July 7, 2022 04:23

person4268 added 2 commits July 7, 2022 00:33

video: backlight: Add support for Richtek RT8555 Backlight

36f8471

The Richtek RT8555 is a 6 channel LED driver that supports up to 35mA/LED. This is a backlight driver that drives ILED1. Signed-off-by: Michael Abood <[email protected]>

arm64: dts: qcom: samsung-gtelwifiue: Add RT8555 backlight driver

342a10d

This adds the RT8555 backlight driver to the device tree. Using i2c-gpio here is necessary, as the backlight ic is not on any i2c pins. Signed-off-by: Michael Abood <[email protected]>

person4268 force-pushed the add-backlight branch from e485625 to 342a10d Compare July 7, 2022 04:34

person4268 commented Jul 8, 2022

View reviewed changes

stephan-gh changed the base branch from master to backup/5.18/please-rebase September 29, 2022 11:59

stephan-gh changed the base branch from backup/5.18/please-rebase to msm8916/5.18 November 11, 2022 09:21

stephan-gh changed the base branch from msm8916/5.18 to old/msm8916/5.18 November 11, 2022 09:42

stephan-gh changed the title ~~Add Richtek RT8555 Backlight Support for samsung-gtelwifiue~~ WIP: Add Richtek RT8555 Backlight Support for samsung-gtelwifiue Nov 24, 2022

stephan-gh added the needs-upstreaming label Jan 9, 2023

stephan-gh changed the title ~~WIP: Add Richtek RT8555 Backlight Support for samsung-gtelwifiue~~ Add Richtek RT8555 Backlight Support for samsung-gtelwifiue Jan 30, 2023

stephan-gh marked this pull request as draft January 30, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Richtek RT8555 Backlight Support for samsung-gtelwifiue #217

Add Richtek RT8555 Backlight Support for samsung-gtelwifiue #217

person4268 commented Jan 16, 2022

TravMurav Jan 16, 2022

person4268 commented Jul 3, 2022 •

edited

Loading

stephan-gh Jul 3, 2022

person4268 Jul 3, 2022

person4268 commented Jul 3, 2022 •

edited

Loading

stephan-gh commented Jul 3, 2022

person4268 commented Jul 3, 2022

person4268 commented Jul 7, 2022 •

edited

Loading

stephan-gh commented Jul 7, 2022

person4268 commented Jul 7, 2022 •

edited

Loading

stephan-gh commented Jul 8, 2022

person4268 commented Jul 8, 2022 •

edited

Loading

person4268 left a comment •

edited

Loading

person4268 Jul 3, 2022

person4268 Jul 3, 2022

RDS5 commented Jul 21, 2024

person4268 commented Jul 21, 2024

Add Richtek RT8555 Backlight Support for samsung-gtelwifiue #217

Are you sure you want to change the base?

Add Richtek RT8555 Backlight Support for samsung-gtelwifiue #217

Conversation

person4268 commented Jan 16, 2022

TravMurav Jan 16, 2022

Choose a reason for hiding this comment

person4268 commented Jul 3, 2022 • edited Loading

stephan-gh Jul 3, 2022

Choose a reason for hiding this comment

person4268 Jul 3, 2022

Choose a reason for hiding this comment

person4268 commented Jul 3, 2022 • edited Loading

stephan-gh commented Jul 3, 2022

person4268 commented Jul 3, 2022

person4268 commented Jul 7, 2022 • edited Loading

stephan-gh commented Jul 7, 2022

person4268 commented Jul 7, 2022 • edited Loading

stephan-gh commented Jul 8, 2022

person4268 commented Jul 8, 2022 • edited Loading

person4268 left a comment • edited Loading

Choose a reason for hiding this comment

person4268 Jul 3, 2022

Choose a reason for hiding this comment

person4268 Jul 3, 2022

Choose a reason for hiding this comment

RDS5 commented Jul 21, 2024

person4268 commented Jul 21, 2024

person4268 commented Jul 3, 2022 •

edited

Loading

person4268 commented Jul 3, 2022 •

edited

Loading

person4268 commented Jul 7, 2022 •

edited

Loading

person4268 commented Jul 7, 2022 •

edited

Loading

person4268 commented Jul 8, 2022 •

edited

Loading

person4268 left a comment •

edited

Loading