[PATCH] regulator: fan53555: add back tcs4526

[PATCH] regulator: fan53555: add back tcs4526

[Rudi Heitbaum]

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@heitbaum.com>
---
 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,
 };
 
@@ -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
 	},
 	{ }
 };
@@ -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

Re: [PATCH] regulator: fan53555: add back tcs4526

[Peter Geis]

On Wed, May 26, 2021 at 12:23 PM Rudi Heitbaum <rudi@heitbaum.com> 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@heitbaum.com>

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.

I also don't see the TCS4525/TCS4526 regulators in the current
linux-next device tree for the N10.

> ---
>  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.

>  };
>
> @@ -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.

>         },
>         { }
>  };
> @@ -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
>

Re: [PATCH] regulator: fan53555: add back tcs4526

[Rudi Heitbaum]

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@heitbaum.com> 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@heitbaum.com>
> 
> 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 {

Re: [PATCH] regulator: fan53555: add back tcs4526

[Peter Geis]

On Thu, May 27, 2021 at 6:59 AM Rudi Heitbaum <rudi@heitbaum.com> 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@heitbaum.com> 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@heitbaum.com>
> >
> > 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?

It's fine to have both compatibles (and avoids confusion in
device-trees), just remember to update the dt-bindings as well.
It's funny to see drivers with both schemes, so we really have to
decide which path we want to go down.
Considering the syr827/syr828 as convention, we should probably just
go down that route for consistency within the driver.

>
> > >         },
> > >         { }
> > >  };
> > > @@ -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;

Removal of the operating points kind of makes the gpu regulator moot,
don't you think?

> +};
> +
> +&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 {

Re: [PATCH] regulator: fan53555: add back tcs4526

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

On Thu, May 27, 2021 at 07:26:01AM -0400, Peter Geis wrote:
> On Thu, May 27, 2021 at 6:59 AM Rudi Heitbaum <rudi@heitbaum.com> wrote:

> > > 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.

..

> > 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?

> It's fine to have both compatibles (and avoids confusion in
> device-trees), just remember to update the dt-bindings as well.
> It's funny to see drivers with both schemes, so we really have to
> decide which path we want to go down.
> Considering the syr827/syr828 as convention, we should probably just
> go down that route for consistency within the driver.

It is generally safer for the DT to be explicit about exactly
what the hardware is and then double check that this matches the
actual hardware, this gives more room for handling things with
quirks if needed in future and makes the life of people writing
DTs for boards easier since they don't need to remap part
numbers from the schematic to the DT.

> > +&gpu {
> > +       mali-supply = <&vdd_gpu>;
> > +       assigned-clocks = <&cru ACLK_GPU>;
> > +       assigned-clock-rates = <200000000>;
> > +       status = "okay";
> > +       /delete-property/ operating-points-v2;

> Removal of the operating points kind of makes the gpu regulator moot,
> don't you think?

It's still better to say what the supply is even if it can't be
changed - that stops you getting warnings about substituting in a
dummy regulator and allows the consumer to read the current state
of the regulator in case that's useful.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: fan53555: add back tcs4526

[Ezequiel Garcia]

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@heitbaum.com> 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@heitbaum.com>
> > 
> > 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

Re: [PATCH] regulator: fan53555: add back tcs4526

[Rudi Heitbaum]

Hi Ezequiel and Peter,

Thanks for the feedback.

On Thu, May 27, 2021 at 08:51:27AM -0300, Ezequiel Garcia wrote:
> 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@heitbaum.com> wrote:

...

> > 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?
> 
> It's fine to have both compatibles (and avoids confusion in
> device-trees), just remember to update the dt-bindings as well.
> It's funny to see drivers with both schemes, so we really have to
> decide which path we want to go down.
> Considering the syr827/syr828 as convention, we should probably just
> go down that route for consistency within the driver.

Thanks Peter - I will resubmit the tcs4526 patch along these lines.

> > Removal of the operating points kind of makes the gpu regulator
> > moot, don't you think?

> As Peter rightly said, this will prevent gpu devfreq from working.

This is the draft that I have been working on within LibreELEC10, still
a ways to go I'm afraid. Having decided to getting the SBC to run
mainline kernel and u-boot. The regulator and subsequent regulator fix
will hopefully address https://bugzilla.kernel.org/show_bug.cgi?id=212917 
too. As you have identified - it is not ready for upstreaming :-)

Thank you both for the direction and pointers on the dts. I will get the
v2 patch going first.

