Add Anbernic RG Vita-Pro
Subsystems (6)
v6 cover Add Anbernic RG Vita-Pro
From: Chris Morgan <[email protected]>
Add support for the Anbernic RG Vita-Pro, based on the Rockchip RK3576 SoC.
Changes Since V1:
- Updated documentation filename to matche compatible string.
- Corrected order of allocating IRQ in charger driver and switched to using devm managed function to allocate work queue.
Changes Since V2:
- Corrected issue with naming of panel binding document and added fallback compatible string.
- Switched to devm helpers in panel driver. Note that devm_drm_panel_add() is currently only in the drm-next branch.
- Optimized setting GPIOs for panel driver.
Changes Since V3:
- Fixed numerous issues identifed by Sashiko bot, mainly relating to unclamped or improperly clamped values from within the charger driver.
- Corrected hard-coded values in the panel driver unprepare routine as well as correcting an improperly checked return condition.
Changes Since V4:
- Fixed errors identified by Sashiko bot on charger driver that could result in an improper value being written to the registers.
- Corrected some error handling in the panel driver.
Changes Since V5:
- Corrected minor errors in documentation and added default values.
- Further corrected clamping behavior in charger driver.
Chris Morgan (6):
dt-bindings: power: supply: sgm41542: document sgm41542
power: supply: sgm41542: Add SG Micro sgm41542 charger
dt-bindings: display: panel: Add Anbernic TD4310 panel drm/panel: anbernic-td4310: Add RG Vita Pro panel
dt-bindings: arm: rockchip: Add Anbernic RG Vita-Pro
arm64: dts: rockchip: Add Anbernic RG Vita-Pro
.../devicetree/bindings/arm/rockchip.yaml | 6 + .../display/panel/anbernic,td4310.yaml | 66 + .../power/supply/sgmicro,sgm41542.yaml | 96 ++ arch/arm64/boot/dts/rockchip/Makefile | 1 + .../rockchip/rk3576-anbernic-rg-vita-pro.dts | 1327 +++++++++++++++++ drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-anbernic-td4310.c | 257 ++++ drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/sgm41542_charger.c | 1057 +++++++++++++ 11 files changed, 2830 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml create mode 100644 Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml create mode 100644 arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts create mode 100644 drivers/gpu/drm/panel/panel-anbernic-td4310.c create mode 100644 drivers/power/supply/sgm41542_charger.c
Signature · 1 line
2.43.0
PATCH v6 1/6 dt-bindings: power: supply: sgm41542: document sgm41542
From: Chris Morgan <[email protected]>
Document the SG Micro sgm41542 battery charger/boost converter. The parameters of input-current-limit-microamp and input-voltage-limit-microvolt are defined as such since they are in common use among multiple bindings currently.
--- .../power/supply/sgmicro,sgm41542.yaml | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml
Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml new file +96 -0
@@ -0,0 +1,96 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/supply/sgmicro,sgm41542.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SGM41542 Battery Charger + +description: + The SGMicro SGM41542 is a single cell battery charger/boost controller. + +maintainers: + - Chris Morgan <[email protected]> + - Xu Shengfei <[email protected]> + +allOf: + - $ref: power-supply.yaml# + +properties: + compatible: + const: sgmicro,sgm41542 + + input-current-limit-microamp: + default: 2400000 + minimum: 100000 + maximum: 3800000 + + input-voltage-limit-microvolt: + default: 4500000 + minimum: 3900000 + maximum: 12000000 + + interrupts: + maxItems: 1 + + monitored-battery: + description: | + The charger uses the following battery properties + constant-charge-current-max-microamp (default 2040000) + constant-charge-voltage-max-microvolt (default 4208000) + charge-term-current-microamp (default 180000) + precharge-current-microamp (default 180000) + + reg: + maxItems: 1 + + regulators: + type: object + properties: + otg-vbus: + type: object + description: OTG boost regulator + $ref: /schemas/regulator/regulator.yaml + unevaluatedProperties: false + additionalProperties: false + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + battery: battery { + compatible = "simple-battery"; + constant-charge-current-max-microamp = <10000000>; + constant-charge-voltage-max-microvolt = <4350000>; + precharge-current-microamp = <180000>; + charge-term-current-microamp = <300000>; + }; + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + charger@3b { + compatible = "sgmicro,sgm41542"; + reg = <0x3b>; + input-current-limit-microamp = <3000000>; + input-voltage-limit-microvolt = <4500000>; + interrupt-parent = <&gpio0>; + interrupts = <26 IRQ_TYPE_EDGE_FALLING>; + monitored-battery = <&battery>; + + regulators { + otg-vbus { + regulator-max-microvolt = <5000000>; + regulator-min-microvolt = <5000000>; + }; + }; + }; + };
Signature · 1 line
2.43.0
Re: [PATCH V6 1/6] dt-bindings: power: supply: sgm41542: document sgm41542
pw-bot: not-applicable
PATCH v6 2/6 power: supply: sgm41542: Add SG Micro sgm41542 charger
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add support for the SG Micro SGM41542 charger/boost converter. Driver was adapted from Rockchip BSP driver [1] and confirmed with vendor datasheet [2].
[1] https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
[2] https://www.sg-micro.de/rect/assets/1e8de70b-657e-4156-be68-a64fdbe8e418/SGM41541_SGM41542.pdf
--- drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/sgm41542_charger.c | 1057 +++++++++++++++++++++++ 3 files changed, 1066 insertions(+) create mode 100644 drivers/power/supply/sgm41542_charger.c
drivers/power/supply/Kconfig +8 -0
@@ -856,6 +856,14 @@ config CHARGER_RK817 help Say Y to include support for Rockchip RK817 Battery Charger. +config CHARGER_SGM41542 + tristate "SGM41542 charger driver" + depends on I2C + depends on GPIOLIB || COMPILE_TEST + select REGMAP_I2C + help + Say Y to enable support for the SGM41542 battery charger. + config CHARGER_SMB347 tristate "Summit Microelectronics SMB3XX Battery Charger" depends on I2C
drivers/power/supply/Makefile +1 -0
@@ -107,6 +107,7 @@ obj-$(CONFIG_CHARGER_BQ25890) += bq25890_charger.o obj-$(CONFIG_CHARGER_BQ25980) += bq25980_charger.o obj-$(CONFIG_CHARGER_BQ256XX) += bq256xx_charger.o obj-$(CONFIG_CHARGER_RK817) += rk817_charger.o +obj-$(CONFIG_CHARGER_SGM41542) += sgm41542_charger.o obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o
drivers/power/supply/sgm41542_charger.c new file +1057 -0
1,057 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V6 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] Invalid pointer dereference of I2C client device during IRQ wake teardown. - [High] Watchdog timer…
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Invalid pointer dereference of I2C client device during IRQ wake teardown.
- [High] Watchdog timer bits are always written as zero, silently permanently disabling the watchdog.
- [High] Hardware watchdog will incorrectly trigger and reset device limits during system suspend.
- [High] TOCTOU race in probe causes integer underflow that applies extreme maximum current limits.
- [High] Ignored return value of enable_irq_wake() results in unbalanced decrement during teardown.
- [Medium] Ignored return value from devm_add_action_or_reset() leaves device without expected wakeup capabilities.
- [Medium] Unprotected concurrent writes to the unused sgm->state field causes a data race.
--
commit a4ca3f3960debe0f5d998a1b64ed909e6c09a1bf
Author: Chris Morgan <[email protected]>
Subject: power: supply: sgm41542: Add SG Micro sgm41542 charger
This commit adds support for the SG Micro SGM41542 charger and boost converter. The driver is adapted from the Rockchip BSP and confirmed with the vendor datasheet.
Quoted 6 lines
drivers/power/supply/sgm41542_charger.c new file +0
-0@@ -0,0 +1,1057 @@
[ ... ]
Quoted 15 lines
drivers/power/supply/sgm41542_charger.c new file +15
-0+static int sgm4154x_set_input_curr_lim (struct sgm4154x_device *sgm,int iindpm) +{ +int reg_val; +int ret; + +if (iindpm <SGM4154X_IINDPM_I_MIN_UA ) +return -EINVAL ; + +/* +* Per the datasheet, values between 100000uA and 3100000uA work +* as expected with the register defined as having a step of +* 100000 and a min/max of 100000 (0x00) through 3100000 (0x1e). +* The register value of 0x1f however corresponds to 3800000uA not +* 3200000uA as one would expect. +*/
drivers/power/supply/sgm41542_charger.c line 389
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+if ((iindpm >SGM4154X_IINDPM_I_MAX_UA ) || (iindpm > sgm->init_data.ilim)) + iindpm =min (SGM4154X_IINDPM_I_MAX_UA , sgm->init_data.ilim); + +if (iindpm >3100000 && iindpm <SGM4154X_IINDPM_I_MAX_UA ) + iindpm =3100000 ; + +if (iindpm ==SGM4154X_IINDPM_I_MAX_UA ) + reg_val =0x1f ; +else + reg_val = (iindpm -SGM4154X_IINDPM_I_MIN_UA ) /SGM4154X_IINDPM_STEP_UA ;
[Severity: High]
Does this logic underflow if called early?
If sysfs limits are set before sgm4154x_hw_init() runs in probe, sgm->init_data.ilim will still be 0. This would cause iindpm to be clamped to 0, and the step calculation above would evaluate to:reg_val = (0 - 100000) / 100000 = -1
When passed to regmap_update_bits(), the -1 will be masked against 0x1F, resulting in 0x1F. This would inadvertently apply the maximum 3.8A limit to the upstream power supply instead of the lowest limit.
Quoted 5 lines
drivers/power/supply/sgm41542_charger.c new file +5
-0+ + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_0 , +SGM4154X_IINDPM_I_MASK , + reg_val);
[ ... ]
drivers/power/supply/sgm41542_charger.c line 448
Quoted 11 lines
drivers/power/supply/sgm41542_charger.c new file +11
-0+static int sgm4154x_set_watchdog_timer (struct sgm4154x_device *sgm,u8 time) +{ +int ret; + +if (time >3 ) +return -EINVAL ; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_5 , +SGM4154X_WDT_TIMER_MASK , + time);
[Severity: High]
Since SGM4154X_WDT_TIMER_MASK spans bits 4 and 5 (GENMASK(5, 4)), shouldn't the time parameter (values 0-3) be shifted left by 4 before updating the register?
Passing the unshifted value here causes the regmap_update_bits masking logic to evaluate to (time & 0x30), which is always 0. This effectively disables the hardware watchdog permanently.
Quoted 2 lines
drivers/power/supply/sgm41542_charger.c new file +2
-0+ +if (ret) {
[ ... ]
Quoted 4 lines
drivers/power/supply/sgm41542_charger.c new file +4
-0+static int sgm4154x_charger_get_property (struct power_supply *psy, +enum power_supply_property psp, +union power_supply_propval *val) +{
drivers/power/supply/sgm41542_charger.c line 630
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+struct sgm4154x_device *sgm =power_supply_get_drvdata (psy); +struct sgm4154x_state state; +int ret; + + ret =sgm4154x_get_state (sgm, &state); +if (ret) { +dev_err (sgm->dev,"get state error!\n" ); +return ret; + } + sgm->state = state;
[Severity: Medium]
Could this concurrent, unprotected write to sgm->state cause a data race between sysfs readers and the IRQ handler below?
Since the sgm->state field is completely unused elsewhere in the driver, can we safely remove it entirely to avoid the regression?
Quoted 2 lines
drivers/power/supply/sgm41542_charger.c new file +2
-0+ +switch (psp) {
[ ... ]
drivers/power/supply/sgm41542_charger.c line 711
Quoted 12 lines
drivers/power/supply/sgm41542_charger.c new file +12
-0+static irqreturn_t sgm4154x_irq_handler_thread (int irq,void *private) +{ +struct sgm4154x_device *sgm4154x = private; +struct sgm4154x_state state; +int ret; + + ret =sgm4154x_get_state (sgm4154x, &state); +if (ret) { +dev_err (sgm4154x->dev,"get state error!\n" ); +return IRQ_NONE ; + } + sgm4154x->state = state;
[Severity: Medium]
Could this concurrent, unprotected write to sgm4154x->state cause a data race?
Quoted 1 line
drivers/power/supply/sgm41542_charger.c new file +1
-0+if (state.vbus_gd) {
[ ... ]
drivers/power/supply/sgm41542_charger.c line 964
Quoted 6 lines
drivers/power/supply/sgm41542_charger.c new file +6
-0+static void sgm4154x_disable_irq_wake (void *data) +{ +struct sgm4154x_device *sgm = data; +struct i2c_client *client =to_i2c_client (sgm->dev->parent); + +disable_irq_wake (client->irq);
[Severity: High]
Is it possible this casts the wrong device pointer?
sgm->dev is already the I2C client's device, so sgm->dev->parent points to the upstream I2C adapter. Casting the adapter via to_i2c_client() and accessing client->irq will read arbitrary memory, potentially corrupting a random IRQ descriptor's wake state when passed to disable_irq_wake().
Quoted 1 line
+}
[ ... ]
Quoted 5 lines
drivers/power/supply/sgm41542_charger.c new file +5
-0+static int sgm4154x_probe (struct i2c_client *client) +{ +struct device *dev = &client->dev; +struct sgm4154x_device *sgm; +int ret;
[ ... ]
drivers/power/supply/sgm41542_charger.c line 993
Quoted 2 lines
drivers/power/supply/sgm41542_charger.c new file +2
-0+ sgm->sgm_monitor_wq =devm_alloc_ordered_workqueue (dev,"sgm-monitor-wq" , +WQ_MEM_RECLAIM |WQ_FREEZABLE );
[Severity: High]
Is it safe to use a freezable workqueue here without providing PM suspend callbacks to disable the hardware watchdog?
If the system suspends while the watchdog is active, the workqueue will pause and the watchdog will expire, resetting the charger configuration and clearing the thermal/current limits established during probe.
drivers/power/supply/sgm41542_charger.c line 1006
Quoted 13 lines
drivers/power/supply/sgm41542_charger.c new file +13
-0+if (!sgm->sgm_monitor_wq) +return -EINVAL ; + + ret =devm_delayed_work_autocancel (dev, &sgm->sgm_delay_work, + sgm_charger_work); +if (ret) +return dev_err_probe (dev, ret,"Unable to register delayed work\n" ); + + ret =sgm4154x_power_supply_init (sgm, dev); +if (ret) +return dev_err_probe (dev, ret,"Failed to register power supply\n" ); + + ret =sgm4154x_hw_init (sgm);
[Severity: High]
Does registering the power supply earlier expose the sysfs interface to userspace before sgm4154x_hw_init() has populated the limits in sgm->init_data?
If userspace writes to the input limit early, the uninitialized zeros will be used in sgm4154x_set_input_curr_lim() which may trigger the underflow mentioned above.
drivers/power/supply/sgm41542_charger.c line 1019
Quoted 13 lines
drivers/power/supply/sgm41542_charger.c new file +13
-0+if (ret) +return dev_err_probe (dev, ret,"Cannot initialize the chip.\n" ); + +if (client->irq) { + ret =devm_request_threaded_irq (dev, client->irq,NULL , + sgm4154x_irq_handler_thread, +IRQF_TRIGGER_FALLING | +IRQF_ONESHOT , +"sgm41542-irq" , sgm); +if (ret) +return ret; + +enable_irq_wake (client->irq);
[Severity: High]
If the IRQ controller doesn't support wakeup and enable_irq_wake() fails, does ignoring the error here result in an unbalanced disable_irq_wake() call during teardown?
drivers/power/supply/sgm41542_charger.c line 1020
Quoted 1 line
drivers/power/supply/sgm41542_charger.c new file +1
-0+devm_add_action_or_reset (dev, sgm4154x_disable_irq_wake, sgm);
[Severity: Medium]
Also, does ignoring the return value of devm_add_action_or_reset() risk leaving the device without its teardown handler if the devm allocation fails?
Quoted 1 line
drivers/power/supply/sgm41542_charger.c new file +1
-0+ }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH V6 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger
In reply to Chris MorganHi,
Quoted 15 lines · Chris Morgan
Add support for the SG Micro SGM41542 charger/boost converter. Driver was adapted from Rockchip BSP driver [1] and confirmed with vendor datasheet [2].
[1] https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
[2] https://www.sg-micro.de/rect/assets/1e8de70b-657e-4156-be68-a64fdbe8e418/SGM41541_SGM41542.pdfSigned-off-by Chris Morgan <[email protected]>--- drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/sgm41542_charger.c | 1057 +++++++++++++++++++++++ 3 files changed, 1066 insertions(+) create mode 100644 drivers/power/supply/sgm41542_charger.c[...]
drivers/power/supply/sgm41542_charger.c line 521
Quoted 8 lines
drivers/power/supply/sgm41542_charger.c new file +8
-0+static int sgm4154x_set_recharge_volt_ua (struct sgm4154x_device *sgm,int recharge_volt) +{ +int reg_val; +int ret; + + recharge_volt = recharge_volt /1000 ; + + reg_val = (recharge_volt -SGM4154X_VRECHRG_OFFSET_MV ) /SGM4154X_VRECHRG_STEP_MV ;
These are the only millivolt offset/steps, so drop the 1000 divison and simply do:
#define SGM4154X_VRECHRG_OFFSET_UV 100000
#define SGM4154X_VRECHRG_STEP_UV 100000
reg_val = (recharge_volt - SGM4154X_VRECHRG_OFFSET_UV) / SGM4154X_VRECHRG_STEP_UV;
Otherwise LGTM.
Quoted 1 line
[...]
Greetings,
-- Sebastian
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v6 3/6 dt-bindings: display: panel: Add Anbernic TD4310 panel
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
The panel used by Anbernic in the RG Vita-Pro is a DSI panel based on the TD4310 controller IC. It measures approximately 5.5 inches diagonally and is 1080x1920 in resolution.
--- .../display/panel/anbernic,td4310.yaml | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml new file +66 -0
@@ -0,0 +1,66 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/anbernic,td4310.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Anbernic TD4310 Based Panels + +maintainers: + - Chris Morgan <[email protected]> + +description: + Anbernic TD4310 Based Panels, such as the RG-Vita-Pro panel + (a 1080x1920 5.5 inch panel). + +allOf: + - $ref: panel-common.yaml# + +properties: + compatible: + items: + - enum: + - anbernic,panel-vita-pro + - const: anbernic,td4310 + + reg: + maxItems: 1 + + vdd-supply: + description: Panel power supply + +required: + - compatible + - port + - reg + - reset-gpios + - vdd-supply + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + dsi { + #address-cells = <1>; + #size-cells = <0>; + + panel@0 { + compatible = "anbernic,panel-vita-pro", "anbernic,td4310"; + reg = <0>; + backlight = <&backlight>; + enable-gpios = <&gpio0 9 GPIO_ACTIVE_HIGH>; + reset-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>; + rotation = <270>; + vdd-supply = <&vdd_lcd>; + + port { + endpoint { + remote-endpoint = <&dsi_out>; + }; + }; + }; + }; + +...
Signature · 1 line
2.43.0
PATCH v6 4/6 drm/panel: anbernic-td4310: Add RG Vita Pro panel
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
The panel used by Anbernic in the RG Vita-Pro is a DSI panel based on the TD4310 controller IC. It measures approximately 5.5 inches diagonally and is 1080x1920 in resolution.
--- drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-anbernic-td4310.c | 257 ++++++++++++++++++ 3 files changed, 268 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-anbernic-td4310.c
drivers/gpu/drm/panel/Kconfig +10 -0
@@ -17,6 +17,16 @@ config DRM_PANEL_ABT_Y030XX067A Y030XX067A 320x480 3.0" panel as found in the YLM RG-280M, RG-300 and RG-99 handheld gaming consoles. +config DRM_PANEL_ANBERNIC_TD4310 + tristate "Anbernic TD4310 LCD panel" + depends on GPIOLIB && OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + help + Say Y here to enable support for Anbernic designed panels with the + TD4310 panel controller such as the ones used on the Anbernic RG + Vita Pro. + config DRM_PANEL_ARM_VERSATILE tristate "ARM Versatile panel driver" depends on OF
drivers/gpu/drm/panel/Makefile +1 -0
@@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_DRM_PANEL_ABT_Y030XX067A) += panel-abt-y030xx067a.o +obj-$(CONFIG_DRM_PANEL_ANBERNIC_TD4310) += panel-anbernic-td4310.o obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o obj-$(CONFIG_DRM_PANEL_ASUS_Z00T_TM5P5_NT35596) += panel-asus-z00t-tm5p5-n35596.o obj-$(CONFIG_DRM_PANEL_AUO_A030JTN01) += panel-auo-a030jtn01.o
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +257 -0
257 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V6 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Requesting `reset_gpio` as `GPIOD_OUT_LOW` in probe backpowers the panel, and missing reset…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Requesting reset_gpio as GPIOD_OUT_LOW in probe backpowers the panel, and missing reset assertion in the prepare error path leaves the panel backpowered.
- [Low] The driver calls a non-existent function devm_drm_panel_add(), causing a compilation failure.
--
commit 0a4500b372d995ebf665c9896430451a3e287a41
Author: Chris Morgan <[email protected]>
drm/panel: anbernic-td4310: Add RG Vita Pro panel
The panel used by Anbernic in the RG Vita-Pro is a DSI panel based on the TD4310 controller IC.
Quoted 4 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +0
-0
[ ... ]
Quoted 2 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +2
-0+static int panel_anbernic_td4310_prepare (struct drm_panel *panel) +{
[ ... ]
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 98
Quoted 11 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +11
-0+if (dsi_ctx.accum_err) { + ret = dsi_ctx.accum_err; + goto err_reset; + } + +return 0 ; + +err_reset: +gpiod_set_value_cansleep (ctx->enable_gpio,0 ); +err_enable: +regulator_disable (ctx->vdd);
[Severity: High]
The err_reset path disables the VDD regulator without re-asserting the reset GPIO (setting it to logical 1 / physical LOW). Could this leave the unpowered panel backpowered through its IO lines if the prepare sequence fails?
Quoted 2 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +2
-0+return ret; +}
[ ... ]
Quoted 2 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +2
-0+static int panel_anbernic_td4310_probe (struct mipi_dsi_device *dsi) +{
[ ... ]
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 172
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +5
-0+ ret =of_drm_get_panel_orientation (dev->of_node, &ctx->orientation); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get panel orientation\n" ); + + ctx->reset_gpio =devm_gpiod_get (dev,"reset" ,GPIOD_OUT_LOW );
[Severity: High]
Assuming the reset GPIO is configured as active-low in the device tree, requesting it with GPIOD_OUT_LOW sets its logical value to 0, which drives the physical pin HIGH.
Because the panel's VDD supply is off at probe time, will driving the reset pin HIGH backpower the panel via its IO protection diodes?
Quoted 3 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +3
-0+if (IS_ERR (ctx->reset_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->reset_gpio), +"Cannot get reset gpio\n" );
[ ... ]
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 198
Quoted 7 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +7
-0+ dsi->mode_flags = ctx->panel_info->mode_flags; + + ret =drm_panel_of_backlight (&ctx->panel); +if (ret) +return ret; + +devm_drm_panel_add (dev, &ctx->panel);
[Severity: Low]
Is devm_drm_panel_add() defined in the DRM panel API? It appears only drm_panel_add() and devm_drm_panel_add_follower() are available, which might cause an implicit declaration build error here.
Quoted 7 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +7
-0+ + ret =devm_mipi_dsi_attach (dev, dsi); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to attach to DSI host\n" ); + +return 0 ; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
PATCH v6 5/6 dt-bindings: arm: rockchip: Add Anbernic RG Vita-Pro
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add compatible string for the Anbernic RG Vita-Pro.
--- Documentation/devicetree/bindings/arm/rockchip.yaml | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/arm/rockchip.yaml +6 -0
@@ -66,6 +66,12 @@ properties: - anbernic,rg-ds - const: rockchip,rk3568 + - description: Anbernic RK3576 Handheld Gaming Console + items: + - enum: + - anbernic,rg-vita-pro + - const: rockchip,rk3576 + - description: Ariaboard Photonicat items: - const: ariaboard,photonicat
Signature · 1 line
2.43.0
PATCH v6 6/6 arm64: dts: rockchip: Add Anbernic RG Vita-Pro
In reply to Chris MorganFrom: Chris Morgan <[email protected]>
Add device tree for the Anbernic RG Vita-Pro, based on the Rockchip RK3576. All hardware is currently working except for the accelerometer, the touchscreen, the battery monitor, and DisplayPort over USB-C. PWM is also missing currently, and as a result the LEDs and panel backlight are currently controlled via GPIO but will be changed to PWM once supported.
The Anbernic RG Vita-Pro includes the following hardware:
- A 1080x1920 DSI display with touch.
- 21 buttons.
- 2 Analog joysticks controlled via userspace.
- 3 LEDs.
- 64GB eMMC, 2 SDMMC slots.
- RTL8852BE WiFi (with Bluetooth via UART)
- 1 USB 3.0 USB-C port in OTG mode.
- 3.5mm headphone jack with play button support.
- 5000mAH battery
The following hardware has incomplete driver support and is not yet working:
- An Invensense icm42607p accelerometer.
- A Cellwise cw221x battery monitor.
- A Synaptics i2c touchscreen.
- DisplayPort over USB-C alt-mode.
- PWM controller.
--- arch/arm64/boot/dts/rockchip/Makefile | 1 + .../rockchip/rk3576-anbernic-rg-vita-pro.dts | 1327 +++++++++++++++++ 2 files changed, 1328 insertions(+) create mode 100644 arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
arch/arm64/boot/dts/rockchip/Makefile +1 -0
@@ -165,6 +165,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-display-vz.dtbo dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-wolfvision-pf5-io-expander.dtbo dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-100ask-dshanpi-a1.dtb +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-anbernic-rg-vita-pro.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-armsom-sige5.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-armsom-sige5-v1.2-wifibt.dtbo dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3576-evb1-v10.dtb
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +1327 -0
1,327 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V6 6/6] arm64: dts: rockchip: Add Anbernic RG Vita-Pro
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The SPI joysticks are inaccessible from userspace due to a missing SPI child node. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The SPI joysticks are inaccessible from userspace due to a missing SPI child node. --
commit 2097a3f0d4026d4220ee53d9f1d7508fedf974d2
Author: Chris Morgan <[email protected]>
arm64: dts: rockchip: Add Anbernic RG Vita-Pro
This commit adds a device tree for the Anbernic RG Vita-Pro based on the Rockchip RK3576. It sets up definitions and configurations for various hardware components like display, buttons, battery, and networking.
Quoted 6 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +0
-0@@ -0,0 +1,1327 @@
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 1236
Quoted 7 lines
+&spi1 {
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +6
-0+ num-cs = <1>; + pinctrl-0 = <&spi1m2_pins>, <&spi1m2_csn0>; + status = "okay"; + + /* SPI controlled MCU for joystick and joystick LEDs. */ +};
[Severity: High]
The commit message mentions "2 Analog joysticks controlled via userspace", and this comment notes an SPI controlled MCU for the joystick. However, there is no child node defined under this &spi1 controller.
Without a child node binding to a driver, the SPI core will not instantiate a logical device, preventing userspace from communicating with the joysticks. Could a spidev or similar child node be added here to expose the interface?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6