Re: [PATCH] regulator: fan53555: add back tcs4526
From: Ezequiel Garcia
Date: Thu May 27 2021 - 07:51:50 EST
Hi Rudi,
Thanks for the patch.
On Thu, 2021-05-27 at 10:59 +0000, Rudi Heitbaum wrote:
> 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;
As Peter rightly said, this will prevent gpu devfreq from working.
> +};
> +
> +&vopl {
> + status = "disabled";
Out of curiosity, why disabling the little VOP?
> +};
> +
> &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";
I can be wrong, but I think regulator-compatible is deprecated.
> + pinctrl-0 = <&vsel1_gpio>;
> + vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
Is vsel-gpios ever used in the mainline driver?
> + 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";
Ditto.
> + pinctrl-0 = <&vsel2_gpio>;
> + vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
Ditto.
> + 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;
Just out of curiosity, is regulator-boot-on really needed for the GPU?
Thanks,
Ezequiel