Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <63724f51.9d0a0220.bdb2c.ea2e@mx.google.com>
Date:   Mon, 14 Nov 2022 08:23:05 -0600
From:   Chris Morgan <macroalpha82@gmail.com>
To:     Maya Matuszczyk <maccraft123mc@gmail.com>
Cc:     linux-rockchip@lists.infradead.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>,
        =?iso-8859-1?Q?Jo=E3o_H_=2E?= Spies <jhlspies@gmail.com>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/5] arm64: dts: rockchip: Add Anbernic RG351M
References: <20221112143411.517906-1-maccraft123mc@gmail.com>
 <20221112143411.517906-6-maccraft123mc@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221112143411.517906-6-maccraft123mc@gmail.com>
Precedence: bulk

On Sat, Nov 12, 2022 at 03:34:11PM +0100, Maya Matuszczyk wrote:
> This device is a clone of Odroid Go Advance, with added PWM motor, internal
> gamepad connected on USB instead of just having it be on GPIO + ADC, and
> missing battery shunt resistor.

I have concerns about using the rk817 battery driver with no shunt
resistor. For the moment, that would result in wrong values being
given for battery capacity and current. What could be done is to
update the driver to only check voltage values when the shunt
resistor is missing (since it's the current values and columb counter
that gets affected with no resistor in place). For now though, I'd
just remove the battery and charger since it's going to report the
wrong data to users.

> 
> There's also an LED on GPIO 77(I *guess* PB5 on &gpio2), that is controlled in a weird way:
> 
> - It is set to red by setting output value to 1
> - Set to green by setting output value to 0
> - Set to yellow by setting gpio direction to input
> 
> I have no idea how to describe that in DTS, without adding a custom
> driver, for now it's just left out.

Check out this document, it says you'd group them together under a
multicolor led node:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-class-multicolor.yaml

> 
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile         |  1 +
>  .../dts/rockchip/rk3326-anbernic-rg351m.dts   | 54 +++++++++++++++++++
>  2 files changed, 55 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3326-anbernic-rg351m.dts
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index 142c83d2e5aa..b379516c15eb 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -7,6 +7,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-roc-cc.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-rock-pi-s.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3318-a95x-z2.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3326-anbernic-rg351m.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3326-odroid-go2.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3326-odroid-go2-v11.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3326-odroid-go3.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/rk3326-anbernic-rg351m.dts b/arch/arm64/boot/dts/rockchip/rk3326-anbernic-rg351m.dts
> new file mode 100644
> index 000000000000..7f4726d4130e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3326-anbernic-rg351m.dts
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (c) 2019 Hardkernel Co., Ltd
> + * Copyright (c) 2020 Theobroma Systems Design und Consulting GmbH
> + * Copyright (c) 2022 Maya Matuszczyk <maccraft123mc@gmail.com>
> + */
> +
> +/dts-v1/;
> +#include "rk3326-odroid-go.dtsi"
> +
> +/ {
> +	model = "Anbernic RG351M";
> +	compatible = "anbernic,rg351m", "rockchip,rk3326";
> +
> +	battery: battery {
> +		compatible = "simple-battery";
> +		charge-full-design-microamp-hours = <3450000>;
> +		charge-term-current-microamp = <300000>;
> +		constant-charge-current-max-microamp = <2000000>;
> +		constant-charge-voltage-max-microvolt = <4200000>;
> +		factory-internal-resistance-micro-ohms = <100000>;
> +		voltage-max-design-microvolt = <4100000>;
> +		voltage-min-design-microvolt = <3500000>;
> +
> +		ocv-capacity-celsius = <20>;
> +		ocv-capacity-table-0 =  <4046950 100>, <4001920 95>, <3967900 90>, <3919950 85>,
> +					<3888450 80>, <3861850 75>, <3831540 70>, <3799130 65>,
> +					<3768190 60>, <3745650 55>, <3726610 50>, <3711630 45>,
> +					<3696720 40>, <3685660 35>, <3674950 30>, <3663050 25>,
> +					<3649470 20>, <3635260 15>, <3616920 10>, <3592440 5>,
> +					<3574170 0>;
> +	};
> +
> +	vibrator {
> +		compatible = "pwm-vibrator";
> +		pwms = <&pwm0 0 1000000 0>;
> +		pwm-names = "enable";
> +	};
> +};
> +
> +/delete-node/ &builtin_gamepad;
> +/delete-node/ &vcc_host; /* conflicts with pwm vibration motor */
> +
> +&internal_display {
> +	compatible = "elida,kd35t133";
> +};
> +
> +&pwm0 {
> +	status = "okay";
> +};
> +
> +&rk817_charger {
> +	monitored-battery = <&battery>;
> +};
> -- 
> 2.38.1
>