Add Anbernic RG Vita-Pro
Subsystems (6)
v4 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.
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 | 66 + .../power/supply/sgmicro,sgm41542.yaml | 104 ++ 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 | 254 ++++ drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/sgm41542_charger.c | 1065 +++++++++++++ 11 files changed, 2843 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 v4 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 | 104 ++++++++++++++++++ 1 file changed, 104 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml
Documentation/devicetree/bindings/power/supply/sgmicro,sgm41542.yaml new file +104 -0
@@ -0,0 +1,104 @@ +# 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: + 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 + +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
PATCH v4 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 | 1065 +++++++++++++++++++++++ 3 files changed, 1074 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 +1065 -0
1,065 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V4 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] The driver fails to enforce the battery's maximum charge current limit (`max_ichg`) when userspace…
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The driver fails to enforce the battery's maximum charge current limit (max_ichg) when userspace writes to CONSTANT_CHARGE_CURRENT.
- [High] Missing .remove callback leads to a Use-After-Free on the delayed worktimer when the device is unbound.
- [High] Missing battery properties evaluate to true and override safety defaults with -EINVAL, causing charge limits to be corrupted.
- [High] Registering the power supply before full initialization exposes uninitialized structures to sysfs writes, causing kernel panics or hardware damage.
- [Medium] Unbalanced disable_irq_wake() calls due to unconditional error handling, and leaked IRQ wake configuration on unbind.
- [Medium] The error code from devm_power_supply_register is squashed to a hardcoded -EINVAL.
--
commit 217e7bc6e3aefce7cb6ef93777df1d2955458981
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 4 lines
drivers/power/supply/sgm41542_charger.c new file +0
-0
[ ... ]
drivers/power/supply/sgm41542_charger.c line 224
Quoted 7 lines
drivers/power/supply/sgm41542_charger.c new file +7
-0+static int sgm4154x_set_ichrg_curr (struct sgm4154x_device *sgm,int cur_ua) +{ +int reg_val; +int ret; + + cur_ua =clamp (cur_ua,SGM4154X_ICHRG_I_MIN_UA ,SGM4154X_ICHRG_I_MAX_UA ); + reg_val = cur_ua /SGM4154X_ICHRG_I_STEP_UA ;
[Severity: High]
When userspace writes to CONSTANT_CHARGE_CURRENT, will this bypass the battery-specific sgm->init_data.max_ichg safety limit?
It appears the requested current is only clamped against the hardware's absolute physical maximum (3.78A) and ignores the battery's maximum charge current limit.
Can this inadvertently program the hardware to supply maximum current and bypass battery safety constraints?
[ ... ]
Quoted 8 lines
drivers/power/supply/sgm41542_charger.c new file +8
-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,
drivers/power/supply/sgm41542_charger.c line 269
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+* per the datasheet. All other values are as expected. So not +* only do we need to clamp between max and min values, but +* also clamp anything below 4352000uv to 4304000uv to prevent +* overcharging. +*/ +if (chrg_volt <4352000 ) + chrg_volt =clamp (chrg_volt,SGM4154X_VREG_V_MIN_UV ,4304000 ); + 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 ;
[Severity: High]
If sgm->init_data.max_vreg is negative (e.g., -EINVAL), will this result in undefined behavior?
The clamp function could produce a negative result here, which is then left shifted (reg_val << 3).
Could shifting a negative integer invoke undefined behavior and write corrupt data to the hardware configuration?
[ ... ]
Quoted 4 lines
drivers/power/supply/sgm41542_charger.c new file +4
-0+static int sgm4154x_power_supply_init (struct sgm4154x_device *sgm, +struct device *dev) +{ +struct power_supply_config psy_cfg = { .drv_data = sgm,
drivers/power/supply/sgm41542_charger.c line 778
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ .fwnode = dev->fwnode, }; + + psy_cfg.supplied_to = sgm4154x_charger_supplied_to; + psy_cfg.num_supplicants =ARRAY_SIZE (sgm4154x_charger_supplied_to); + psy_cfg.fwnode = dev->fwnode; + sgm->charger =devm_power_supply_register (sgm->dev, + &sgm4154x_power_supply_desc, + &psy_cfg); +if (IS_ERR (sgm->charger)) +return -EINVAL ;
[Severity: Medium]
Should this propagate the actual error code instead of returning -EINVAL?
Squashing the error code from devm_power_supply_register to a hardcoded -EINVAL suppresses the original failure context.
Would this break deferred probing if the power supply subsystem requests -EPROBE_DEFER?
[ ... ]
drivers/power/supply/sgm41542_charger.c line 807
Quoted 8 lines
drivers/power/supply/sgm41542_charger.c new file +8
-0+ 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) + 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;
[Severity: High]
Are these property checks sufficient to validate the battery info?
Missing battery properties are initialized to -EINVAL (-22) by power_supply_get_battery_info. Because -22 is non-zero, simple truthiness checks will evaluate to true.
Will this overwrite the safe hardware defaults with negative values, corrupting charge limits later on?
[ ... ]
drivers/power/supply/sgm41542_charger.c line 1005
Quoted 12 lines
drivers/power/supply/sgm41542_charger.c new file +12
-0+ sgm->sgm_monitor_wq =devm_alloc_ordered_workqueue (dev,"%s" , +WQ_MEM_RECLAIM |WQ_FREEZABLE ,"sgm-monitor-wq" ); +if (!sgm->sgm_monitor_wq) +return -EINVAL ; + + 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" );
[Severity: High]
Is it safe to register the power supply before the hardware boundaries and delayed work structures are initialized?
Userspace or udev can immediately write to sysfs attributes like online as soon as the power supply is registered.
If this happens, could sgm4154x_set_watchdog_timer queue an uninitialized delayed_work struct, potentially corrupting timer lists and causing a kernel panic?
[ ... ]
drivers/power/supply/sgm41542_charger.c line 1019
Quoted 13 lines
drivers/power/supply/sgm41542_charger.c new file +13
-0+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); + } + +INIT_DELAYED_WORK (&sgm->sgm_delay_work, sgm_charger_work);
[Severity: High]
Does this driver safely tear down sgm_delay_work on unbind?
The delayed work is initialized here but there appears to be no .remove callback or devm_delayed_work_autocancel action to cancel it. When the device is unbound, devres will automatically destroy the workqueue and free the sgm structure.
If the 5-second timer fires afterward, could this result in a use-after-free?
drivers/power/supply/sgm41542_charger.c line 1026
Quoted 7 lines
drivers/power/supply/sgm41542_charger.c new file +7
-0+ + ret =sgm4154x_vbus_regulator_register (sgm); +if (ret) { +disable_irq_wake (client->irq); +return dev_err_probe (dev, ret, +"Unable to register VBUS regulator\n" ); + }
[Severity: Medium]
Will this result in an unbalanced disable_irq_wake call if client->irq is 0?
The enable_irq_wake call is properly gated behind a check for client->irq. However, if sgm4154x_vbus_regulator_register fails, the error path unconditionally calls disable_irq_wake.
Additionally, could the driver leak the IRQ wake depth count since it lacks a .remove callback to disable the wake IRQ upon successful unbind?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH V4 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger
In reply to Chris MorganHi,
Quoted 11 lines · Chris Morgan
From: 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.pdfSigned-off-by Chris Morgan <[email protected]>---
This looks mostly okay, but I have a few comments; please find them inline.
Quoted 43 lines
drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/sgm41542_charger.c | 1065 +++++++++++++++++++++++ 3 files changed, 1074 insertions(+) create mode 100644 drivers/power/supply/sgm41542_charger.cdrivers/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 I2Cdrivers/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.odrivers/power/supply/sgm41542_charger.c new file +0
-0
drivers/power/supply/sgm41542_charger.c line 3
Quoted 3 lines
drivers/power/supply/sgm41542_charger.c new file +3
-0@@ -0,0 +1,1065 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Chrager driver for Sgm4154x
Charger
drivers/power/supply/sgm41542_charger.c line 14
Quoted 11 lines
drivers/power/supply/sgm41542_charger.c new file +11
-0+* +* Copyright (c) 2026 Rockchip Electronics Co., Ltd. +* +* Author: Xu Shengfei <[email protected]> +*/ + +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/power_supply.h>
#include <linux/property.h>
drivers/power/supply/sgm41542_charger.c line 18
Quoted 4 lines
drivers/power/supply/sgm41542_charger.c new file +4
-0+#include <linux/regmap.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/of_regulator.h> +#include <linux/regulator/machine.h>
machine.h?
Quoted 179 lines
drivers/power/supply/sgm41542_charger.c new file +179
-0+#include <linux/types.h> + +#define SGM4154X_MANUFACTURER "SGMICRO" +#define SGM4154X_NAME "sgm41542" + +#define SGM4154X_CHRG_CTRL_0 0x00 +#define SGM4154X_HIZ_EN BIT(7) +#define SGM4154X_IINDPM_I_MASK GENMASK(4, 0) +#define SGM4154X_IINDPM_I_MIN_UA 100000 +#define SGM4154X_IINDPM_I_MAX_UA 3800000 +#define SGM4154X_IINDPM_STEP_UA 100000 +#define SGM4154X_IINDPM_DEF_UA 2400000 + +#define SGM4154X_CHRG_CTRL_1 0x01 +#define SGM4154X_WDT_RST BIT(6) +#define SGM4154X_OTG_EN BIT(5) +#define SGM4154X_CHRG_EN BIT(4) + +#define SGM4154X_CHRG_CTRL_2 0x02 +#define SGM4154X_BOOST_LIM BIT(7) +#define SGM4154X_ICHRG_CUR_MASK GENMASK(5, 0) +#define SGM4154X_ICHRG_I_STEP_UA 60000 +#define SGM4154X_ICHRG_I_MIN_UA 0 +#define SGM4154X_ICHRG_I_MAX_UA 3780000 +#define SGM4154X_ICHRG_I_DEF_UA 2040000 + +#define SGM4154X_CHRG_CTRL_3 0x03 +#define SGM4154X_PRECHRG_CUR_MASK GENMASK(7, 4) +#define SGM4154X_PRECHRG_CURRENT_STEP_UA 60000 +#define SGM4154X_PRECHRG_I_MIN_UA 60000 +#define SGM4154X_PRECHRG_I_MAX_UA 780000 +#define SGM4154X_PRECHRG_I_DEF_UA 180000 +#define SGM4154X_TERMCHRG_CUR_MASK GENMASK(3, 0) +#define SGM4154X_TERMCHRG_CURRENT_STEP_UA 60000 +#define SGM4154X_TERMCHRG_I_MIN_UA 60000 +#define SGM4154X_TERMCHRG_I_MAX_UA 960000 +#define SGM4154X_TERMCHRG_I_DEF_UA 180000 + +#define SGM4154X_CHRG_CTRL_4 0x04 +#define SGM4154X_VREG_V_MASK GENMASK(7, 3) +#define SGM4154X_VREG_V_MAX_UV 4624000 +#define SGM4154X_VREG_V_MIN_UV 3856000 +#define SGM4154X_VREG_V_DEF_UV 4208000 +#define SGM4154X_VREG_V_STEP_UV 32000 +#define SGM4154X_VRECHARGE BIT(0) +#define SGM4154X_VRECHRG_STEP_MV 100 +#define SGM4154X_VRECHRG_OFFSET_MV 100 + +#define SGM4154X_CHRG_CTRL_5 0x05 +#define SGM4154X_TERM_EN BIT(7) +#define SGM4154X_WDT_TIMER_MASK GENMASK(5, 4) +#define SGM4154X_WDT_TIMER_40S BIT(4) +#define SGM4154X_WDT_TIMER_80S BIT(5) +#define SGM4154X_WDT_TIMER_160S (BIT(4) | BIT(5)) +#define SGM4154X_WDT_TIMER_DISABLE 0 + +#define SGM4154X_CHRG_CTRL_6 0x06 +#define SGM4154X_VAC_OVP_MASK GENMASK(7, 6) +#define SGM4154X_OVP_14V (BIT(7) | BIT(6)) +#define SGM4154X_OVP_10_5V BIT(7) +#define SGM4154X_OVP_6_5V BIT(6) +#define SGM4154X_OVP_5_5V 0 +#define SGM4154X_OVP_DEFAULT SGM4154X_OVP_14V +#define SGM4154X_BOOSTV GENMASK(5, 4) +#define SGM4154X_VINDPM_V_MASK GENMASK(3, 0) +#define SGM4154X_VINDPM_V_MIN_UV 3900000 +#define SGM4154X_VINDPM_V_MAX_UV 12000000 +#define SGM4154X_VINDPM_STEP_UV 100000 +#define SGM4154X_VINDPM_DEF_UV 4500000 + +#define SGM4154X_CHRG_CTRL_7 0x07 + +#define SGM4154X_CHRG_STAT 0x08 +#define SGM4154X_VBUS_STAT_MASK GENMASK(7, 5) +#define SGM4154X_OTG_MODE (BIT(7) | BIT(6) | BIT(5)) +#define SGM4154X_NON_STANDARD (BIT(7) | BIT(6)) +#define SGM4154X_UNKNOWN (BIT(7) | BIT(5)) +#define SGM4154X_USB_DCP (BIT(6) | BIT(5)) +#define SGM4154X_USB_CDP BIT(6) +#define SGM4154X_USB_SDP BIT(5) +#define SGM4154X_NOT_CHRGING 0 +#define SGM4154X_CHG_STAT_MASK GENMASK(4, 3) +#define SGM4154X_TERM_CHRG (BIT(4) | BIT(3)) +#define SGM4154X_FAST_CHRG BIT(4) +#define SGM4154X_PRECHRG BIT(3) +#define SGM4154X_PG_STAT BIT(2) +#define SGM4154X_THERM_STAT BIT(1) +#define SGM4154X_VSYS_STAT BIT(0) + +#define SGM4154X_CHRG_FAULT 0x09 +#define SGM4154X_TEMP_MASK GENMASK(2, 0) +#define SGM4154X_TEMP_HOT (BIT(2) | BIT(1)) +#define SGM4154X_TEMP_COLD (BIT(2) | BIT(0)) +#define SGM4154X_TEMP_COOL (BIT(1) | BIT(0)) +#define SGM4154X_TEMP_WARM BIT(1) +#define SGM4154X_TEMP_NORMAL BIT(0) + +#define SGM4154X_CHRG_CTRL_A 0x0a +#define SGM4154X_VBUS_GOOD BIT(7) +#define SGM4154X_VINDPM_INT_MASK BIT(1) +#define SGM4154X_IINDPM_INT_MASK BIT(0) + +#define SGM4154X_CHRG_CTRL_B 0x0b +#define SGM4154X_PN_ID (BIT(6) | BIT(5) | BIT(3)) +#define SGM4154X_PN_MASK GENMASK(6, 3) + +#define SGM4154X_CHRG_CTRL_C 0x0c + +#define SGM4154X_CHRG_CTRL_D 0x0d +#define SGM4154X_JEITA_EN BIT(0) + +#define SGM4154X_INPUT_DET 0x0e +#define SGM4154X_DPDM_ONGOING BIT(7) + +#define SGM4154X_CHRG_CTRL_F 0x0f +#define SGM4154X_VINDPM_OS_MASK GENMASK(1, 0) + +#define SGM4154X_DEFAULT_INPUT_CUR (500 * 1000) + +struct sgm4154x_init_data { +int ilim;/* input current limit */ +int vlim;/* minimum system voltage limit */ +int iterm;/* termination current */ +int iprechg;/* precharge current */ +int max_ichg;/* maximum charge current */ +int max_vreg;/* maximum charge voltage */ +}; + +struct sgm4154x_state { +bool vsys_stat; +bool therm_stat; +bool online; +u8 chrg_stat; +bool chrg_en; +bool vbus_gd; +u8 chrg_type; +u8 health; +u8 chrg_fault; +u8 ntc_fault; +}; + +struct sgm4154x_device { +struct i2c_client *client; +struct device *dev; +struct power_supply *charger; +struct regmap *regmap; +char model_name[I2C_NAME_SIZE ]; +struct sgm4154x_init_data init_data; +struct sgm4154x_state state; +struct regulator_dev *otg_rdev; +bool watchdog_enable; +struct workqueue_struct *sgm_monitor_wq; +struct delayed_work sgm_delay_work; +}; + +enum SGM4154X_VINDPM_OS { +VINDPM_OS_3900MV , +VINDPM_OS_5900MV , +VINDPM_OS_7500MV , +VINDPM_OS_10500MV , +}; + +static int sgm4154x_set_term_curr (struct sgm4154x_device *sgm,int cur_ua) +{ +int reg_val; +int ret; + + cur_ua =clamp (cur_ua,SGM4154X_TERMCHRG_I_MIN_UA ,SGM4154X_TERMCHRG_I_MAX_UA ); + reg_val = (cur_ua -SGM4154X_TERMCHRG_I_MIN_UA ) /SGM4154X_TERMCHRG_CURRENT_STEP_UA ; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_3 , +SGM4154X_TERMCHRG_CUR_MASK , + reg_val); +if (ret) +dev_err (sgm->dev,"set term current error!\n" ); + +return ret; +}
drivers/power/supply/sgm41542_charger.c line 207
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ +static int sgm4154x_set_prechrg_curr (struct sgm4154x_device *sgm,int cur_ua) +{ +int reg_val; +int ret; + + cur_ua =clamp (cur_ua,SGM4154X_PRECHRG_I_MIN_UA ,SGM4154X_PRECHRG_I_MAX_UA ); + reg_val = (cur_ua -SGM4154X_PRECHRG_I_MIN_UA ) /SGM4154X_PRECHRG_CURRENT_STEP_UA ; + + reg_val = reg_val <<4 ;
FIELD_PREP(SGM4154X_PRECHRG_CUR_MASK, reg_val);
Quoted 52 lines
drivers/power/supply/sgm41542_charger.c new file +52
-0+ ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_3 , +SGM4154X_PRECHRG_CUR_MASK , + reg_val); +if (ret) +dev_err (sgm->dev,"set precharge current error!\n" ); + +return ret; +} + +static int sgm4154x_set_ichrg_curr (struct sgm4154x_device *sgm,int cur_ua) +{ +int reg_val; +int ret; + + cur_ua =clamp (cur_ua,SGM4154X_ICHRG_I_MIN_UA ,SGM4154X_ICHRG_I_MAX_UA ); + reg_val = cur_ua /SGM4154X_ICHRG_I_STEP_UA ; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_2 , +SGM4154X_ICHRG_CUR_MASK , + reg_val); +if (ret) +dev_err (sgm->dev,"set icharge current error!\n" ); + +return ret; +} + +static int sgm4154x_get_ichrg_curr (struct sgm4154x_device *sgm) +{ +u32 reg; +int ret, val; + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_CTRL_2 , ®); +if (ret) { +dev_err (sgm->dev,"get charge current error!\n" ); +return ret; + } + + val =FIELD_GET (SGM4154X_ICHRG_CUR_MASK , reg); + +return val *SGM4154X_ICHRG_I_STEP_UA ; +} + +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,
drivers/power/supply/sgm41542_charger.c line 269
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+* per the datasheet. All other values are as expected. So not +* only do we need to clamp between max and min values, but +* also clamp anything below 4352000uv to 4304000uv to prevent +* overcharging. +*/ +if (chrg_volt <4352000 ) + chrg_volt =clamp (chrg_volt,SGM4154X_VREG_V_MIN_UV ,4304000 ); + 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 ;
FIELD_PREP
Quoted 48 lines
drivers/power/supply/sgm41542_charger.c new file +48
-0+ ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_4 , +SGM4154X_VREG_V_MASK , + reg_val); +if (ret) +dev_err (sgm->dev,"set charge voltage error!\n" ); + +return ret; +} + +static int sgm4154x_get_chrg_volt (struct sgm4154x_device *sgm) +{ +u32 reg; +int ret, val; + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_CTRL_4 , ®); +if (ret) { +dev_err (sgm->dev,"get charge voltage error!\n" ); +return ret; + } + + val =FIELD_GET (SGM4154X_VREG_V_MASK , reg); + +/* +* 0x01111 is a special value meaning 4352000uV, all other +* values are as expected based on the offset and step values. +*/ +if (val ==0x0f ) +return 4352000 ; + +return val *SGM4154X_VREG_V_STEP_UV +SGM4154X_VREG_V_MIN_UV ; +} + +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 ; + } +
drivers/power/supply/sgm41542_charger.c line 327
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+/* +* Supported ranges per the datasheet are as follows: +* 3.9v - 5.4v with a 3.9v offset +* 5.9v - 7.4v with a 5.9v offset +* 7.5v - 9.0v with a 7.5v offset +* 10.5v - 12.0v with a 10.5v offset +* Step size is a constant 100mv +*/ +if (vindpm <5900000 ) { + vindpm =clamp (vindpm,VINDPM_OS_3900MV ,5400000 );
As far as I can tell the lower clamp value is wrong here and in the following else branches. I doubt you want the enum values.
drivers/power/supply/sgm41542_charger.c line 330
Quoted 3 lines
drivers/power/supply/sgm41542_charger.c new file +3
-0+ os_val =VINDPM_OS_3900MV ; + offset =3900000 ; + }else if (vindpm >=5900000 && vindpm <7500000 ) {
You can drop the "vindpm >= 5900000 &&", that already covered by the previous if.
Quoted 68 lines
drivers/power/supply/sgm41542_charger.c new file +68
-0+ vindpm =clamp (vindpm,VINDPM_OS_5900MV ,7400000 ); + os_val =VINDPM_OS_5900MV ; + offset =5900000 ; + }else if (vindpm >=7500000 && vindpm <10500000 ) { + vindpm =clamp (vindpm,VINDPM_OS_7500MV ,9000000 ); + os_val =VINDPM_OS_7500MV ; + offset =7500000 ; + }else { + vindpm =clamp (vindpm,VINDPM_OS_10500MV ,12000000 ); + os_val =VINDPM_OS_10500MV ; + offset =10500000 ; + } + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_F , +SGM4154X_VINDPM_OS_MASK , + 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); +if (ret) +dev_err (sgm->dev,"input voltage error!\n" ); + +return ret; +} + +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 >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 ; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_0 , +SGM4154X_IINDPM_I_MASK , + reg_val); +if (ret) +dev_err (sgm->dev,"set input current limit error!\n" ); + +return ret; +} +
drivers/power/supply/sgm41542_charger.c line 408
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+static int sgm4154x_get_input_curr_lim (struct sgm4154x_device *sgm) +{ +int ret; +int ilim; + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_CTRL_0 , &ilim); +if (ret) { +dev_err (sgm->dev,"get input current limit error!\n" ); +return ret; + }
ilim &= SGM4154X_IINDPM_I_MASK;
Then you can just use ilim in the following code.
Quoted 27 lines
drivers/power/supply/sgm41542_charger.c new file +27
-0+/* Max value is not 3200000uA as expected but is 3800000uA */ +if (SGM4154X_IINDPM_I_MASK == (ilim &SGM4154X_IINDPM_I_MASK )) +return SGM4154X_IINDPM_I_MAX_UA ; + + ilim = (ilim &SGM4154X_IINDPM_I_MASK ) *SGM4154X_IINDPM_STEP_UA +SGM4154X_IINDPM_I_MIN_UA ; + +return ilim; +} + +static int sgm4154x_watchdog_timer_reset (struct sgm4154x_device *sgm) +{ +int ret; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_1 , +SGM4154X_WDT_RST , +SGM4154X_WDT_RST ); + +if (ret) +dev_err (sgm->dev,"set watchdog timer error!\n" ); + +return ret; +} + +static int sgm4154x_set_watchdog_timer (struct sgm4154x_device *sgm,int time) +{ +u8 reg_val;
drivers/power/supply/sgm41542_charger.c line 446
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+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 ;
enum sgm4154x_wdt_timer {
SGM4154X_WDT_TIMER_DISABLE = 0,
SGM4154X_WDT_TIMER_40S = 1,
SGM4154X_WDT_TIMER_80S = 2,
SGM4154X_WDT_TIMER_160S = 3,
};static int sgm4154x_set_watchdog_timer(struct sgm4154x_device *sgm, enum sgm4154x_wdt_timer time) {
...
reg_val = FIELD_PREP(SGM4154X_WDT_TIMER_MASK, time);
...This is less code, easier to read and also fixes the bug you have down below, where the function is already called with SGM4154X_WDT_TIMER_40S as argument, which right now would result in 160 seconds being configured...
Quoted 65 lines
drivers/power/supply/sgm41542_charger.c new file +65
-0+ ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_5 , +SGM4154X_WDT_TIMER_MASK , + reg_val); + +if (ret) { +dev_err (sgm->dev,"set watchdog timer error!\n" ); +return ret; + } + +if (time) { +if (!sgm->watchdog_enable) +queue_delayed_work (sgm->sgm_monitor_wq, + &sgm->sgm_delay_work, +msecs_to_jiffies (1000 *5 )); + sgm->watchdog_enable = true; + }else { + sgm->watchdog_enable = false; +sgm4154x_watchdog_timer_reset (sgm); + } + +return ret; +} + +static int sgm4154x_enable_charger (struct sgm4154x_device *sgm) +{ +int ret; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_1 , +SGM4154X_CHRG_EN , +SGM4154X_CHRG_EN ); +if (ret) +dev_err (sgm->dev,"enable charger error!\n" ); + +return ret; +} + +static int sgm4154x_disable_charger (struct sgm4154x_device *sgm) +{ +int ret; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_1 , +SGM4154X_CHRG_EN , +0 ); +if (ret) +dev_err (sgm->dev,"disable charger error!\n" ); + +return ret; +} + +static int sgm4154x_set_vac_ovp (struct sgm4154x_device *sgm) +{ +int reg_val; +int ret; + + reg_val =SGM4154X_OVP_DEFAULT &SGM4154X_VAC_OVP_MASK ; + + ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_6 , +SGM4154X_VAC_OVP_MASK , + reg_val); +if (ret) +dev_err (sgm->dev,"set vac ovp error!\n" );
drivers/power/supply/sgm41542_charger.c line 522
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ +return ret; +} + +static int sgm4154x_set_recharge_volt (struct sgm4154x_device *sgm,int recharge_volt) +{ +int reg_val; +int ret; + + reg_val = (recharge_volt -SGM4154X_VRECHRG_OFFSET_MV ) /SGM4154X_VRECHRG_STEP_MV ;
I think it makes sense to convert the function to microvolt to be consistent.
Quoted 145 lines
drivers/power/supply/sgm41542_charger.c new file +145
-0+ ret =regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_4 , +SGM4154X_VRECHARGE , + reg_val); +if (ret) +dev_err (sgm->dev,"set recharger error!\n" ); + +return ret; +} + +static int sgm4154x_get_state (struct sgm4154x_device *sgm, +struct sgm4154x_state *state) +{ +int reg, ret; + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_STAT , ®); +if (ret) { +dev_err (sgm->dev,"read SGM4154X_CHRG_STAT fail\n" ); +return ret; + } + state->chrg_type = reg &SGM4154X_VBUS_STAT_MASK ; + state->chrg_stat = reg &SGM4154X_CHG_STAT_MASK ; + state->online = !!(reg &SGM4154X_PG_STAT ); + state->therm_stat = !!(reg &SGM4154X_THERM_STAT ); + state->vsys_stat = !!(reg &SGM4154X_VSYS_STAT ); + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_FAULT , ®); +if (ret) { +dev_err (sgm->dev,"read SGM4154X_CHRG_FAULT fail\n" ); +return ret; + } + state->chrg_fault = reg; + state->ntc_fault = reg &SGM4154X_TEMP_MASK ; + state->health = state->ntc_fault; + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_CTRL_A , ®); +if (ret) { +dev_err (sgm->dev,"read SGM4154X_CHRG_CTRL_A fail\n" ); +return ret; + } + state->vbus_gd = !!(reg &SGM4154X_VBUS_GOOD ); + +return ret; +} + +static int sgm4154x_property_is_writeable (struct power_supply *psy, +enum power_supply_property prop) +{ +switch (prop) { +case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT : +case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE : +case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT : +case POWER_SUPPLY_PROP_ONLINE : +return true; +default : +return false; + } +} + +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 ); + }else { +sgm4154x_set_watchdog_timer (sgm,0 ); + ret =sgm4154x_disable_charger (sgm); + } +break ; + +case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT : + ret =sgm4154x_set_input_curr_lim (sgm, val->intval); +break ; +case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT : + ret =sgm4154x_set_ichrg_curr (sgm, val->intval); +break ; +case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE : + ret =sgm4154x_set_chrg_volt (sgm, val->intval); +break ; + +default : +return -EINVAL ; + } + +return ret; +} + +static int sgm4154x_charger_get_property (struct power_supply *psy, +enum power_supply_property psp, +union power_supply_propval *val) +{ +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; + +switch (psp) { +case POWER_SUPPLY_PROP_STATUS : +if (!state.chrg_type || (state.chrg_type ==SGM4154X_OTG_MODE )) + val->intval =POWER_SUPPLY_STATUS_DISCHARGING ; +else if (!state.chrg_stat) + val->intval =POWER_SUPPLY_STATUS_NOT_CHARGING ; +else if (state.chrg_stat ==SGM4154X_TERM_CHRG ) + val->intval =POWER_SUPPLY_STATUS_FULL ; +else + val->intval =POWER_SUPPLY_STATUS_CHARGING ; +break ; +case POWER_SUPPLY_PROP_CHARGE_TYPE : +switch (state.chrg_stat) { +case SGM4154X_PRECHRG : + val->intval =POWER_SUPPLY_CHARGE_TYPE_TRICKLE ; +break ; +case SGM4154X_FAST_CHRG : + val->intval =POWER_SUPPLY_CHARGE_TYPE_FAST ; +break ; +case SGM4154X_TERM_CHRG : + val->intval =POWER_SUPPLY_CHARGE_TYPE_TRICKLE ; +break ; +case SGM4154X_NOT_CHRGING : + val->intval =POWER_SUPPLY_CHARGE_TYPE_NONE ; +break ; +default : + val->intval =POWER_SUPPLY_CHARGE_TYPE_UNKNOWN ; + } +break ; +case POWER_SUPPLY_PROP_MANUFACTURER : + val->strval =SGM4154X_MANUFACTURER ; +break ; + +case POWER_SUPPLY_PROP_MODEL_NAME : + val->strval =SGM4154X_NAME ; +break ;
drivers/power/supply/sgm41542_charger.c line 678
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ +case POWER_SUPPLY_PROP_ONLINE : + val->intval = state.online; +break ; +case POWER_SUPPLY_PROP_PRESENT : + val->intval = state.vbus_gd; +break ; +case POWER_SUPPLY_PROP_TYPE : + val->intval =POWER_SUPPLY_TYPE_USB ; +break ;
Drop. POWER_SUPPLY_PROP_TYPE is exposed and handled automatically.
Quoted 43 lines
drivers/power/supply/sgm41542_charger.c new file +43
-0+case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE : + val->intval =sgm4154x_get_chrg_volt (sgm); +if (val->intval <0 ) +return -EINVAL ; +break ; +case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT : + val->intval =sgm4154x_get_ichrg_curr (sgm); +if (val->intval <0 ) +return -EINVAL ; +break ; +case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT : + val->intval = sgm->init_data.vlim; +break ; +case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT : + val->intval =sgm4154x_get_input_curr_lim (sgm); +if (val->intval <0 ) +return -EINVAL ; +break ; +default : +return -EINVAL ; + } + +return ret; +} + +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; +if (state.vbus_gd) { +if (sgm4154x->init_data.ilim >=SGM4154X_DEFAULT_INPUT_CUR ) { + ret =sgm4154x_set_input_curr_lim (sgm4154x, sgm4154x->init_data.ilim); +if (ret) { +dev_err (sgm4154x->dev,"set input current error!\n" ); +return IRQ_NONE ;
drivers/power/supply/sgm41542_charger.c line 731
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ } + } + } +power_supply_changed (sgm4154x->charger); + +return IRQ_HANDLED ; +} + +static enum power_supply_property sgm4154x_power_supply_props[] = { +POWER_SUPPLY_PROP_TYPE ,
Also drop it from here :)
Quoted 5 lines
drivers/power/supply/sgm41542_charger.c new file +5
-0+POWER_SUPPLY_PROP_MANUFACTURER , +POWER_SUPPLY_PROP_MODEL_NAME , +POWER_SUPPLY_PROP_STATUS , +POWER_SUPPLY_PROP_ONLINE , + POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT,
drivers/power/supply/sgm41542_charger.c line 746
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT , +POWER_SUPPLY_PROP_CHARGE_TYPE , +POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE , +POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT , + POWER_SUPPLY_PROP_PRESENT +}; + +static char *sgm4154x_charger_supplied_to[] = { +"battery-monitor" , +};
why?
Quoted 15 lines
drivers/power/supply/sgm41542_charger.c new file +15
-0+static struct power_supply_desc sgm4154x_power_supply_desc = { + .name ="sgm4154x-charger" , + .type =POWER_SUPPLY_TYPE_USB , + .properties = sgm4154x_power_supply_props,battery-monitor + .num_properties =ARRAY_SIZE (sgm4154x_power_supply_props), + .get_property = sgm4154x_charger_get_property, + .set_property = sgm4154x_charger_set_property, + .property_is_writeable = sgm4154x_property_is_writeable, +}; + +static const struct regmap_config sgm4154x_regmap_config = { + .reg_bits =8 , + .val_bits =8 , + .max_register =SGM4154X_CHRG_CTRL_F , + .cache_type =REGCACHE_NONE ,
drivers/power/supply/sgm41542_charger.c line 772
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+}; + +static int sgm4154x_power_supply_init (struct sgm4154x_device *sgm, +struct device *dev) +{ +struct power_supply_config psy_cfg = { .drv_data = sgm, + .fwnode = dev->fwnode, }; + + psy_cfg.supplied_to = sgm4154x_charger_supplied_to; + psy_cfg.num_supplicants =ARRAY_SIZE (sgm4154x_charger_supplied_to);
I doubt you want to set any hardcoded supplied data and rely on the DT setup (i.e. add "power-supplies" property).
drivers/power/supply/sgm41542_charger.c line 773
Quoted 1 line
drivers/power/supply/sgm41542_charger.c new file +1
-0+ psy_cfg.fwnode = dev->fwnode;
You already set that a few lines above.
Quoted 193 lines
drivers/power/supply/sgm41542_charger.c new file +193
-0+ sgm->charger =devm_power_supply_register (sgm->dev, + &sgm4154x_power_supply_desc, + &psy_cfg); +if (IS_ERR (sgm->charger)) +return -EINVAL ; + +return 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) + 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); + } + + ret =device_property_read_u32 (sgm->dev, +"input-voltage-limit-microvolt" , + &val); +if (!ret) + sgm->init_data.vlim =clamp (val,SGM4154X_VINDPM_V_MIN_UV , +SGM4154X_VINDPM_V_MAX_UV ); + + ret =device_property_read_u32 (sgm->dev, +"input-current-limit-microamp" , + &val); +if (!ret) + sgm->init_data.ilim =clamp (val,SGM4154X_IINDPM_I_MIN_UA , +SGM4154X_IINDPM_I_MAX_UA ); + + ret =sgm4154x_set_watchdog_timer (sgm,0 ); +if (ret) +return ret; + + ret =sgm4154x_set_prechrg_curr (sgm, sgm->init_data.iprechg); +if (ret) +return ret; + + ret =sgm4154x_set_chrg_volt (sgm, sgm->init_data.max_vreg); +if (ret) +return ret; + + ret =sgm4154x_set_term_curr (sgm, sgm->init_data.iterm); +if (ret) +return ret; + + ret =sgm4154x_set_ichrg_curr (sgm, sgm->init_data.max_ichg); +if (ret) +return ret; + + ret =sgm4154x_set_input_volt_lim (sgm, sgm->init_data.vlim); +if (ret) +return ret; + + ret =sgm4154x_set_input_curr_lim (sgm, sgm->init_data.ilim); +if (ret) +return ret; + + ret =sgm4154x_set_vac_ovp (sgm); +if (ret) +return ret; + +regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_D , +SGM4154X_JEITA_EN , +0 ); + +regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_A , +SGM4154X_IINDPM_INT_MASK , +SGM4154X_IINDPM_INT_MASK ); + +regmap_update_bits (sgm->regmap, +SGM4154X_CHRG_CTRL_A , +SGM4154X_VINDPM_INT_MASK , +SGM4154X_VINDPM_INT_MASK ); + +/* +* Recharge voltage set to 200 by BSP driver instead of hardware +* default value of 100. +*/ + ret =sgm4154x_set_recharge_volt (sgm,200 ); + +return ret; +} + +static const u32 sgm4154x_chg_otg_cur_ua[] = { +1200000 ,2000000 , +}; + +static const struct regulator_ops sgm4154x_vbus_ops = { + .list_voltage = regulator_list_voltage_linear, + .enable = regulator_enable_regmap, + .disable = regulator_disable_regmap, + .is_enabled = regulator_is_enabled_regmap, + .set_voltage_sel = regulator_set_voltage_sel_regmap, + .get_voltage_sel = regulator_get_voltage_sel_regmap, + .set_current_limit = regulator_set_current_limit_regmap, + .get_current_limit = regulator_get_current_limit_regmap, +}; + +static const struct regulator_desc sgm4154x_otg_rdesc = { + .of_match ="otg-vbus" , + .name ="otg-vbus" , + .regulators_node =of_match_ptr ("regulators" ), + .ops = &sgm4154x_vbus_ops, + .owner =THIS_MODULE , + .type =REGULATOR_VOLTAGE , + .min_uV =4850000 , + .uV_step =150000 , + .n_voltages =4 , + .vsel_reg =SGM4154X_CHRG_CTRL_6 , + .vsel_mask =SGM4154X_BOOSTV , + .enable_reg =SGM4154X_CHRG_CTRL_1 , + .enable_mask =SGM4154X_OTG_EN , + .curr_table = sgm4154x_chg_otg_cur_ua, + .n_current_limits =ARRAY_SIZE (sgm4154x_chg_otg_cur_ua), + .csel_reg =SGM4154X_CHRG_CTRL_2 , + .csel_mask =SGM4154X_BOOST_LIM , +}; + +static int sgm4154x_vbus_regulator_register (struct sgm4154x_device *sgm) +{ +struct regulator_config config = { + .dev = sgm->dev, + .regmap = sgm->regmap, + .driver_data = sgm, + }; + + sgm->otg_rdev =devm_regulator_register (sgm->dev, + &sgm4154x_otg_rdesc, + &config); + +return PTR_ERR_OR_ZERO (sgm->otg_rdev); +} + +static int sgm4154x_hw_chipid_detect (struct sgm4154x_device *sgm) +{ +int ret; +int val; + + ret =regmap_read (sgm->regmap,SGM4154X_CHRG_CTRL_B , &val); +if (ret) +return ret; + +if ((val &SGM4154X_PN_MASK ) !=SGM4154X_PN_ID ) +dev_warn (sgm->dev,"sgm4154x device ID mismatch\n" ); + +return 0 ; +} + +static void sgm_charger_work (struct work_struct *work) +{ +struct sgm4154x_device *sgm = +container_of (work, + struct sgm4154x_device, + sgm_delay_work.work); + +sgm4154x_watchdog_timer_reset (sgm); +if (sgm->watchdog_enable) +queue_delayed_work (sgm->sgm_monitor_wq, + &sgm->sgm_delay_work, +msecs_to_jiffies (1000 *5 )); +} + +static int sgm4154x_probe (struct i2c_client *client) +{
drivers/power/supply/sgm41542_charger.c line 976
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+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;
You can drop sgm->client as it is unused.
Quoted 9 lines
drivers/power/supply/sgm41542_charger.c new file +9
-0+ 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" ); +
drivers/power/supply/sgm41542_charger.c line 995
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+i2c_set_clientdata (client, sgm); + + ret =sgm4154x_hw_chipid_detect (sgm); +if (ret) +return dev_err_probe (dev, ret,"Unable to read HW ID\n" ); + +device_init_wakeup (dev,1 ); + + sgm->sgm_monitor_wq =devm_alloc_ordered_workqueue (dev,"%s" , +WQ_MEM_RECLAIM |WQ_FREEZABLE ,"sgm-monitor-wq" );
For a fixed name you don't need "%s".
Quoted 11 lines
drivers/power/supply/sgm41542_charger.c new file +11
-0+if (!sgm->sgm_monitor_wq) +return -EINVAL ; + + 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" ); +
drivers/power/supply/sgm41542_charger.c line 1016
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+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);
missing disable on module removal, use devm_add_action_or_reset() to register disable_irq_wake. Then you can also drop the extra call a few lines below.
drivers/power/supply/sgm41542_charger.c line 1019
Quoted 3 lines
drivers/power/supply/sgm41542_charger.c new file +3
-0+ } + +INIT_DELAYED_WORK (&sgm->sgm_delay_work, sgm_charger_work);
devm_delayed_work_autocancel(), otherwise the delayed work may run after module removal.
Quoted 35 lines
drivers/power/supply/sgm41542_charger.c new file +35
-0+ ret =sgm4154x_vbus_regulator_register (sgm); +if (ret) { +disable_irq_wake (client->irq); +return dev_err_probe (dev, ret, +"Unable to register VBUS regulator\n" ); + } + +return 0 ; +} + +static void sgm4154x_charger_shutdown (struct i2c_client *client) +{ +struct sgm4154x_device *sgm =i2c_get_clientdata (client); + +sgm4154x_set_prechrg_curr (sgm,SGM4154X_PRECHRG_I_DEF_UA ); +sgm4154x_disable_charger (sgm); +} + +static const struct i2c_device_id sgm4154x_i2c_ids[] = { + {"sgm41542" }, + { }, +}; +MODULE_DEVICE_TABLE (i2c, sgm4154x_i2c_ids); + +static const struct of_device_id sgm4154x_of_match[] = { + { .compatible ="sgmicro,sgm41542" , }, + { }, +}; +MODULE_DEVICE_TABLE (of, sgm4154x_of_match); + +static struct i2c_driver sgm4154x_driver = { + .driver = { + .name ="sgm4154x-charger" , + .of_match_table = sgm4154x_of_match, + },
drivers/power/supply/sgm41542_charger.c line 1065
Quoted 10 lines
drivers/power/supply/sgm41542_charger.c new file +10
-0+ .probe = sgm4154x_probe, + .shutdown = sgm4154x_charger_shutdown, + .id_table = sgm4154x_i2c_ids, +}; + +module_i2c_driver (sgm4154x_driver); + +MODULE_AUTHOR ("Xu Shengfei <[email protected]>" ); +MODULE_DESCRIPTION ("sgm4154x charger driver" ); +MODULE_LICENSE ("GPL" );
Greetings,
-- Sebastian
_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v4 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 | 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 { + panel_in: endpoint { + remote-endpoint = <&dsi_out>; + }; + }; + }; + }; + +...
Signature · 1 line
2.43.0
PATCH v4 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 | 254 ++++++++++++++++++ 3 files changed, 265 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 +254 -0
254 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH V4 4/6] drm/panel: anbernic-td4310: Add RG Vita Pro panel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The `panel_anbernic_td4310_prepare` function incorrectly returns success (0) even when MIPI DSI…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The panel_anbernic_td4310_prepare function incorrectly returns success (0) even when MIPI DSI DCS commands fail, silently masking initialization errors.
- [High] The enable GPIO is initialized to its active logical state during probe, driving the physical pin before the VDD regulator is enabled.
- [Low] Implicit declaration and undefined reference to devm_drm_panel_add, leading to a build failure.
--
commit ae79417c9bc07c168f26d7711393a542d43e2756
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. It measures approximately 5.5 inches diagonally and is 1080x1920 in resolution.
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 88
Quoted 7 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +7
-0+mipi_dsi_dcs_exit_sleep_mode_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->prepare_delay); +mipi_dsi_dcs_set_display_on_multi (&dsi_ctx); +mipi_dsi_msleep (&dsi_ctx, ctx->panel_info->prepare_delay); + +if (dsi_ctx.accum_err) + goto err_reset;
[Severity: High]
Does this code silently mask MIPI DSI initialization errors? If dsi_ctx.accum_err is non-zero, this jumps to err_reset but returns the current value of ret, which was previously set to 0. Should this update ret to dsi_ctx.accum_err before jumping?
Quoted 9 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +9
-0+ +return 0 ; + +err_reset: +gpiod_set_value_cansleep (ctx->enable_gpio,0 ); +err_enable: +regulator_disable (ctx->vdd); +return ret; +}
[ ... ]
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 174
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 );
[Severity: High]
Is it safe to initialize the enable GPIO with GPIOD_OUT_HIGH here during probe? This drives the physical pin to its active logic state before the VDD regulator is enabled in panel_anbernic_td4310_prepare(), which could potentially back-power the unpowered display controller.
Quoted 11 lines
drivers/gpu/drm/panel/panel-anbernic-td4310.c new file +11
-0+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 195
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; + +devm_drm_panel_add (dev, &ctx->panel);
[Severity: Low]
Does this code build? The function devm_drm_panel_add() does not appear to exist in the DRM panel API, which would cause an implicit declaration and undefined reference compilation failure.
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 v4 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 v4 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 | 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 V4 6/6] arm64: dts: rockchip: Add Anbernic RG Vita-Pro
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Hardware Description Mismatch for Bluetooth Controller - [Medium] Incomplete Device Tree…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware Description Mismatch for Bluetooth Controller
- [Medium] Incomplete Device Tree description for SPI Joysticks
- [Medium] Contradictory Suspend States for PMIC Regulators
--
commit 79a0a3133515f64c6cd7697921d2b225d5dc8aa8
Author: Chris Morgan <[email protected]>
Subject: arm64: dts: rockchip: Add Anbernic RG Vita-Pro
This commit adds the device tree for the Anbernic RG Vita-Pro handset, based on the Rockchip RK3576 SoC. It includes initial hardware descriptions for the display, audio, buttons, networking, and power management.
Quoted 1 line
- RTL8852BE WiFi/Bluetooth combo
[Severity: High]
The commit message mentions an RTL8852BE combo chip, but the device tree configures the Bluetooth controller using the realtek,rtl8822cs-bt compatible over a UART bus below.
Is this compatible string correct for this hardware? The RTL8852BE typically interfaces its Bluetooth module via USB rather than UART. If the hardware is indeed RTL8822CS, should the commit message be updated to reflect the correct chipset?
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 658
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>; + }; + };
[Severity: Medium]
Is it intentional to set both regulator-off-in-suspend and regulator-suspend-microvolt here?
These properties appear to contradict each other, as turning the regulator off would make the target suspend voltage meaningless. Would it be better to use regulator-on-in-suspend if the voltage needs to be maintained during sleep?
[ ... ]
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 683
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>; + }; + };
[Severity: Medium]
Does this GPU regulator have the same contradictory suspend state issue as vdd_cpu_lit_s0 above?
[ ... ]
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: Medium]
The commit message notes the analog joysticks are controlled via userspace. Since there is no child node defined on this SPI bus, how does the kernel instantiate the userspace interface?
Should there be a spidev node or another specific child device defined here to allow userspace to communicate with the MCU?
[ ... ]
Quoted 5 lines
+&uart6 {
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +4
-0+ pinctrl-0 = <&uart6m1_xfer>, <&uart6m1_ctsn>, <&uart6m1_rtsn>; + pinctrl-names = "default"; + uart-has-rtscts; + status = "okay";
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts line 1286
Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3576-anbernic-rg-vita-pro.dts new file +10
-0+ + 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>; + pinctrl-0 = <&bt_reset_gpio>, <&bt_wake_gpio>, <&bt_wake_host_irq>; + pinctrl-names = "default"; + }; +};
[Severity: High]
This is the UART bus configuration mentioned earlier where the bluetooth controller is defined. Could you clarify if the RTL8852BE mentioned in the commit message is accurate, or if this rtl8822cs-bt compatible string is the correct hardware description?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6