From: Rudi Heitbaum <rudi@heitbaum.com> To: Peter Geis <pgwipeout@gmail.com> Cc: devicetree@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>, Mark Brown <broonie@kernel.org>, Ezequiel Garcia <ezequiel@collabora.com>, chenjh@rock-chips.com Subject: Re: [PATCH] regulator: fan53555: add back tcs4526 Date: Thu, 27 May 2021 10:59:47 +0000 [thread overview] Message-ID: <20210527105943.GA441@7698f5da3a10> (raw) In-Reply-To: <CAMdYzYpZoKs3P62j02RW-+5BEpqC9JL3apjucTWLWmvNFrOrCg@mail.gmail.com> 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 {
WARNING: multiple messages have this Message-ID (diff)
From: Rudi Heitbaum <rudi@heitbaum.com> To: Peter Geis <pgwipeout@gmail.com> Cc: devicetree@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>, Mark Brown <broonie@kernel.org>, Ezequiel Garcia <ezequiel@collabora.com>, chenjh@rock-chips.com Subject: Re: [PATCH] regulator: fan53555: add back tcs4526 Date: Thu, 27 May 2021 10:59:47 +0000 [thread overview] Message-ID: <20210527105943.GA441@7698f5da3a10> (raw) In-Reply-To: <CAMdYzYpZoKs3P62j02RW-+5BEpqC9JL3apjucTWLWmvNFrOrCg@mail.gmail.com> 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 { _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
next prev parent reply other threads:[~2021-05-27 11:00 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-26 16:23 [PATCH] regulator: fan53555: add back tcs4526 Rudi Heitbaum 2021-05-26 16:23 ` Rudi Heitbaum 2021-05-26 18:41 ` Peter Geis 2021-05-26 18:41 ` Peter Geis 2021-05-27 10:59 ` Rudi Heitbaum [this message] 2021-05-27 10:59 ` Rudi Heitbaum 2021-05-27 11:26 ` Peter Geis 2021-05-27 11:26 ` Peter Geis 2021-05-27 13:03 ` Mark Brown 2021-05-27 13:03 ` Mark Brown 2021-05-27 11:51 ` Ezequiel Garcia 2021-05-27 11:51 ` Ezequiel Garcia 2021-05-27 12:29 ` Rudi Heitbaum 2021-05-27 12:29 ` Rudi Heitbaum 2021-05-27 13:05 ` Mark Brown 2021-05-27 13:05 ` Mark Brown 2021-05-28 10:27 ` Rudi Heitbaum 2021-05-28 10:27 ` Rudi Heitbaum 2021-05-28 10:19 ` [PATCH v2] regulator: fan53555: add tcs4526 Rudi Heitbaum 2021-05-28 10:19 ` Rudi Heitbaum 2021-06-01 12:56 ` Mark Brown 2021-06-01 12:56 ` Mark Brown 2021-06-04 16:32 ` Mark Brown 2021-06-04 16:32 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210527105943.GA441@7698f5da3a10 \ --to=rudi@heitbaum.com \ --cc=broonie@kernel.org \ --cc=chenjh@rock-chips.com \ --cc=devicetree@vger.kernel.org \ --cc=ezequiel@collabora.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=pgwipeout@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.