Add Anbernic RG Vita-Pro
Subsystems (6)
v2 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.
Chris Morgan (6):
dt-bindings: power: supply: sgm41542: document sgm41542
power: supply: sgm41542: Add SG Micro sgm41542 charger
dt-bindings: display: panel: document 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 | 64 + .../power/supply/sgmicro,sgm41542.yaml | 99 ++ arch/arm64/boot/dts/rockchip/Makefile | 3 + .../rockchip/rk3576-anbernic-rg-vita-pro.dts | 1317 +++++++++++++++++ drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-anbernic-td4310.c | 291 ++++ drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/sgm41542_charger.c | 1035 +++++++++++++ 11 files changed, 2835 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 v2 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 | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml
Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml new file +99 -0
@@ -0,0 +1,99 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/supply/sgmicro,sgm41542-charger.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]> + +properties: + compatible: + const: sgmicro,sgm41542 + + input-current-limit-microamp: + description: + Optional value to clamp the maximum input current limit to for + the device. If omitted, the default value for the hardware will + be used (2400000). + minimum: 100000 + maximum: 3800000 + + input-voltage-limit-microvolt: + description: + Optional value to clamp the maximum input voltage limit to for + the device. If omitted, the default value for the hardware will + be used (4500000). + minimum: 3900000 + maximum: 12000000 + + interrupts: + maxItems: 1 + + monitored-battery: + description: | + A phandle to a monitored battery node. Values for the following + are used by the driver and if not present will result in default + values being applied: + constant-charge-current-max-microamp + constant-charge-voltage-max-microvolt + charge-term-current-microamp + precharge-current-microamp + + 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 + +additionalProperties: false + +examples: + - | + 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>; + }; + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + 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
[email protected]
AI reviewRe: [PATCH V2 1/6] dt-bindings: power: supply: sgm41542: document sgm41542
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `$id` field in the YAML binding does not match the file path, which breaks the schema…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The $id field in the YAML binding does not match the file path, which breaks the schema cross-reference system. --
commit e462413669a11b133f4d59f276a9c7d14b146226
Author: Chris Morgan <[email protected]>
dt-bindings: power: supply: sgm41542: document sgm41542
This commit documents the device tree bindings for the SG Micro sgm41542 single-cell battery charger and boost controller. It establishes properties for the hardware's input limits, interrupts, regulators, and monitored battery values.
Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml line 4
Quoted 10 lines
Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml new file +4
-0@@ -0,0 +1,99 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/supply/sgmicro,sgm41542-charger.yaml#
Does the $id path need to exactly match the file name? The file is named sgmicro,sgm41542.yaml, but the $id includes an extra -charger suffix.
A mismatch here can break the schema cross-reference system and cause dt_binding_check to silently skip validation.
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Rob Herring (Arm)
maintainerRe: [PATCH V2 1/6] dt-bindings: power: supply: sgm41542: document sgm41542
In reply to Chris MorganQuoted 7 lines · Chris Morgan
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.
Quoted 6 lines
Signed-off-by Chris Morgan <[email protected]>--- .../power/supply/sgmicro,sgm41542.yaml | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
$id: http://devicetree.org/schemas/power/supply/sgmicro,sgm41542-charger.yaml
file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.kernel.org/project/devicetree/patch/[email protected]
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
PATCH v2 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 | 1035 +++++++++++++++++++++++ 3 files changed, 1044 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 +1035 -0
1,035 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V2 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [Critical] Uninitialized pointer dereference causes kernel panic on missing battery info - [High] Missing…
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Uninitialized pointer dereference causes kernel panic on missing battery info
- [High] Missing .remove callback leads to delayed work timer Use-After-Free
- [High] Race condition exposing uninitialized structures to userspace
- [High] Input current limit configuration truncates 3.8A to 600mA
- [High] Input voltage limit calculation wrap-around incorrectly applies low voltage bounds
- [High] Charge voltage calculation maps 4.336V request to 4.352V, risking battery overcharge
- [Medium] Watchdog timer logic applies incorrect bitmask instead of integer duration
- [Low] Ignored devm_regulator_register return value masks initialization failures --
commit 974520d45c64c2d48e4b5e84a0082620a313d9d4
Author: Chris Morgan <[email protected]>
power: supply: sgm41542: Add SG Micro sgm41542 charger
Add support for the SG Micro SGM41542 charger/boost converter. Driver was adapted from Rockchip BSP driver and confirmed with vendor datasheet.
Quoted 5 lines
drivers/power/supply/sgm41542_charger.c new file +0
-0
[ ... ]
drivers/power/supply/sgm41542_charger.c line 264
Quoted 13 lines
drivers/power/supply/sgm41542_charger.c new file +13
-0+static int sgm4154x_set_chrg_volt (struct sgm4154x_device *sgm,int chrg_volt) +{ +int reg_val; +int ret; + +/* +* Note that the value of 0x01111 represents a "special value" +* corresponding to 4352000uV instead of the expected 4336000uV, +* per the datasheet. All other values are as expected. +*/ + chrg_volt =clamp (chrg_volt,SGM4154X_VREG_V_MIN_UV , sgm->init_data.max_vreg); + reg_val = (chrg_volt -SGM4154X_VREG_V_MIN_UV ) /SGM4154X_VREG_V_STEP_UV ; + reg_val = reg_val <<3 ;
Could this apply a charge voltage higher than requested?
If chrg_volt is exactly 4336000, reg_val becomes 15 (0x0f). According to the comment and sgm4154x_get_chrg_volt(), 0x0f maps to 4352000uV. Might this overcharge the battery by 16mV?
[ ... ]
Quoted 32 lines
drivers/power/supply/sgm41542_charger.c new file +32
-0+static int sgm4154x_set_input_volt_lim (struct sgm4154x_device *sgm, +unsigned int vindpm) +{ +enum SGM4154X_VINDPM_OS os_val; +unsigned int offset; +u8 reg_val; +int ret; + + +if (vindpm <SGM4154X_VINDPM_V_MIN_UV || + vindpm >SGM4154X_VINDPM_V_MAX_UV ) { +dev_err (sgm->dev,"input voltage limit %u outside range\n" , vindpm); +return -EINVAL ; + } + +if (vindpm <5900000 ) { + os_val =VINDPM_OS_3900MV ; + offset =3900000 ; + }else if (vindpm >=5900000 && vindpm <7500000 ) { + os_val =VINDPM_OS_5900MV ; + offset =5900000 ; + }else if (vindpm >=7500000 && vindpm <10500000 ) { + os_val =VINDPM_OS_7500MV ; + offset =7500000 ; + }else { + os_val =VINDPM_OS_10500MV ; + offset =10500000 ; + } + + ret = regmap_update_bits(sgm->regmap, + SGM4154X_CHRG_CTRL_F, + SGM4154X_VINDPM_OS_MASK,
drivers/power/supply/sgm41542_charger.c line 339
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ os_val); +if (ret) { +dev_err (sgm->dev,"set vin dpm error!\n" ); +return ret; + } + + reg_val = (vindpm - offset) /SGM4154X_VINDPM_STEP_UV ; + + ret =regmap_update_bits (sgm->regmap,SGM4154X_CHRG_CTRL_6 , +SGM4154X_VINDPM_V_MASK , reg_val);
Is it possible for reg_val to wrap around the register mask here?
If vindpm is 5800000, it falls into the first range (vindpm < 5900000). The calculation yields a reg_val of 19 (0x13). Since SGM4154X_VINDPM_V_MASK is only 4 bits (GENMASK(3, 0)), updating the bits will truncate it to 3. This would set the limit to 4.2V instead of clamping it to the highest supported value in this range.
[ ... ]
Quoted 17 lines
drivers/power/supply/sgm41542_charger.c new file +17
-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. +*/ + +if (iindpm >3100000 && iindpm <SGM4154X_IINDPM_I_MAX_UA )
drivers/power/supply/sgm41542_charger.c line 372
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ iindpm =3100000 ; +if ((iindpm >SGM4154X_IINDPM_I_MAX_UA ) || (iindpm > sgm->init_data.ilim)) + iindpm =min (SGM4154X_IINDPM_I_MAX_UA , sgm->init_data.ilim); + + reg_val = (iindpm -SGM4154X_IINDPM_I_MIN_UA ) /SGM4154X_IINDPM_STEP_UA ; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_0 , +SGM4154X_IINDPM_I_MASK , + reg_val);
Could this calculation truncate for the 3.8A setting?
If iindpm is exactly 3800000, reg_val evaluates to 37 (0x25). Since SGM4154X_IINDPM_I_MASK is GENMASK(4, 0) (0x1f), this truncates to 0x05. Might this configure the charger to 600mA instead of the intended 3.8A?
[ ... ]
Quoted 13 lines
drivers/power/supply/sgm41542_charger.c new file +13
-0+static int sgm4154x_set_watchdog_timer (struct sgm4154x_device *sgm,int time) +{ +u8 reg_val; +int ret; + +if (time ==0 ) + reg_val =SGM4154X_WDT_TIMER_DISABLE ; +else if (time ==40 ) + reg_val =SGM4154X_WDT_TIMER_40S ; +else if (time ==80 ) + reg_val =SGM4154X_WDT_TIMER_80S ; +else + reg_val =SGM4154X_WDT_TIMER_160S ;
[ ... ]
drivers/power/supply/sgm41542_charger.c line 574
Quoted 12 lines
drivers/power/supply/sgm41542_charger.c new file +12
-0+static int sgm4154x_charger_set_property (struct power_supply *psy, +enum power_supply_property prop, +const union power_supply_propval *val) +{ +struct sgm4154x_device *sgm =power_supply_get_drvdata (psy); +int ret = -EINVAL ; + +switch (prop) { +case POWER_SUPPLY_PROP_ONLINE : +if (val->intval) { + ret =sgm4154x_enable_charger (sgm); +sgm4154x_set_watchdog_timer (sgm,SGM4154X_WDT_TIMER_40S );
Will this pass the correct duration to sgm4154x_set_watchdog_timer()?
SGM4154X_WDT_TIMER_40S is defined as a bitmask (BIT(4), which is 16). Since the check inside sgm4154x_set_watchdog_timer() looks for exactly 40, might passing 16 fall through to the else branch and unintentionally set a 160-second watchdog?
[ ... ]
Quoted 22 lines
drivers/power/supply/sgm41542_charger.c new file +22
-0+static int sgm4154x_hw_init (struct sgm4154x_device *sgm) +{ +struct power_supply_battery_info *bat_info; +int ret; +u32 val; + +/* +* If unable to read devicetree info, use default/reset +* values from hardware. +*/ + sgm->init_data.iprechg =SGM4154X_PRECHRG_I_DEF_UA ; + sgm->init_data.iterm =SGM4154X_TERMCHRG_I_DEF_UA ; + sgm->init_data.max_ichg =SGM4154X_ICHRG_I_DEF_UA ; + sgm->init_data.max_vreg =SGM4154X_VREG_V_DEF_UV ; + sgm->init_data.vlim =SGM4154X_VINDPM_DEF_UV ; + sgm->init_data.ilim =SGM4154X_IINDPM_DEF_UA ; + + ret =power_supply_get_battery_info (sgm->charger, &bat_info); +if (ret) +dev_warn (sgm->dev,"sgm4154x: cannot read battery info\n" ); +else { +if (bat_info->constant_charge_current_max_ua)
drivers/power/supply/sgm41542_charger.c line 790
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ sgm->init_data.max_ichg = bat_info->constant_charge_current_max_ua; +if (bat_info->constant_charge_voltage_max_uv) + sgm->init_data.max_vreg = bat_info->constant_charge_voltage_max_uv; +if (bat_info->charge_term_current_ua) + sgm->init_data.iterm = bat_info->charge_term_current_ua; +if (bat_info->precharge_current_ua) + sgm->init_data.iprechg = bat_info->precharge_current_ua; + } + +power_supply_put_battery_info (sgm->charger, bat_info);
Does this code dereference an uninitialized pointer?
If power_supply_get_battery_info() returns an error, bat_info remains uninitialized. Calling power_supply_put_battery_info() with that pointer outside of the success block will try to free its internal tables using garbage data.
[ ... ]
Quoted 40 lines
drivers/power/supply/sgm41542_charger.c new file +40
-0+static int sgm4154x_probe (struct i2c_client *client) +{ +const struct i2c_device_id *id =i2c_client_get_device_id (client); +struct device *dev = &client->dev; +struct sgm4154x_device *sgm; +int ret; + + sgm =devm_kzalloc (dev,sizeof (*sgm),GFP_KERNEL ); +if (!sgm) +return -ENOMEM ; + + sgm->client = client; + sgm->dev = dev; + +strscpy (sgm->model_name, id->name,I2C_NAME_SIZE ); + + sgm->regmap =devm_regmap_init_i2c (client, &sgm4154x_regmap_config); +if (IS_ERR (sgm->regmap)) +return dev_err_probe (dev,PTR_ERR (sgm->regmap), +"Failed to allocate register map\n" ); + +i2c_set_clientdata (client, sgm); + + ret =sgm4154x_hw_chipid_detect (sgm); +if (ret) +dev_err_probe (dev, ret,"Unable to read HW ID\n" ); + +device_init_wakeup (dev,1 ); + + 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); +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,
drivers/power/supply/sgm41542_charger.c line 990
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+IRQF_TRIGGER_FALLING | +IRQF_ONESHOT , + "sgm41542-irq", sgm); +if (ret) +return ret; +enable_irq_wake (client->irq); + } + + sgm->sgm_monitor_wq =devm_alloc_ordered_workqueue (dev,"%s" , +WQ_MEM_RECLAIM |WQ_FREEZABLE ,"sgm-monitor-wq" );
Could this result in a race condition exposing uninitialized structures?
sgm4154x_power_supply_init() makes the sysfs interface available before the workqueue or sgm_delay_work are initialized here. If userspace writes to the ONLINE property in this small window, it triggers sgm4154x_set_watchdog_timer() which queues the uninitialized work to a NULL workqueue.
[ ... ]
drivers/power/supply/sgm41542_charger.c line 999
Quoted 4 lines
drivers/power/supply/sgm41542_charger.c new file +4
-0+sgm4154x_vbus_regulator_register (sgm); + +return 0 ; +}
Does this silently ignore regulator registration failures?
If sgm4154x_vbus_regulator_register() fails and returns an error code (like -EPROBE_DEFER), the return value is dropped and the function returns 0.
[ ... ]
drivers/power/supply/sgm41542_charger.c line 1029
Quoted 9 lines
drivers/power/supply/sgm41542_charger.c new file +9
-0+static struct i2c_driver sgm4154x_driver = { + .driver = { + .name ="sgm4154x-charger" , + .of_match_table = sgm4154x_of_match, + }, + .probe = sgm4154x_probe, + .shutdown = sgm4154x_charger_shutdown, + .id_table = sgm4154x_i2c_ids, +};
Is it possible for the delayed work timer to cause a Use-After-Free?
Since there is no .remove callback and the driver uses devm allocation for struct sgm4154x_device and the workqueue, devres will automatically free them when the module unbinds. However, the driver does not explicitly cancel the delayed work item (sgm->sgm_delay_work). If the timer fires after the driver structures are freed, it will dereference freed memory.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v2 3/6 dt-bindings: display: panel: document 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 | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml new file +64 -0
@@ -0,0 +1,64 @@ +# 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: + - const: anbernic,panel-vita-pro + + 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"; + 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 { + panel_in: endpoint { + remote-endpoint = <&dsi_out>; + }; + }; + }; + }; + +...
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V2 3/6] dt-bindings: display: panel: document Anbernic TD4310 panel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The YAML filename does not match the compatible string. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The YAML filename does not match the compatible string.
--
commit 5f66eb8e2fe14ce244cbb7ee82f13643cfae18e7
Author: Chris Morgan <[email protected]>
dt-bindings: display: panel: document Anbernic TD4310 panel
This commit introduces a new device tree binding for the TD4310-based panel used in the Anbernic RG Vita-Pro device.
Quoted 5 lines
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml new file +0
-0
[ ... ]
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml line 22
Quoted 4 lines
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml new file +4
-0+properties: + compatible: + items: + - const: anbernic,panel-vita-pro
This isn't a bug, but the YAML filename is anbernic,td4310.yaml while the only defined compatible string is anbernic,panel-vita-pro.
According to device tree schema guidelines, the YAML filename should generally match the compatible string.
Should the filename be renamed to anbernic,panel-vita-pro.yaml, or should anbernic,td4310 be added as a compatible string here?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH V2 3/6] dt-bindings: display: panel: document Anbernic TD4310 panel
In reply to Chris MorganDocumentation/devicetree/bindings/display/panel/anbernic,td4310.yaml line 22
Quoted 8 lines · Chris Morgan
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml new file +8
-0+ +allOf: + - $ref: panel-common.yaml# + +properties: + compatible: + items: + - const: anbernic,panel-vita-pro
You should have fallback compatible for the IC, at least that is the review I was consistently giving to others recently. Apologies if someone gave you opposite comment...
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml line 35
Quoted 13 lines
Documentation/devicetree/bindings/display/panel/anbernic,td4310.yaml new file +13
-0+ + reg: + maxItems: 1 + + vdd-supply: + description: Panel power supply + +required: + - compatible + - port + - reg + - reset-gpios + - vdd-supply
I guess this could go to panel-simple (after using power-supply instead of vdd), but it's fine as well.
Review
Quoted 2 lines
(quoted diff) +2
-0+ +unevaluatedProperties: false
Best regards,
Krzysztof
PATCH v2 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 | 291 ++++++++++++++++++ 3 files changed, 302 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 +291 -0
291 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V2 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Resource leak and hardware state mismatch on prepare error path. - [High] Enable GPIO is…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Resource leak and hardware state mismatch on prepare error path.
- [High] Enable GPIO is incorrectly asserted during probe before the VDD regulator is enabled.
- [Medium] Panel-specific delay configuration fields are ignored in favor of hardcoded values.
- [Low] Extraneous semicolon after if-statement block.
--
commit b88e27c6a433bac0f5f02ad289ba084e02f359ed
Author: Chris Morgan <[email protected]>
drm/panel: anbernic-td4310: Add RG Vita Pro panel
This commit adds support for the TD4310 DSI panel used in the Anbernic RG Vita Pro handheld console. It introduces a new panel driver with power sequencing, DCS initialization, and display mode definitions.
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +0
-0
[ ... ]
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+static int panel_anbernic_td4310_prepare (struct drm_panel *panel) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +struct mipi_dsi_device *dsi = ctx->dsi; +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; +int ret;
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 68
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+ + ret =regulator_enable (ctx->vdd); +if (ret) +return ret; + +if (ctx->enable_gpio) { + ret =gpiod_set_value_cansleep (ctx->enable_gpio,1 ); +if (ret) + goto err_enable; + };
This isn't a bug, but is there an extra semicolon here after the if block?
Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +8
-0+ +gpiod_set_value_cansleep (ctx->reset_gpio,0 ); +mipi_dsi_msleep (&dsi_ctx,10 ); + + ret =gpiod_set_value_cansleep (ctx->reset_gpio,1 ); +if (ret) + goto err_reset; +if (ctx->panel_info->reset_delay)
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 86
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->reset_delay); + ret =gpiod_set_value_cansleep (ctx->reset_gpio,0 ); +if (ret) + goto err_reset; + +if (ctx->panel_info->enable_delay) +mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->enable_delay); + +mipi_dsi_dcs_exit_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,100 );
Does this code ignore the prepare_delay and init_delay fields defined in anbernic_panel_td4310_info by using a hardcoded 100ms sleep?
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 90
Quoted 4 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +4
-0+mipi_dsi_dcs_set_display_on_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,100 ); + +return dsi_ctx.accum_err;
If mipi_dsi_dcs_exit_sleep_mode_multi or mipi_dsi_dcs_set_display_on_multi encounters an error and populates dsi_ctx.accum_err, does this return bypass the err_reset and err_enable cleanup paths?
If the DRM framework does not call unprepare when prepare fails, does this leave the vdd regulator enabled indefinitely and the GPIOs in their active states?
Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +8
-0+ +err_reset: +gpiod_set_value_cansleep (ctx->enable_gpio,0 ); +err_enable: +regulator_disable (ctx->vdd); +return ret; +} +
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 108
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+static int panel_anbernic_td4310_unprepare (struct drm_panel *panel) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +struct mipi_dsi_device *dsi = ctx->dsi; +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; + +mipi_dsi_dcs_set_display_off_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,120 ); +mipi_dsi_dcs_enter_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,120 );
Are the disable_delay and unprepare_delay fields defined in anbernic_panel_td4310_info intended to be used here instead of the hardcoded 120ms sleeps?
[ ... ]
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +5
-0+static int panel_anbernic_td4310_probe (struct mipi_dsi_device *dsi) +{ +struct device *dev = &dsi->dev; +struct anbernic_panel_td4310 *ctx; +int ret;
[ ... ]
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 192
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+ ctx->reset_gpio =devm_gpiod_get (dev,"reset" ,GPIOD_OUT_LOW ); +if (IS_ERR (ctx->reset_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->reset_gpio), +"Cannot get reset gpio\n" ); + + ctx->enable_gpio =devm_gpiod_get_optional (dev,"enable" ,GPIOD_OUT_HIGH );
Does initializing enable_gpio with GPIOD_OUT_HIGH assert the GPIO immediately during probe, before the vdd regulator is enabled in panel_anbernic_td4310_prepare?
Could driving control pins active while the main IC power supply is off cause leakage current or latch-up through ESD diodes?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
[email protected]
maintainerRe: [PATCH V2 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel
In reply to Chris MorganQuoted 106 lines · Chris Morgan
From: 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.
Signed-off-by Chris Morgan <[email protected]>--- drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-anbernic-td4310.c | 291 ++++++++++++++++++ 3 files changed, 302 insertions(+)create mode 100644
drivers/gpu/drm/panel/panel-anbernic-td4310.cdrivers/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 OFdrivers/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.odrivers/gpu/drm/panel/panel-anbernic-td4310.c new file +54
-0@@ -0,0 +1,291 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Driver for Anbernic panels with TD4310 panel controller. +* +* Copyright (C) 2026 Chris Morgan <[email protected]> +* +*/ + +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regulator/consumer.h> + +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> + +struct anbernic_panel_td4310_info { +const struct drm_display_mode *display_modes; +unsigned int num_modes; +u16 width_mm; +u16 height_mm; +u32 bus_flags; +unsigned long mode_flags; +u32 format; +u32 lanes; +u16 prepare_delay; +u16 reset_delay; +u16 init_delay; +u16 enable_delay; +u16 disable_delay; +u16 unprepare_delay; +}; + +struct anbernic_panel_td4310 { +struct device *dev; +struct mipi_dsi_device *dsi; +struct drm_panel panel; +const struct anbernic_panel_td4310_info *panel_info; +struct gpio_desc *reset_gpio; +struct gpio_desc *enable_gpio; +struct regulator *vdd; +enum drm_panel_orientation orientation; +}; + +static inline struct anbernic_panel_td4310 *panel_to_anbernic_panel_td4310 (struct drm_panel *panel) +{ + return container_of(panel, struct anbernic_panel_td4310, panel); +} + +static int panel_anbernic_td4310_prepare (struct drm_panel *panel) +{
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 64
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +struct mipi_dsi_device *dsi = ctx->dsi; +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; +int ret; + + ret =regulator_enable (ctx->vdd); +if (ret) +return ret; + +if (ctx->enable_gpio) {
Drop the if, gpiod_set_value_cansleep will check a return if the gpio desc is NULL.
Quoted 18 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +18
-0+ ret =gpiod_set_value_cansleep (ctx->enable_gpio,1 ); +if (ret) + goto err_enable; + }; + +gpiod_set_value_cansleep (ctx->reset_gpio,0 ); +mipi_dsi_msleep (&dsi_ctx,10 ); + + ret =gpiod_set_value_cansleep (ctx->reset_gpio,1 ); +if (ret) + goto err_reset; +if (ctx->panel_info->reset_delay) +mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->reset_delay); + ret =gpiod_set_value_cansleep (ctx->reset_gpio,0 ); +if (ret) + goto err_reset; + +if (ctx->panel_info->enable_delay)
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 92
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->enable_delay); + +mipi_dsi_dcs_exit_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,100 ); +mipi_dsi_dcs_set_display_on_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,100 ); + +return dsi_ctx.accum_err; + +err_reset:
And you don't check it here
Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +8
-0+gpiod_set_value_cansleep (ctx->enable_gpio,0 ); +err_enable: +regulator_disable (ctx->vdd); +return ret; +} + +static int panel_anbernic_td4310_unprepare (struct drm_panel *panel) +{
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 110
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +struct mipi_dsi_device *dsi = ctx->dsi; +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; + +mipi_dsi_dcs_set_display_off_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,120 ); +mipi_dsi_dcs_enter_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,120 ); + +if (ctx->enable_gpio)
Ditto
Quoted 21 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +21
-0+gpiod_set_value_cansleep (ctx->enable_gpio,0 ); + +gpiod_set_value_cansleep (ctx->reset_gpio,1 ); + +regulator_disable (ctx->vdd); + +return 0 ; +} + +static int panel_anbernic_td4310_get_modes (struct drm_panel *panel, +struct drm_connector *connector) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +const struct anbernic_panel_td4310_info *panel_info = ctx->panel_info; +struct drm_display_mode *mode; +unsigned int i; + +for (i =0 ; i < panel_info->num_modes; i++) { + mode =drm_mode_duplicate (connector->dev, + &panel_info->display_modes[i]); +if (!mode)
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 141
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+return -ENOMEM ; + +drm_mode_set_name (mode); + + mode->type =DRM_MODE_TYPE_DRIVER ; +if (panel_info->num_modes ==1 ) + mode->type |=DRM_MODE_TYPE_PREFERRED ; + +drm_mode_probed_add (connector, mode); + }
Can you add support for a sinlge mode right now, call drm_connector_helper_get_modes_fixed() and implement support for multiple modes when a panel needs it ?
Quoted 62 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +62
-0+ + connector->display_info.bpc =8 ; + connector->display_info.width_mm = panel_info->width_mm; + connector->display_info.height_mm = panel_info->height_mm; + connector->display_info.bus_flags = panel_info->bus_flags; + +return panel_info->num_modes; +} + +static enum drm_panel_orientation panel_anbernic_td4310_get_orientation (struct drm_panel *panel) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); + +return ctx->orientation; +} + +static const struct drm_panel_funcs panel_anbernic_td4310_funcs = { + .prepare = panel_anbernic_td4310_prepare, + .unprepare = panel_anbernic_td4310_unprepare, + .get_modes = panel_anbernic_td4310_get_modes, + .get_orientation = panel_anbernic_td4310_get_orientation, +}; + +static int panel_anbernic_td4310_probe (struct mipi_dsi_device *dsi) +{ +struct device *dev = &dsi->dev; +struct anbernic_panel_td4310 *ctx; +int ret; + + ctx = devm_drm_panel_alloc(dev, struct anbernic_panel_td4310, panel, + &panel_anbernic_td4310_funcs, +DRM_MODE_CONNECTOR_DSI ); +if (IS_ERR (ctx)) +return PTR_ERR (ctx); + + ctx->dev = dev; + + ctx->panel_info =of_device_get_match_data (dev); +if (!ctx->panel_info) +return -EINVAL ; + + 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 ); +if (IS_ERR (ctx->reset_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->reset_gpio), +"Cannot get reset gpio\n" ); + + ctx->enable_gpio =devm_gpiod_get_optional (dev,"enable" ,GPIOD_OUT_HIGH ); +if (IS_ERR (ctx->enable_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->enable_gpio), +"Cannot get enable gpio\n" ); + + ctx->vdd =devm_regulator_get (dev,"vdd" ); +if (IS_ERR (ctx->vdd)) +return dev_err_probe (dev,PTR_ERR (ctx->vdd), +"Failed to request vdd regulator\n" ); + + ctx->dsi = dsi; +mipi_dsi_set_drvdata (dsi, ctx);
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 213
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+ + dsi->lanes = ctx->panel_info->lanes; + dsi->format = ctx->panel_info->format; + dsi->mode_flags = ctx->panel_info->mode_flags; + + ret =drm_panel_of_backlight (&ctx->panel); +if (ret) +return ret; + +drm_panel_add (&ctx->panel);
devm_drm_panel_add()
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 215
Quoted 2 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +2
-0+ + ret =mipi_dsi_attach (dsi);
devm_mipi_dsi_attach()
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+if (ret <0 ) { +dev_err (dev,"mipi_dsi_attach failed: %d\n" , ret); +drm_panel_remove (&ctx->panel); +return ret; + } + +return 0 ; +} + +static void panel_anbernic_td4310_remove (struct mipi_dsi_device *dsi)
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 235
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+{ +struct anbernic_panel_td4310 *ctx =mipi_dsi_get_drvdata (dsi); +int ret; + + ret =mipi_dsi_detach (dsi); +if (ret <0 ) +dev_err (&dsi->dev,"Failed to detach from DSI host: %d\n" , ret); + +drm_panel_remove (&ctx->panel); +}
And drop remove completely
Quoted 46 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +46
-0+ +static const struct drm_display_mode anbernic_vitapro_modes[] = { + { + .clock =140020 , + .hdisplay =1080 , + .hsync_start =1080 +50 , + .hsync_end =1080 +50 +4 , + .htotal =1080 +50 +4 +50 , + .vdisplay =1920 , + .vsync_start =1920 +15 , + .vsync_end =1920 +15 +4 , + .vtotal =1920 +15 +4 +32 , + .flags =DRM_MODE_FLAG_NHSYNC |DRM_MODE_FLAG_NVSYNC , + }, +}; + +static const struct anbernic_panel_td4310_info anbernic_vitapro_info = { + .display_modes = anbernic_vitapro_modes, + .num_modes =ARRAY_SIZE (anbernic_vitapro_modes), + .width_mm =69 , + .height_mm =121 , + .bus_flags =DRM_BUS_FLAG_DE_LOW |DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE , + .mode_flags =MIPI_DSI_MODE_VIDEO |MIPI_DSI_MODE_VIDEO_BURST | +MIPI_DSI_MODE_LPM |MIPI_DSI_MODE_NO_EOT_PACKET | +MIPI_DSI_CLOCK_NON_CONTINUOUS , + .format =MIPI_DSI_FMT_RGB888 , + .lanes =4 , + .prepare_delay =50 , + .reset_delay =220 , + .enable_delay =120 , + .disable_delay =50 , + .unprepare_delay =20 , +}; + +static const struct of_device_id panel_anbernic_td4310_of_match[] = { + { + .compatible ="anbernic,panel-vita-pro" , + .data = &anbernic_vitapro_info, + }, + { }, +}; +MODULE_DEVICE_TABLE (of, panel_anbernic_td4310_of_match); + +static struct mipi_dsi_driver anbernic_panel_td4310_driver = { + .driver = { + .name ="panel-anbernic-td4310" ,
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 291
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+ .of_match_table = panel_anbernic_td4310_of_match, + }, + .probe = panel_anbernic_td4310_probe, + .remove = panel_anbernic_td4310_remove, +}; +module_mipi_dsi_driver (anbernic_panel_td4310_driver); + +MODULE_AUTHOR ("Chris Morgan <[email protected]>" ); +MODULE_DESCRIPTION ("DRM driver for Anbernic TD4310 MIPI DSI panels" ); +MODULE_LICENSE ("GPL" );
Thanks,
Neil
Re: [PATCH V2 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel
Quoted 141 lines · On Mon, May 18, 2026 at 03:55:08PM +0200, [email protected] wrote:
Quoted 105 lines · Chris Morgan
From: 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.
Signed-off-by Chris Morgan <[email protected]>--- drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-anbernic-td4310.c | 291 ++++++++++++++++++ 3 files changed, 302 insertions(+)create mode 100644
drivers/gpu/drm/panel/panel-anbernic-td4310.cdrivers/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 OFdrivers/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.odrivers/gpu/drm/panel/panel-anbernic-td4310.c new file +54
-0@@ -0,0 +1,291 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Driver for Anbernic panels with TD4310 panel controller. +* +* Copyright (C) 2026 Chris Morgan <[email protected]> +* +*/ + +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regulator/consumer.h> + +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> + +struct anbernic_panel_td4310_info { +const struct drm_display_mode *display_modes; +unsigned int num_modes; +u16 width_mm; +u16 height_mm; +u32 bus_flags; +unsigned long mode_flags; +u32 format; +u32 lanes; +u16 prepare_delay; +u16 reset_delay; +u16 init_delay; +u16 enable_delay; +u16 disable_delay; +u16 unprepare_delay; +}; + +struct anbernic_panel_td4310 { +struct device *dev; +struct mipi_dsi_device *dsi; +struct drm_panel panel; +const struct anbernic_panel_td4310_info *panel_info; +struct gpio_desc *reset_gpio; +struct gpio_desc *enable_gpio; +struct regulator *vdd; +enum drm_panel_orientation orientation; +}; + +static inline struct anbernic_panel_td4310 *panel_to_anbernic_panel_td4310 (struct drm_panel *panel) +{ + return container_of(panel, struct anbernic_panel_td4310, panel); +} + +static int panel_anbernic_td4310_prepare (struct drm_panel *panel) +{drivers/gpu/drm/panel/panel-anbernic-td4310.c line 64
Quoted 10 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +10
-0+struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +struct mipi_dsi_device *dsi = ctx->dsi; +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; +int ret; + + ret =regulator_enable (ctx->vdd); +if (ret) +return ret; + +if (ctx->enable_gpio) {Drop the if,
gpiod_set_value_cansleepwill check a return if the gpio desc is NULL.Quoted 22 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +22
-0+ ret =gpiod_set_value_cansleep (ctx->enable_gpio,1 ); +if (ret) + goto err_enable; + }; + +gpiod_set_value_cansleep (ctx->reset_gpio,0 ); +mipi_dsi_msleep (&dsi_ctx,10 ); + + ret =gpiod_set_value_cansleep (ctx->reset_gpio,1 ); +if (ret) + goto err_reset; +if (ctx->panel_info->reset_delay) +mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->reset_delay); + ret =gpiod_set_value_cansleep (ctx->reset_gpio,0 ); +if (ret) + goto err_reset; + +if (ctx->panel_info->enable_delay) +mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->enable_delay); + +mipi_dsi_dcs_exit_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,100 );
Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 92
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+mipi_dsi_dcs_set_display_on_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,100 ); + +return dsi_ctx.accum_err; + +err_reset:And you don't check it here
Should I check the value and print an error if it fails? I had planned on continuing even after a failure with the shutdown so it could turn the device off, unless that's incorrect.
Quoted 13 lines
Quoted 13 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +12
-0+gpiod_set_value_cansleep (ctx->enable_gpio,0 ); +err_enable: +regulator_disable (ctx->vdd); +return ret; +} + +static int panel_anbernic_td4310_unprepare (struct drm_panel *panel) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +struct mipi_dsi_device *dsi = ctx->dsi; +struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; +
Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 110
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+mipi_dsi_dcs_set_display_off_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,120 ); +mipi_dsi_dcs_enter_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx,120 ); + +if (ctx->enable_gpio)Ditto
I'll remove the if, is it okay to not check the return value since I'm going to continue anyway or should I check and error (and then continue).
Quoted 27 lines
Quoted 27 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +26
-0+gpiod_set_value_cansleep (ctx->enable_gpio,0 ); + +gpiod_set_value_cansleep (ctx->reset_gpio,1 ); + +regulator_disable (ctx->vdd); + +return 0 ; +} + +static int panel_anbernic_td4310_get_modes (struct drm_panel *panel, +struct drm_connector *connector) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); +const struct anbernic_panel_td4310_info *panel_info = ctx->panel_info; +struct drm_display_mode *mode; +unsigned int i; + +for (i =0 ; i < panel_info->num_modes; i++) { + mode =drm_mode_duplicate (connector->dev, + &panel_info->display_modes[i]); +if (!mode) +return -ENOMEM ; + +drm_mode_set_name (mode); + + mode->type =DRM_MODE_TYPE_DRIVER ;
Quoted 9 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 141
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +5
-0+if (panel_info->num_modes ==1 ) + mode->type |=DRM_MODE_TYPE_PREFERRED ; + +drm_mode_probed_add (connector, mode); + }Can you add support for a sinlge mode right now, call
drm_connector_helper_get_modes_fixed()and implement support for multiple modes when a panel needs it ?
Sounds like a plan, thank you.
Quoted 66 lines
Quoted 66 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +66
-0+ + connector->display_info.bpc =8 ; + connector->display_info.width_mm = panel_info->width_mm; + connector->display_info.height_mm = panel_info->height_mm; + connector->display_info.bus_flags = panel_info->bus_flags; + +return panel_info->num_modes; +} + +static enum drm_panel_orientation panel_anbernic_td4310_get_orientation (struct drm_panel *panel) +{ +struct anbernic_panel_td4310 *ctx =panel_to_anbernic_panel_td4310 (panel); + +return ctx->orientation; +} + +static const struct drm_panel_funcs panel_anbernic_td4310_funcs = { + .prepare = panel_anbernic_td4310_prepare, + .unprepare = panel_anbernic_td4310_unprepare, + .get_modes = panel_anbernic_td4310_get_modes, + .get_orientation = panel_anbernic_td4310_get_orientation, +}; + +static int panel_anbernic_td4310_probe (struct mipi_dsi_device *dsi) +{ +struct device *dev = &dsi->dev; +struct anbernic_panel_td4310 *ctx; +int ret; + + ctx = devm_drm_panel_alloc(dev, struct anbernic_panel_td4310, panel, + &panel_anbernic_td4310_funcs, +DRM_MODE_CONNECTOR_DSI ); +if (IS_ERR (ctx)) +return PTR_ERR (ctx); + + ctx->dev = dev; + + ctx->panel_info =of_device_get_match_data (dev); +if (!ctx->panel_info) +return -EINVAL ; + + 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 ); +if (IS_ERR (ctx->reset_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->reset_gpio), +"Cannot get reset gpio\n" ); + + ctx->enable_gpio =devm_gpiod_get_optional (dev,"enable" ,GPIOD_OUT_HIGH ); +if (IS_ERR (ctx->enable_gpio)) +return dev_err_probe (dev,PTR_ERR (ctx->enable_gpio), +"Cannot get enable gpio\n" ); + + ctx->vdd =devm_regulator_get (dev,"vdd" ); +if (IS_ERR (ctx->vdd)) +return dev_err_probe (dev,PTR_ERR (ctx->vdd), +"Failed to request vdd regulator\n" ); + + ctx->dsi = dsi; +mipi_dsi_set_drvdata (dsi, ctx); + + dsi->lanes = ctx->panel_info->lanes; + dsi->format = ctx->panel_info->format; + dsi->mode_flags = ctx->panel_info->mode_flags;
Quoted 9 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 213
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+ + ret =drm_panel_of_backlight (&ctx->panel); +if (ret) +return ret; + +drm_panel_add (&ctx->panel);
devm_drm_panel_add()
Will do.
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 215
Quoted 2 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +2
-0+ + ret =mipi_dsi_attach (dsi);
devm_mipi_dsi_attach()
Will do.
Quoted 14 lines
Quoted 14 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +14
-0+if (ret <0 ) { +dev_err (dev,"mipi_dsi_attach failed: %d\n" , ret); +drm_panel_remove (&ctx->panel); +return ret; + } + +return 0 ; +} + +static void panel_anbernic_td4310_remove (struct mipi_dsi_device *dsi) +{ +struct anbernic_panel_td4310 *ctx =mipi_dsi_get_drvdata (dsi); +int ret; +
Quoted 9 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 235
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+ ret =mipi_dsi_detach (dsi); +if (ret <0 ) +dev_err (&dsi->dev,"Failed to detach from DSI host: %d\n" , ret); + +drm_panel_remove (&ctx->panel); +}And drop remove completely
I take it the devm calls above remove the need for a remove callback? I take it that's new? Thank you.
Quoted 51 lines
Quoted 51 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +51
-0+ +static const struct drm_display_mode anbernic_vitapro_modes[] = { + { + .clock =140020 , + .hdisplay =1080 , + .hsync_start =1080 +50 , + .hsync_end =1080 +50 +4 , + .htotal =1080 +50 +4 +50 , + .vdisplay =1920 , + .vsync_start =1920 +15 , + .vsync_end =1920 +15 +4 , + .vtotal =1920 +15 +4 +32 , + .flags =DRM_MODE_FLAG_NHSYNC |DRM_MODE_FLAG_NVSYNC , + }, +}; + +static const struct anbernic_panel_td4310_info anbernic_vitapro_info = { + .display_modes = anbernic_vitapro_modes, + .num_modes =ARRAY_SIZE (anbernic_vitapro_modes), + .width_mm =69 , + .height_mm =121 , + .bus_flags =DRM_BUS_FLAG_DE_LOW |DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE , + .mode_flags =MIPI_DSI_MODE_VIDEO |MIPI_DSI_MODE_VIDEO_BURST | +MIPI_DSI_MODE_LPM |MIPI_DSI_MODE_NO_EOT_PACKET | +MIPI_DSI_CLOCK_NON_CONTINUOUS , + .format =MIPI_DSI_FMT_RGB888 , + .lanes =4 , + .prepare_delay =50 , + .reset_delay =220 , + .enable_delay =120 , + .disable_delay =50 , + .unprepare_delay =20 , +}; + +static const struct of_device_id panel_anbernic_td4310_of_match[] = { + { + .compatible ="anbernic,panel-vita-pro" , + .data = &anbernic_vitapro_info, + }, + { }, +}; +MODULE_DEVICE_TABLE (of, panel_anbernic_td4310_of_match); + +static struct mipi_dsi_driver anbernic_panel_td4310_driver = { + .driver = { + .name ="panel-anbernic-td4310" , + .of_match_table = panel_anbernic_td4310_of_match, + }, + .probe = panel_anbernic_td4310_probe, + .remove = panel_anbernic_td4310_remove, +};
Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 291
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +5
-0+module_mipi_dsi_driver (anbernic_panel_td4310_driver); + +MODULE_AUTHOR ("Chris Morgan <[email protected]>" ); +MODULE_DESCRIPTION ("DRM driver for Anbernic TD4310 MIPI DSI panels" ); +MODULE_LICENSE ("GPL" );Thanks,
Neil
Thank you,
Chris
Neil Armstrong
maintainerRe: [PATCH V2 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel
Quoted 4 lines · Chris Morgan
Quoted 3 lines · On Mon, May 18, 2026 at 03:55:08PM +0200, [email protected] wrote:
Quoted 2 lines · Chris Morgan
From: Chris Morgan <[email protected]>
<snip>
Quoted 10 lines
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 92
Quoted 4 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +4
-0+ +return dsi_ctx.accum_err; + +err_reset:And you don't check it here
Should I check the value and print an error if it fails? I had planned on continuing even after a failure with the shutdown so it could turn the device off, unless that's incorrect.
yeah continue and ignore the return value on the error path
Quoted 2 lines
Quoted 2 lines
<snip>
Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+ +static void panel_anbernic_td4310_remove (struct mipi_dsi_device *dsi) +{ +struct anbernic_panel_td4310 *ctx =mipi_dsi_get_drvdata (dsi); +int ret; +Quoted 9 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 235
Quoted 6 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +6
-0+ ret =mipi_dsi_detach (dsi); +if (ret <0 ) +dev_err (&dsi->dev,"Failed to detach from DSI host: %d\n" , ret); + +drm_panel_remove (&ctx->panel); +}And drop remove completely
Quoted 2 lines
I take it the devm calls above remove the need for a remove callback? I take it that's new? Thank you.
Yep !
Quoted 63 lines
Quoted 52 lines
Quoted 52 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +51
-0+ +static const struct drm_display_mode anbernic_vitapro_modes[] = { + { + .clock =140020 , + .hdisplay =1080 , + .hsync_start =1080 +50 , + .hsync_end =1080 +50 +4 , + .htotal =1080 +50 +4 +50 , + .vdisplay =1920 , + .vsync_start =1920 +15 , + .vsync_end =1920 +15 +4 , + .vtotal =1920 +15 +4 +32 , + .flags =DRM_MODE_FLAG_NHSYNC |DRM_MODE_FLAG_NVSYNC , + }, +}; + +static const struct anbernic_panel_td4310_info anbernic_vitapro_info = { + .display_modes = anbernic_vitapro_modes, + .num_modes =ARRAY_SIZE (anbernic_vitapro_modes), + .width_mm =69 , + .height_mm =121 , + .bus_flags =DRM_BUS_FLAG_DE_LOW |DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE , + .mode_flags =MIPI_DSI_MODE_VIDEO |MIPI_DSI_MODE_VIDEO_BURST | +MIPI_DSI_MODE_LPM |MIPI_DSI_MODE_NO_EOT_PACKET | +MIPI_DSI_CLOCK_NON_CONTINUOUS , + .format =MIPI_DSI_FMT_RGB888 , + .lanes =4 , + .prepare_delay =50 , + .reset_delay =220 , + .enable_delay =120 , + .disable_delay =50 , + .unprepare_delay =20 , +}; + +static const struct of_device_id panel_anbernic_td4310_of_match[] = { + { + .compatible ="anbernic,panel-vita-pro" , + .data = &anbernic_vitapro_info, + }, + { }, +}; +MODULE_DEVICE_TABLE (of, panel_anbernic_td4310_of_match); + +static struct mipi_dsi_driver anbernic_panel_td4310_driver = { + .driver = { + .name ="panel-anbernic-td4310" , + .of_match_table = panel_anbernic_td4310_of_match, + }, + .probe = panel_anbernic_td4310_probe, + .remove = panel_anbernic_td4310_remove, +};Quoted 8 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c line 291
Quoted 5 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +5
-0+module_mipi_dsi_driver (anbernic_panel_td4310_driver); + +MODULE_AUTHOR ("Chris Morgan <[email protected]>" ); +MODULE_DESCRIPTION ("DRM driver for Anbernic TD4310 MIPI DSI panels" ); +MODULE_LICENSE ("GPL" );Thanks,
NeilThank you,
Chris
PATCH v2 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 v2 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/Bluetooth combo
- 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 | 3 + .../rockchip/rk3576-anbernic-rg-vita-pro.dts | 1317 +++++++++++++++++ 2 files changed, 1320 insertions(+) create mode 100644 arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts
arch/arm64/boot/dts/rockchip/Makefile +3 -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
@@ -215,6 +216,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-coolpi-4b.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-evb1-v10.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-gameforce-ace.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova.dtb +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova-tower.dtb +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova-test.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-khadas-edge2.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6s.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6c.dtb
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +1317 -0
1,317 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V2 6/6] arm64: dts: rockchip: Add Anbernic RG Vita-Pro
Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [High] Missing DTB source files for unrelated Makefile additions. - [High] Undefined `&pmic_pins`…
Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Missing DTB source files for unrelated Makefile additions.
- [High] Undefined &pmic_pins phandle causes compilation failure.
- [High] PCIe reset GPIO polarity is incorrectly configured as active-high.
- [High] Bluetooth GPIO pins are mismatched with their pinctrl definitions.
- [High] Hardware description contradicts the commit message regarding the Bluetooth module.
- [High] Battery charge current is dangerously high and capacity contradicts the commit message.
- [Medium] SPI bus lacks a child device node, breaking userspace joystick control.
- [Medium] Incorrect pinctrl assignment for the &sdio controller.
- [Medium] Contradictory regulator states defined for vdd_cpu_lit_s0 and vdd_gpu_s0.
- [Medium] Misplaced vbus-supply property in the Type-C Port Controller node.
- [Medium] LCD panel supply regulator is incorrectly marked as always-on.
- [Low] Undocumented enablement of HDMI components.
--
commit f6eae2256befc6eb3871393bb039d8a06e21e7b0
Author: Chris Morgan <[email protected]>
arm64: dts: rockchip: Add Anbernic RG Vita-Pro
This commit adds the device tree for the Anbernic RG Vita-Pro, based on the Rockchip RK3576. It includes support for the display, buttons, joysticks, LEDs, eMMC, SDMMC, WiFi/Bluetooth, and battery.
Quoted 1 line
The Anbernic RG Vita-Pro includes the following hardware:
[ ... ]
Quoted 1 line
- RTL8852BE WiFi/Bluetooth combo
The commit message claims the device uses an RTL8852BE Wi-Fi/Bluetooth combo, which interfaces Bluetooth over USB. However, the device tree defines a realtek,rtl8822cs-bt compatible string under a UART interface (&uart6).
Does this hardware mismatch cause the probe to fail?
Quoted 1 line
- 5000mAH battery
The commit message specifies a 5000mAh battery, but the device tree specifies a charge-full-design-microamp-hours of 8000000 (8000mAh).
Additionally, the commit message completely omits HDMI from its detailed list of hardware, but the device tree enables HDMI components (hdmi, hdptxphy, and hdmi-con). Is the commit message missing this hardware description, or is this leftover code?
Quoted 4 lines
arch/arm64/boot/dts/rockchip/Makefile +0
-0
[ ... ]
arch/arm64/boot/dts/rockchip/Makefile dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-coolpi-4b.dtb · line 220
Quoted 6 lines
arch/arm64/boot/dts/rockchip/Makefile +2
-0@@ -215,6 +216,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-coolpi-4b.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-evb1-v10.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-gameforce-ace.dtb dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova.dtb +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova-tower.dtb +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-indiedroid-nova-test.dtb
The .dts source files for these two additions are missing from the patch. Will this cause the kernel build to fail when make dtbs is executed?
Quoted 5 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +0
-0
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 69
Quoted 5 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +5
-0+ battery: battery { + compatible = "simple-battery"; + charge-full-design-microamp-hours = <8000000>; + charge-term-current-microamp = <300000>; + constant-charge-current-max-microamp = <10000000>;
The constant-charge-current-max-microamp is set to 10 Amps (10000000). Does this exceed the physical limits of the hardware and pose a safety risk for a handheld single-cell battery?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 379
Quoted 4 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +4
-0+ vdd_lcd: regulator-vdd-lcd { + compatible = "regulator-fixed"; + regulator-name = "vdd_lcd"; + regulator-always-on;
By marking this regulator as always-on, the panel's power rail cannot be shut down by the DRM/panel driver framework during screen blanking or system suspend. Does this result in continuous unnecessary battery drain?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 511
Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +10
-0+ pmic@23 { + compatible = "rockchip,rk806"; + reg = <0x23>; + #gpio-cells = <2>; + gpio-controller; + interrupt-parent = <&gpio0>; + interrupts = <RK_PA6 IRQ_TYPE_LEVEL_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&pmic_pins + &rk806_dvs1_null
This pinctrl-0 property references &pmic_pins, but this node does not appear to be defined in the .dts file. Will this missing definition cause a fatal device tree compilation error?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 648
Quoted 12 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +12
-0+ vdd_cpu_lit_s0: dcdc-reg3 { + regulator-always-on; + regulator-boot-on; + regulator-min-microvolt = <550000>; + regulator-max-microvolt = <950000>; + regulator-name = "vdd_cpu_lit_s0"; + regulator-ramp-delay = <12500>; + regulator-state-mem { + regulator-off-in-suspend; + regulator-suspend-microvolt = <750000>; + }; + };
Is it semantically valid to request a specific voltage level (regulator-suspend-microvolt) during suspend for a regulator that is explicitly commanded to shut off (regulator-off-in-suspend)? Can this lead to undefined behavior in the PMIC framework?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 673
Quoted 12 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +12
-0+ vdd_gpu_s0: dcdc-reg5 { + regulator-boot-on; + regulator-enable-ramp-delay = <400>; + regulator-min-microvolt = <550000>; + regulator-max-microvolt = <900000>; + regulator-name = "vdd_gpu_s0"; + regulator-ramp-delay = <12500>; + regulator-state-mem { + regulator-off-in-suspend; + regulator-suspend-microvolt = <850000>; + }; + };
Similarly, could this combination of regulator-off-in-suspend and regulator-suspend-microvolt for the GPU regulator cause issues during suspend?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 934
Quoted 11 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +11
-0+ husb311: typec-portc@4e { + compatible = "hynetek,husb311", "richtek,rt1711h"; + reg = <0x4e>; + interrupt-parent = <&gpio0>; + interrupts = <RK_PD1 IRQ_TYPE_LEVEL_LOW>; + pinctrl-0 = <&usbc_int>; + pinctrl-names = "default"; + status = "okay"; + vbus-supply = <&vbus5v0_typec>; + + connector {
According to standard USB Type-C bindings, shouldn't the vbus-supply property reside inside the connector child node? Will the TCPM framework fail to find the VBUS regulator if it is placed in the root of the husb311 node, preventing the device from supplying power in OTG mode?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 1001
Quoted 4 lines
+&pcie0 {
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +3
-0+ pinctrl-names = "default"; + pinctrl-0 = <&pcie0_perstn>; + reset-gpios = <&gpio2 RK_PB4 GPIO_ACTIVE_HIGH>;
The PCIe specification dictates that the PERST# reset signal is active-low, which is supported by the pcie0_perstn pin name. By configuring it as active-high, will the PCIe controller invert the reset logic and permanently hold the Wi-Fi card in reset, causing link training to fail?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 1178
Quoted 10 lines
+&sdio {
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +9
-0+ bus-width = <4>; + cap-mmc-highspeed; + cap-sd-highspeed; + disable-wp; + max-frequency = <150000000>; + no-mmc; + no-sdio; + pinctrl-0 = <&sdmmc1m0_bus4>, <&sdmmc1m0_clk>, + <&sdmmc1m0_cmd>, <&sdmmc1m0_det>;
The &sdio node configures its pins using sdmmc1m0 properties. Since the SDIO controller is a distinct IP block from SDMMC1, does routing the SDIO block through pins intended for another controller disconnect it from the physical SD card slot?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 1226
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. */ +};
The &spi1 bus is enabled for userspace-controlled analog joysticks, but no child device node (such as spidev or a dedicated driver compatible) is defined. Since the Linux SPI core will not instantiate any devices on an empty bus, does this completely prevent userspace from communicating with the microcontroller?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 1272
Quoted 11 lines
+&uart6 {
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +10
-0+ pinctrl-0 = <&uart6m1_xfer>, <&uart6m1_ctsn>, <&uart6m1_rtsn>; + pinctrl-names = "default"; + uart-has-rtscts; + status = "okay"; + + bluetooth { + compatible = "realtek,rtl8822cs-bt"; + device-wake-gpios = <&gpio2 RK_PB6 GPIO_ACTIVE_HIGH>; + enable-gpios = <&gpio2 RK_PB5 GPIO_ACTIVE_HIGH>; + host-wake-gpios = <&gpio2 RK_PC1 GPIO_ACTIVE_HIGH>;
The enable-gpios is assigned to RK_PB5 and device-wake-gpios to RK_PB6. However, the corresponding pinctrl configurations map RK_PB5 to bt_wake_gpio and RK_PB6 to bt_reset_gpio.
Does this logical swap mean the driver will toggle the wake line when it intends to reset the device, causing Bluetooth initialization to fail?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6