| Message ID | 20241218152842.97483-1-frieder@fris.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use correct LDO5 control registers for PCA9450 | expand |
Hi, On 18.12.24 4:27 PM, Frieder Schrempf wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > This is a follow-up of [1]. > > The main objective of this is to fix the PCA9450 driver to > use the correct control register for the LDO5 regulator. > > Currently the control register to use for LDO5 is hardcoded to > LDO5CTRL_H. This is wrong for two reasons: > > 1. LDO5CTRL_H doesn't contain the bits for enabling/disabling > the regulator. Only LDO5CTRL_L does. > > 2. The actual output voltage of the regulator is determined by > the LDO5CTRL_H only if the SD_VSEL input is HIGH. If it is > low, then LDO5CTRL_L is used. The driver does not take this > into account. > > This can cause several problems: > > 1. LDO5 can not be turned on/off and we rely on the bootloader > to leave it turned on. On the other hand we can't save > power if LDO5 is unused. > > 2. There is a potential for corner-cases where switching > SD_VSEL via USDHC_VSELECT and writing to the (wrong) > control register can cause wrong output voltage and therfore > SD card failures (not observed yet). > > 3. Reading the current voltage of the LDO5 regulator (e. g. via > sysfs can yield the wrong value as the voltage is read from > the wrong control register. > > At the same time there is now hardware that hardwires SD_VSEL > to a fixed LOW level and therefore relies on switching the > voltage only via a single control register. We add support for > this through an additional property "nxp,sd-vsel-fixed-low" in > the LDO5 node. > > Summary of binding changes (patch 1): > > 1. Adjust the bindings to remove the old and abandoned use of > sd-vsel-gpios property. > > 2. Adjust the bindings to use sd-vsel-gpios in the LDO5 node to > retrieve an input that can be used to sample the SD_VSEL > status. > > 3. Adjust bindings to allow "nxp,sd-vsel-fixed-low" to be used > for boards that have SD_VSEL hardwired to low level. > > Summary of driver changes (patch 2-6): > > 1. Remove the old sd-vsel-gpios handling. > > 2. Use the new sd-vsel-gpios property to determine the correct > control register for LDO5. > > 3. Fix the enable register for LDO5. > > 4. Support hardware with fixed low level of SD_VSEL. > > Summary of devicetree changes (patch 7-9): > > Implement the changes in the devicetrees for Kontron hardware > (i.MX8MM, i.MX8MP and i.MX93). > > Changelog: > > v2 -> v3: > > * Merge binding patches into one and extend commit message > * Rebase to next-20241218 > > v1 -> v2: > > * Split binding patch > * Add solution for hardwired SD_VSEL > * Leave regulator core untouched as requested by Mark > * Add devicetree changes for i.MX8MP and i.MX93 > > [1] https://lore.kernel.org/lkml/20230213155833.1644366-1-frieder@fris.de/ > > Frieder Schrempf (9): > dt-bindings: regulator: pca9450: Add properties for handling LDO5 > arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT > Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5" > regulator: pca9450: Fix control register for LDO5 > regulator: pca9450: Fix enable register for LDO5 > regulator: pca9450: Handle hardware with fixed SD_VSEL for LDO5 > arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal > arm64: dts: imx93-kontron: Fix SD card IO voltage control > arm64: dts: imx8mp-kontron: Add support for reading SD_VSEL signal > > .../regulator/nxp,pca9450-regulator.yaml | 29 ++++- > .../boot/dts/freescale/imx8mm-kontron-bl.dts | 10 +- > .../dts/freescale/imx8mm-kontron-osm-s.dtsi | 7 +- > .../dts/freescale/imx8mp-kontron-osm-s.dtsi | 7 +- > .../boot/dts/freescale/imx8mp-skov-reva.dtsi | 5 +- > .../dts/freescale/imx93-kontron-osm-s.dtsi | 5 +- > drivers/regulator/pca9450-regulator.c | 103 +++++++++++++++--- > 7 files changed, 133 insertions(+), 33 deletions(-) > I just want to ask kindly if there is any feedback for the driver and devicetree changes in this set? Thanks Frieder
On Tue, Jan 28, 2025 at 08:44:28AM +0100, Frieder Schrempf wrote: > I just want to ask kindly if there is any feedback for the driver and > devicetree changes in this set? Please don't send content free pings and please allow a reasonable time for review. People get busy, go on holiday, attend conferences and so on so unless there is some reason for urgency (like critical bug fixes) please allow at least a couple of weeks for review. If there have been review comments then people may be waiting for those to be addressed. Sending content free pings adds to the mail volume (if they are seen at all) which is often the problem and since they can't be reviewed directly if something has gone wrong you'll have to resend the patches anyway, so sending again is generally a better approach though there are some other maintainers who like them - if in doubt look at how patches for the subsystem are normally handled.
On Wed, 18 Dec 2024 16:27:23 +0100, Frieder Schrempf wrote: > This is a follow-up of [1]. > > The main objective of this is to fix the PCA9450 driver to > use the correct control register for the LDO5 regulator. > > Currently the control register to use for LDO5 is hardcoded to > LDO5CTRL_H. This is wrong for two reasons: > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/9] dt-bindings: regulator: pca9450: Add properties for handling LDO5 commit: f9cbf56b0a1966d977df87d15a5bdbff2c342062 [2/9] arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT commit: b5ec74c2aec76fbdff9bc16951455602e11902bf [3/9] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5" commit: c73be62caabbec6629689c705aea65e5ce364d5d [4/9] regulator: pca9450: Fix control register for LDO5 commit: 3ce6f4f943ddd9edc03e450a2a0d89cb025b165b [5/9] regulator: pca9450: Fix enable register for LDO5 commit: f5aab0438ef17f01c5ecd25e61ae6a03f82a4586 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
On Wed, Dec 18, 2024 at 04:27:23PM +0100, Frieder Schrempf wrote: > Frieder Schrempf (9): > arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT ... > arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal > arm64: dts: imx93-kontron: Fix SD card IO voltage control > arm64: dts: imx8mp-kontron: Add support for reading SD_VSEL signal Applied all 4 DTS patches, thanks!
Hi Mark, On 03.02.25 8:57 PM, Mark Brown wrote: > On Wed, 18 Dec 2024 16:27:23 +0100, Frieder Schrempf wrote: >> This is a follow-up of [1]. >> >> The main objective of this is to fix the PCA9450 driver to >> use the correct control register for the LDO5 regulator. >> >> Currently the control register to use for LDO5 is hardcoded to >> LDO5CTRL_H. This is wrong for two reasons: >> >> [...] > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next > > Thanks! > > [1/9] dt-bindings: regulator: pca9450: Add properties for handling LDO5 > commit: f9cbf56b0a1966d977df87d15a5bdbff2c342062 > [2/9] arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT > commit: b5ec74c2aec76fbdff9bc16951455602e11902bf > [3/9] Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5" > commit: c73be62caabbec6629689c705aea65e5ce364d5d > [4/9] regulator: pca9450: Fix control register for LDO5 > commit: 3ce6f4f943ddd9edc03e450a2a0d89cb025b165b > [5/9] regulator: pca9450: Fix enable register for LDO5 > commit: f5aab0438ef17f01c5ecd25e61ae6a03f82a4586 > Thanks for picking up these patches. Meanwhile Shawn has applied patch 2 and 7-9 on his side. What about patch 6? Is there a specific reason why this was left out? Thanks Frieder
From: Frieder Schrempf <frieder.schrempf@kontron.de> This is a follow-up of [1]. The main objective of this is to fix the PCA9450 driver to use the correct control register for the LDO5 regulator. Currently the control register to use for LDO5 is hardcoded to LDO5CTRL_H. This is wrong for two reasons: 1. LDO5CTRL_H doesn't contain the bits for enabling/disabling the regulator. Only LDO5CTRL_L does. 2. The actual output voltage of the regulator is determined by the LDO5CTRL_H only if the SD_VSEL input is HIGH. If it is low, then LDO5CTRL_L is used. The driver does not take this into account. This can cause several problems: 1. LDO5 can not be turned on/off and we rely on the bootloader to leave it turned on. On the other hand we can't save power if LDO5 is unused. 2. There is a potential for corner-cases where switching SD_VSEL via USDHC_VSELECT and writing to the (wrong) control register can cause wrong output voltage and therfore SD card failures (not observed yet). 3. Reading the current voltage of the LDO5 regulator (e. g. via sysfs can yield the wrong value as the voltage is read from the wrong control register. At the same time there is now hardware that hardwires SD_VSEL to a fixed LOW level and therefore relies on switching the voltage only via a single control register. We add support for this through an additional property "nxp,sd-vsel-fixed-low" in the LDO5 node. Summary of binding changes (patch 1): 1. Adjust the bindings to remove the old and abandoned use of sd-vsel-gpios property. 2. Adjust the bindings to use sd-vsel-gpios in the LDO5 node to retrieve an input that can be used to sample the SD_VSEL status. 3. Adjust bindings to allow "nxp,sd-vsel-fixed-low" to be used for boards that have SD_VSEL hardwired to low level. Summary of driver changes (patch 2-6): 1. Remove the old sd-vsel-gpios handling. 2. Use the new sd-vsel-gpios property to determine the correct control register for LDO5. 3. Fix the enable register for LDO5. 4. Support hardware with fixed low level of SD_VSEL. Summary of devicetree changes (patch 7-9): Implement the changes in the devicetrees for Kontron hardware (i.MX8MM, i.MX8MP and i.MX93). Changelog: v2 -> v3: * Merge binding patches into one and extend commit message * Rebase to next-20241218 v1 -> v2: * Split binding patch * Add solution for hardwired SD_VSEL * Leave regulator core untouched as requested by Mark * Add devicetree changes for i.MX8MP and i.MX93 [1] https://lore.kernel.org/lkml/20230213155833.1644366-1-frieder@fris.de/ Frieder Schrempf (9): dt-bindings: regulator: pca9450: Add properties for handling LDO5 arm64: dts: imx8mp-skov-reva: Use hardware signal for SD card VSELECT Revert "regulator: pca9450: Add SD_VSEL GPIO for LDO5" regulator: pca9450: Fix control register for LDO5 regulator: pca9450: Fix enable register for LDO5 regulator: pca9450: Handle hardware with fixed SD_VSEL for LDO5 arm64: dts: imx8mm-kontron: Add support for reading SD_VSEL signal arm64: dts: imx93-kontron: Fix SD card IO voltage control arm64: dts: imx8mp-kontron: Add support for reading SD_VSEL signal .../regulator/nxp,pca9450-regulator.yaml | 29 ++++- .../boot/dts/freescale/imx8mm-kontron-bl.dts | 10 +- .../dts/freescale/imx8mm-kontron-osm-s.dtsi | 7 +- .../dts/freescale/imx8mp-kontron-osm-s.dtsi | 7 +- .../boot/dts/freescale/imx8mp-skov-reva.dtsi | 5 +- .../dts/freescale/imx93-kontron-osm-s.dtsi | 5 +- drivers/regulator/pca9450-regulator.c | 103 +++++++++++++++--- 7 files changed, 133 insertions(+), 33 deletions(-)