auxdisplay: vk2c21: add sysfs brightness control#501
Draft
rpardini wants to merge 3 commits into
Draft
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe VK2C21 auxdisplay driver gains writable brightness control with Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rpardini
force-pushed
the
meko-r58x-pro-LCD-brightness-claude
branch
from
June 14, 2026 09:52
6868e35 to
2141aee
Compare
Member
Author
|
Tested on-hardware, actually works |
rpardini
force-pushed
the
meko-r58x-pro-LCD-brightness-claude
branch
from
July 7, 2026 12:23
2141aee to
00d8614
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
drivers/input/remotectl/rockchip_pwm_remotectl.c (1)
935-950: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating IRQ index by known pwm_version instead of blind try/fallback.
ddata->pwm_data(and thuspwm_version) is already available at this point (set at line 868), but the version check only happens later at line 953. Deciding the IRQ index based onddata->pwm_data->pwm_version >= 4would make the intent explicit and avoid relying on DTS resource-count side effects to distinguish v4 from older chips.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/input/remotectl/rockchip_pwm_remotectl.c` around lines 935 - 950, The IRQ selection in the remotes control probe is currently using a blind try/fallback between platform_get_irq indices, which hides the actual PWM generation-specific intent. Update the IRQ lookup in rockchip_pwm_remotectl.c to choose index 1 only when ddata->pwm_data->pwm_version indicates PWM v4 or newer, and use index 0 for older versions, using the existing ddata->pwm_data/pwm_version available before the later version check. Keep the existing error handling in the probe path, but make the index decision explicit in the code near the IRQ assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drivers/input/remotectl/rockchip_pwm_remotectl.c`:
- Around line 943-945: The IRQ lookup in the remotectl probe path is logging an
expected failure because the speculative index-1 fetch is treated as an error
before falling back to IRQ 0. Update the logic around platform_get_irq in
rockchip_pwm_remotectl so the first lookup uses platform_get_irq_optional() (or
equivalent non-erroring optional lookup), then keep the existing fallback to
platform_get_irq(pdev, 0) for the real required IRQ path.
---
Nitpick comments:
In `@drivers/input/remotectl/rockchip_pwm_remotectl.c`:
- Around line 935-950: The IRQ selection in the remotes control probe is
currently using a blind try/fallback between platform_get_irq indices, which
hides the actual PWM generation-specific intent. Update the IRQ lookup in
rockchip_pwm_remotectl.c to choose index 1 only when
ddata->pwm_data->pwm_version indicates PWM v4 or newer, and use index 0 for
older versions, using the existing ddata->pwm_data/pwm_version available before
the later version check. Keep the existing error handling in the probe path, but
make the index decision explicit in the code near the IRQ assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0accab6-db61-4908-b662-010dbde2a4d8
📒 Files selected for processing (3)
arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588-nas.dtsdrivers/auxdisplay/lcd-vk2c21.cdrivers/input/remotectl/rockchip_pwm_remotectl.c
🚧 Files skipped from review as they are similar to previous changes (1)
- drivers/auxdisplay/lcd-vk2c21.c
On PWM v4 (RK3588), channel 3 of each PWM controller block exposes two interrupts: a group IRQ at index 0 shared with channels 0/1/2 of the same block, and a dedicated capture IRQ at index 1 intended for IR receive / power-key wakeup use of channel 3. The driver always grabbed index 0 and requested it without IRQF_SHARED, which collides with pwm-rockchip when another channel in the same block is enabled (e.g. pwm1 for a fan on the FriendlyElec CM3588-NAS). The in-tree v4 path in pwm-rockchip.c also omits IRQF_SHARED, so probe fails with -EBUSY: remotectl-pwm fd8b0030.pwm: cannot claim IRQ 28 remotectl-pwm: probe of fd8b0030.pwm failed with error -16 Prefer index 1 when present and fall back to index 0 for PWM v1-v3 nodes that only expose a single interrupt. The capture-mode handler (rockchip_pwm_irq_v4) is unchanged and remains correct for the dedicated IRQ. Signed-off-by: Ricardo Pardini <ricardo@pardini.net> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Expose a `brightness` sysfs attribute that drives the chip's IVASET
register (0x8A):
- "default" (the boot-time state): IVASET=0x0F, IVA off, bias driven
by the board's external VR resistor (same as before this change).
- 0..15: IVASET=0x3F..0x30 (IVA on, SEG mode);
15 = brightest (1.000 x VDD), 0 =
dimmest (0.529 x VDD at 1/3 bias).
SEG mode (0x30..0x3F) is used rather than VLCD-output mode (0x10..0x1F)
because the R58X-Pro ties the VLCD pin to VDD through an external
resistor: in VLCD-output mode the external circuit dominates the chip's
internal driver and the brightness register has no visible effect. In
SEG mode the chip ignores the VLCD pin and generates the bias voltage
purely internally, so the 16 levels actually take effect.
Probe-time behaviour is unchanged: init still sends IVA=0x0F, and the
attribute reads back as "default" until userspace writes a number.
Signed-off-by: Ricardo Pardini <ricardo@pardini.net>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The per-device statistics shown in /proc/mpp_service/load are only ever recomputed inside mpp_dev_load(), which is called exclusively from the task-completion paths (mpp_common.c task worker and the rkvdec2 link/ccu workers). Within that function the load and utilization figures are only refreshed when a task finishes *and* at least one load_interval has elapsed; the result is stored in mpp->load_info and reset for the next window. There is no timer, runtime-PM hook, or reader-side logic that ages the value out, so mpp->load_info.load is a snapshot of the last completed interval, not a live measurement. As a consequence, once a device stops receiving tasks its stored load is never updated again and /proc/mpp_service/load keeps reporting the last busy interval indefinitely (until load_interval is toggled, which clears the stats via mpp_dev_load_clear()). This is most visible on the standalone AV1 decoder (fdc70000.av1d). It is driven one task at a time by the default worker, so when playback stops the final frame's completion is genuinely the last event that will ever call mpp_dev_load() for that core, and its load freezes at the busy value. Multi-core rkvdec2 decoders in link/ccu mode keep draining their queued task list after playback ends, which happens to log a further, near-idle interval and pulls the figure back down, so the staleness goes unnoticed there. The underlying defect is common to every device. Rather than introduce a periodic recompute (a per-core timer with the associated runtime-PM interactions), detect the idle case in the reader. A device whose load_info has not been updated for more than one full load_interval has, by definition, completed no task in that window and is idle; report zero for it instead of the stale snapshot. Devices that never started load tracking (!load_en) are likewise reported as zero. Signed-off-by: Ricardo Pardini <ricardo@pardini.net> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rpardini
force-pushed
the
meko-r58x-pro-LCD-brightness-claude
branch
from
July 14, 2026 18:47
b34b784 to
7585a63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drivers/input/remotectl/rockchip_pwm_remotectl.c`:
- Around line 943-945: Update the IRQ lookup around platform_get_irq in the
probe path to use platform_get_irq_optional(pdev, 1), preserving -EPROBE_DEFER
and other errors. Fall back to platform_get_irq_optional(pdev, 0) only when the
first lookup returns -ENXIO, rather than for every negative result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 768ac27a-b307-4a29-874b-b8f73e4a5477
📒 Files selected for processing (3)
drivers/auxdisplay/lcd-vk2c21.cdrivers/input/remotectl/rockchip_pwm_remotectl.cdrivers/video/rockchip/mpp/mpp_service.c
🚧 Files skipped from review as they are similar to previous changes (1)
- drivers/auxdisplay/lcd-vk2c21.c
Comment on lines
+943
to
+945
| irq = platform_get_irq(pdev, 1); | ||
| if (irq < 0) | ||
| irq = platform_get_irq(pdev, 0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
platform_c="$(rg --files | rg '(^|/)drivers/base/platform\.c$' | head -n1)"
test -n "$platform_c"
rg -n -A35 'int platform_get_irq_optional' "$platform_c" |
rg -- '-EPROBE_DEFER|-ENXIO|platform_get_irq'Repository: armbian/linux-rockchip
Length of output: 2137
Don’t fall back on all IRQ errors here. platform_get_irq(pdev, 1) can return -EPROBE_DEFER; this "< 0" check would drop to IRQ 0 instead of deferring probe. Use platform_get_irq_optional(pdev, 1) and fall back only on -ENXIO.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@drivers/input/remotectl/rockchip_pwm_remotectl.c` around lines 943 - 945,
Update the IRQ lookup around platform_get_irq in the probe path to use
platform_get_irq_optional(pdev, 1), preserving -EPROBE_DEFER and other errors.
Fall back to platform_get_irq_optional(pdev, 0) only when the first lookup
returns -ENXIO, rather than for every negative result.
Source: MCP tools
Member
Author
|
Ooops, no, pushed stuff to wrong branch. Will cleanup in a bit |
rpardini
marked this pull request as draft
July 15, 2026 09:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
auxdisplay: vk2c21: add sysfs brightness control
SEG mode (0x30..0x3F) is used rather than VLCD-output mode (0x10..0x1F)
because the R58X-Pro ties the VLCD pin to VDD through an external
resistor: in VLCD-output mode the external circuit dominates the chip's
internal driver and the brightness register has no visible effect. In
SEG mode the chip ignores the VLCD pin and generates the bias voltage
purely internally, so the 16 levels actually take effect.
Probe-time behaviour is unchanged: init still sends IVA=0x0F, and the
attribute reads back as "default" until userspace writes a number.