> Out of curiosity, why disabling the little VOP?

Only disabled within my WIP dts so as to focus my attention on successful 
running of LE10 on the rk3399pro, before I move to attempting to enable 
the NPU and the other nodes.

> I can be wrong, but I think regulator-compatible is deprecated.

I will look it to this. 

Now with the addition of the regulator and Peter's fix I will start
qualifing each of the dts nodes and the correct options against the
schematic.

> > +??????????????????????????????vsel-gpios = <&gpio1 RK_PC1 GPIO_ACTIVE_HIGH>;
> 
> Is vsel-gpios ever used in the mainline driver?
>
> > +??????????????????????????????regulator-compatible = "fan53555-reg";
> 
> Ditto.
>
> > +??????????????????????????????vsel-gpios = <&gpio1 RK_PB6 GPIO_ACTIVE_HIGH>;
> 
> Ditto.
>
> > +??????????????????????????????regulator-boot-on;
> 
> Just out of curiosity, is regulator-boot-on really needed for the GPU?

Will check.

Re: [PATCH] regulator: fan53555: add back tcs4526

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Thu, May 27, 2021 at 08:51:27AM -0300, Ezequiel Garcia wrote:
> On Thu, 2021-05-27 at 10:59 +0000, Rudi Heitbaum wrote:

> > +               reg = <0x1c>;
> > +               vin-supply = <&vcc5v0_sys>;
> > +               regulator-compatible = "fan53555-reg";

> I can be wrong, but I think regulator-compatible is deprecated.

Yes.

> > +               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?

It should only be used if it's not possible to read the state of
the regulator enable from the hardware.

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: fan53555: add back tcs4526

[Rudi Heitbaum]

Hi Mark,

I have submiited the v2 patch. Thanks for the feedback too.

On Thu, May 27, 2021 at 02:05:53PM +0100, Mark Brown wrote:
> On Thu, May 27, 2021 at 08:51:27AM -0300, Ezequiel Garcia wrote:
> > On Thu, 2021-05-27 at 10:59 +0000, Rudi Heitbaum wrote:

> > > +       /delete-property/ operating-points-v2;

> > Removal of the operating points kind of makes the gpu regulator moot,
> > don't you think?

> It's still better to say what the supply is even if it can't be
> changed - that stops you getting warnings about substituting in a
> dummy regulator and allows the consumer to read the current state
> of the regulator in case that's useful.

I'll look into this.
Thank you all for the feedback and direction on the dts.

> > > +???????????????regulator-compatible = "fan53555-reg";
> 
> > I can be wrong, but I think regulator-compatible is deprecated.
> 
> Yes.

will action

> > > +???????????????regulator-boot-on;
> 
> > Just out of curiosity, is regulator-boot-on really needed for the GPU?
> 
> It should only be used if it's not possible to read the state of
> the regulator enable from the hardware.

will do further testing

[PATCH v2] regulator: fan53555: add tcs4526

[Rudi Heitbaum]

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.

Signed-off-by: Rudi Heitbaum <rudi@heitbaum.com>
---
 drivers/regulator/fan53555.c | 11 +++++++++++
 1 file changed, 11 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
@@ -93,6 +93,10 @@ enum {
 	TCS4525_CHIP_ID_12 = 12,
 };
 
+enum {
+	TCS4526_CHIP_ID_00 = 0,
+};
+
 /* IC mask revision */
 enum {
 	FAN53555_CHIP_REV_00 = 0x3,
@@ -374,6 +375,7 @@ static int fan53555_voltages_setup_silergy(struct fan53555_device_info *di)
 {
 	switch (di->chip_id) {
 	case TCS4525_CHIP_ID_12:
+	case TCS4526_CHIP_ID_00:
 		di->slew_reg = TCS4525_TIME;
 		di->slew_mask = TCS_SLEW_MASK;
 		di->slew_shift = 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
 	},
 	{ }
 };
@@ -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

Re: [PATCH v2] regulator: fan53555: add tcs4526

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

On Fri, May 28, 2021 at 10:19:50AM +0000, Rudi Heitbaum wrote:

> ---
>  drivers/regulator/fan53555.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

This is adding a new DT binding, it needs to update the binding document
too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] regulator: fan53555: add tcs4526

[Mark Brown]

On Fri, 28 May 2021 10:19:50 +0000, Rudi Heitbaum 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
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: fan53555: add tcs4526
      commit: 5eee5eced95f1b35c8567688ed52932b7e58deee

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark