dt-bindings: regulator: fcs,fan53555: Remove vsel-gpios property
Subsystems (2)
PATCH 1/1 dt-bindings: regulator: fcs,fan53555: Remove vsel-gpios property
The vsel-gpios property is currently documented in the binding but is not used by the driver. The FAN53555 family of regulators supports two voltage selector registers (VSEL0/VSEL1), and the selection between them is intended to be controlled by an external hardware pin (VSEL). However, the driver does not support dynamic toggling of this pin via a GPIO, it only uses the fcs,suspend-voltage-selector property to statically assign which register is used for runtime voltage and which for suspend voltage.
Remove the vsel-gpios property from the binding to prevent incorrect DT usage and to reflect the actual hardware description supported by the driver.
--- .../devicetree/bindings/regulator/fcs,fan53555.yaml | 5 ----- 1 file changed, 5 deletions(-)
Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml +0 -5
@@ -43,11 +43,6 @@ properties: vin-supply: description: Supply for the vin pin - vsel-gpios: - description: Voltage Select. When this pin is LOW, VOUT is set by the - VSEL0 register. When this pin is HIGH, VOUT is set by the VSEL1 register. - maxItems: 1 - required: - compatible - reg
Signature · 1 line
2.52.0
Quoted 11 lines · Alexander Shiyan
The vsel-gpios property is currently documented in the binding but is not used by the driver. The FAN53555 family of regulators supports two voltage selector registers (VSEL0/VSEL1), and the selection between them is intended to be controlled by an external hardware pin (VSEL). However, the driver does not support dynamic toggling of this pin via a GPIO, it only uses the fcs,suspend-voltage-selector property to statically assign which register is used for runtime voltage and which for suspend voltage.
Remove the vsel-gpios property from the binding to prevent incorrect DT usage and to reflect the actual hardware description supported by the driver.
From the wording/justification here, I disagree with this patch. The binding should document what the hardware can do, not what the driver can.
Maybe instead you should make fcs,suspend-voltage-selector mutually exclusive with vsel-gpios?
Cheers,
Conor.
pw-bot: changes-requested
Quoted 25 lines
Signed-off-by Alexander Shiyan <[email protected]>--- .../devicetree/bindings/regulator/fcs,fan53555.yaml | 5 ----- 1 file changed, 5 deletions(-)Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml +0
-5@@ -43,11 +43,6 @@ properties: vin-supply: description: Supply for the vin pin - vsel-gpios: - description: Voltage Select. When this pin is LOW, VOUT is set by the - VSEL0 register. When this pin is HIGH, VOUT is set by the VSEL1 register. - maxItems: 1 - required: - compatible - regSignature · 1 line
2.52.0
Hello Comor.
Quoted 13 lines · Alexander Shiyan
Quoted 11 lines
The vsel-gpios property is currently documented in the binding but is not used by the driver. The FAN53555 family of regulators supports two voltage selector registers (VSEL0/VSEL1), and the selection between them is intended to be controlled by an external hardware pin (VSEL). However, the driver does not support dynamic toggling of this pin via a GPIO, it only uses the fcs,suspend-voltage-selector property to statically assign which register is used for runtime voltage and which for suspend voltage.
Remove the vsel-gpios property from the binding to prevent incorrect DT usage and to reflect the actual hardware description supported by the driver.
Quoted 6 lines
From the wording/justification here, I disagree with this patch. The binding should document what the hardware can do, not what the driver can.
Maybe instead you should make fcs,suspend-voltage-selector mutually exclusive with vsel-gpios?
The main problem here is that this feature (vsel-gpios) was never implemented in the driver. So, the patch consists solely of removing a non-existent property.
Thanks!
Quoted 19 lines · Alexander Shiyan
Hello Comor.
Quoted 17 lines · Alexander Shiyan
Quoted 11 lines
The vsel-gpios property is currently documented in the binding but is not used by the driver. The FAN53555 family of regulators supports two voltage selector registers (VSEL0/VSEL1), and the selection between them is intended to be controlled by an external hardware pin (VSEL). However, the driver does not support dynamic toggling of this pin via a GPIO, it only uses the fcs,suspend-voltage-selector property to statically assign which register is used for runtime voltage and which for suspend voltage.
Remove the vsel-gpios property from the binding to prevent incorrect DT usage and to reflect the actual hardware description supported by the driver.From the wording/justification here, I disagree with this patch. The binding should document what the hardware can do, not what the driver can.
Quoted 6 lines
Quoted 2 lines
Maybe instead you should make fcs,suspend-voltage-selector mutually exclusive with vsel-gpios?
The main problem here is that this feature (vsel-gpios) was never implemented in the driver. So, the patch consists solely of removing a non-existent property.
I'm not disputing that, but if the functionality is supported by the hardware the existence of the property is fine. Bindings should be complete, even if the driver in linux doesn't use something.
Rob's 2023 patch that added the property even says "Add the undocumented 'vsel-gpios' property used to control the VSEL pin.", so having it was an intentional decision by the main binding maintainer!
Mark Brown
maintainerQuoted 3 lines · Conor Dooley
Rob's 2023 patch that added the property even says "Add the undocumented 'vsel-gpios' property used to control the VSEL pin.", so having it was an intentional decision by the main binding maintainer!
Also sounds like the property was used by the code at some point in the past...
Mark Brown
maintainerDocumentation/devicetree/bindings/regulator/fcs,fan53555.yaml properties: · line 49
Quoted 4 lines · Alexander Shiyan
Documentation/devicetree/bindings/regulator/fcs,fan53555.yaml +0
-4- vsel-gpios: - description: Voltage Select. When this pin is LOW, VOUT is set by the - VSEL0 register. When this pin is HIGH, VOUT is set by the VSEL1 register. - maxItems: 1
In addition to Conor's concerns have you checked for existing usage?
Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone.