On Wed, May 26, 2021 at 02:41:00PM -0400, Peter Geis wrote:
> On Wed, May 26, 2021 at 12:23 PM Rudi Heitbaum <rudi@xxxxxxxxxxxx> wrote:
> >
> >
> > For rk3399pro boards the tcs4526 regulator supports the vdd_gpu
> > regulator. The tcs4526 regulator has a chip id of <0>.
> > Add the compatibile tcs,tcs4526
> >
> > without this patch, the dmesg output is:
> > fan53555-regulator 0-0010: Chip ID 0 not supported!
> > fan53555-regulator 0-0010: Failed to setup device!
> > fan53555-regulator: probe of 0-0010 failed with error -22
> > with this patch, the dmesg output is:
> > vdd_gpu: supplied by vcc5v0_sys
> >
> > The regulators are described as:
> > - Dedicated power management IC TCS4525
> > - Lithium battery protection chip TCS4526
> >
> > This has been tested with a Radxa Rock Pi N10.
> >
> > Fixes: f9028dcdf589 ("regulator: fan53555: only bind tcs4525 to correct chip id")
> > Signed-off-by: Rudi Heitbaum <rudi@xxxxxxxxxxxx>
>
> Considering the TCS4525 wasn't supported prior to its recent addition,
> and the TCS4526 wasn't supported by the driver at all, this isn't a
> fix but a feature addition.
> Binding only to the correct device ID exists for this reason, to
> prevent unsafe voltage setting.
Hi Peter, thanks for the detailed feedback. You are quite right (I had
started using the tcs4525 patch as a tcs452x patch. I'll update that in
the resubmission.
> I also don't see the TCS4525/TCS4526 regulators in the current
> linux-next device tree for the N10.
I have a working rk3399pro-vmarc-som.dtsi that I intend to submit, but
wanted to get clarity on the tcs452x first. I have included it at the
bottom of this email.
> > ---
> > drivers/regulator/fan53555.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/regulator/fan53555.c b/drivers/regulator/fan53555.c
> > index 2695be617373..ddab9359ea20 100644
> > --- a/drivers/regulator/fan53555.c
> > +++ b/drivers/regulator/fan53555.c
> > @@ -90,6 +90,7 @@ enum {
> > };
> >
> > enum {
> > + TCS4525_CHIP_ID_00 = 0,
> > TCS4525_CHIP_ID_12 = 12,
>
> This isn't a TCS4525, but a TCS4526.
I'll update this to TCS4526_CHIP_ID_00
> > };
> >
> > @@ -373,6 +374,7 @@ static int fan53555_voltages_setup_silergy(struct fan53555_device_info *di)
> > static int fan53526_voltages_setup_tcs(struct fan53555_device_info *di)
> > {
> > switch (di->chip_id) {
> > + case TCS4525_CHIP_ID_00:
> > case TCS4525_CHIP_ID_12:
> > di->slew_reg = TCS4525_TIME;
> > di->slew_mask = TCS_SLEW_MASK;
> > @@ -564,6 +566,9 @@ static const struct of_device_id __maybe_unused fan53555_dt_ids[] = {
> > }, {
> > .compatible = "tcs,tcs4525",
> > .data = (void *)FAN53526_VENDOR_TCS
> > + }, {
> > + .compatible = "tcs,tcs4526",
> > + .data = (void *)FAN53526_VENDOR_TCS
>
> Since you aren't adding any functional code, is there a particular
> reason you can't just add the chip id and simply use the tcs4525
> compatible?
> This will prevent you from needing to modify the dt-bindings as well.
In and earlier commit to the BSP kernel the proposal was to rename to
tcs452x. ref:
https://github.com/CK-LINUX/kernel/commit/b3bbe8018c56362feed1e49c8d243a8dbcdcc07b
I chose to follow the example of silergy,syr827 and silergy,syr828 for
tcs4526 (given I made the mistake in assuming that support for tcs4525
meant support for tcs4525.) This would maintain consistency of naming of
tcs4526 throughout the source. Is that ok?
> > },
> > { }
> > };
> > @@ -672,6 +677,9 @@ static const struct i2c_device_id fan53555_id[] = {
> > }, {
> > .name = "tcs4525",
> > .driver_data = FAN53526_VENDOR_TCS
> > + }, {
> > + .name = "tcs4526",
> > + .driver_data = FAN53526_VENDOR_TCS
> > },
> > { },
> > };
> > --
> > 2.29.2
> >
Below is the draft patch for the dtsi includeing the 2 missing regulators and
to enable the GPU on the Radxa Rock Pi N10 which utilises the VMARC RK3399Pro SoM.
This will be submitted seperately to the "tcs4526 regulator" patch.
--- a/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi 2021-05-08 09:11:59.000000000 +0000
+++ b/arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi 2021-05-08 09:11:59.000000000 +0000
@@ -57,6 +57,22 @@
pinctrl-0 = <&hdmi_cec>;
};
+&hdmi_sound {
+ status = "okay";
+};
+
+&gpu {
+ mali-supply = <&vdd_gpu>;
+ assigned-clocks = <&cru ACLK_GPU>;
+ assigned-clock-rates = <200000000>;
+ status = "okay";
+ /delete-property/ operating-points-v2;
+};
+
+&vopl {
+ status = "disabled";
+};
+
&i2c0 {
clock-frequency = <400000>;
i2c-scl-falling-time-ns = <30>;
@@ -289,6 +288,50 @@
};
};
};
+
+ vdd_cpu_b: tcs4525@1c {
+ compatible = "tcs,tcs4525";
+ reg = <0x1c>;
+ vin-supply = <&vcc5v0_sys>;
+ regulator-compatible = "fan53555-reg";
+ pinctrl-0 = <&vsel1_gpio>;
+ vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
+ regulator-name = "vdd_cpu_b";
+ regulator-min-microvolt = <712500>;
+ regulator-max-microvolt = <1500000>;
+ regulator-ramp-delay = <2300>;
+ fcs,suspend-voltage-selector = <1>;
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-initial-state = <3>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+
+ vdd_gpu: tcs4526@10 {
+ compatible = "tcs,tcs4526";
+ reg = <0x10>;
+ vin-supply = <&vcc5v0_sys>;
+ regulator-compatible = "fan53555-reg";
+ pinctrl-0 = <&vsel2_gpio>;
+ vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
+ regulator-name = "vdd_gpu";
+ regulator-min-microvolt = <735000>;
+ regulator-max-microvolt = <1400000>;
+ regulator-ramp-delay = <1000>;
+ fcs,suspend-voltage-selector = <1>;
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-initial-state = <3>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
};
+
+&i2s2 {
+ status = "okay";
+};
&i2c1 {
@@ -381,6 +380,29 @@
pmic_int_l: pmic-int-l {
rockchip,pins = <1 RK_PC2 0 &pcfg_pull_up>;
};
+ vsel1_gpio: vsel1-gpio {
+ rockchip,pins =
+ <1 RK_PC1 0 &pcfg_pull_down>;
+ };
+ vsel2_gpio: vsel2-gpio {
+ rockchip,pins =
+ <1 RK_PB6 0 &pcfg_pull_down>;
+ };
+
+ soc_slppin_gpio: soc-slppin-gpio {
+ rockchip,pins =
+ <1 RK_PA5 0 &pcfg_output_low>;
+ };
+
+ soc_slppin_slp: soc-slppin-slp {
+ rockchip,pins =
+ <1 RK_PA5 1 &pcfg_pull_down>;
+ };
+
+ soc_slppin_rst: soc-slppin-rst {
+ rockchip,pins =
+ <1 RK_PA5 2 &pcfg_pull_none>;
+ };
};
sdio-pwrseq {
