| Message ID | 20220705094239.17174-17-johan+linaro@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | phy: qcom,qmp: fix dt-bindings and deprecate lane suffix | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 05/07/2022 11:42, Johan Hovold wrote: > Only UFS PHY nodes in mainline have a vddp-ref-clk supply. Drop it from > the PCIe PHY binding. > That's not really good reason. Either the hardware uses ref-clk supply or not. Now it looks like you copied everything from common schema and clean things up. That's not how it should be organize. It's okay to copy existing bindings which are applicable and then in separate patch deprecate things or remove pieces which are not correct. But all this in assumption that the first copy already selected only applicable parts. Best regards, Krzysztof
On Tue, Jul 05, 2022 at 12:16:34PM +0200, Krzysztof Kozlowski wrote: > On 05/07/2022 11:42, Johan Hovold wrote: > > Only UFS PHY nodes in mainline have a vddp-ref-clk supply. Drop it from > > the PCIe PHY binding. > > > > That's not really good reason. Either the hardware uses ref-clk supply > or not. Now it looks like you copied everything from common schema and > clean things up. That's not how it should be organize. Yes, and I've been pretty clear that that's how I'm going about to disentangle the current binding. > It's okay to copy existing bindings which are applicable and then in > separate patch deprecate things or remove pieces which are not correct. > But all this in assumption that the first copy already selected only > applicable parts. But how would you be able to tell what parts I left out from the original copy unless I first do the split and then explicitly remove things that were presumably *never* applicable and just happened to be added because all bindings where combined in one large mess of a schema? Johan
On 05/07/2022 13:46, Johan Hovold wrote: >> It's okay to copy existing bindings which are applicable and then in >> separate patch deprecate things or remove pieces which are not correct. >> But all this in assumption that the first copy already selected only >> applicable parts. > > But how would you be able to tell what parts I left out from the > original copy They are obvious and immediately visible. I see old bindings and new bindings - no troubles to compare. I review new bindings - everything in place. I don't want to review old code, inapplicable code. The patch I am reviewing (the one doing the split) must bring correct bindings, except these few differences like deprecated stuff. > unless I first do the split and then explicitly remove > things that were presumably *never* applicable and just happened to be > added because all bindings where combined in one large mess of a schema? Best regards, Krzysztof
On Tue, Jul 05, 2022 at 01:59:26PM +0200, Krzysztof Kozlowski wrote: > On 05/07/2022 13:46, Johan Hovold wrote: > >> It's okay to copy existing bindings which are applicable and then in > >> separate patch deprecate things or remove pieces which are not correct. > >> But all this in assumption that the first copy already selected only > >> applicable parts. > > > > But how would you be able to tell what parts I left out from the > > original copy > > They are obvious and immediately visible. I see old bindings and new > bindings - no troubles to compare. I review new bindings - everything in > place. Heh, with all these conditionals in place that may be harder than it sounds. > I don't want to review old code, inapplicable code. The patch I am > reviewing (the one doing the split) must bring correct bindings, except > these few differences like deprecated stuff. Sure, I get that. But this very patch is an example of why I tried to remove things explicitly instead folding this into the original patch and risking it not being noticed. It's not always obvious what is applicable and what is not, especially when the old schema is in the state it is. > > unless I first do the split and then explicitly remove > > things that were presumably *never* applicable and just happened to be > > added because all bindings where combined in one large mess of a schema? So you suggest we keep this regulator for all PHY variants even though it was probably only needed for UFS on some older SoCs? Johan
On 05/07/2022 14:43, Johan Hovold wrote: > On Tue, Jul 05, 2022 at 01:59:26PM +0200, Krzysztof Kozlowski wrote: >> On 05/07/2022 13:46, Johan Hovold wrote: >>>> It's okay to copy existing bindings which are applicable and then in >>>> separate patch deprecate things or remove pieces which are not correct. >>>> But all this in assumption that the first copy already selected only >>>> applicable parts. >>> >>> But how would you be able to tell what parts I left out from the >>> original copy >> >> They are obvious and immediately visible. I see old bindings and new >> bindings - no troubles to compare. I review new bindings - everything in >> place. > > Heh, with all these conditionals in place that may be harder than it > sounds. True and your patchset split does not make it easier. > >> I don't want to review old code, inapplicable code. The patch I am >> reviewing (the one doing the split) must bring correct bindings, except >> these few differences like deprecated stuff. > > Sure, I get that. But this very patch is an example of why I tried to > remove things explicitly instead folding this into the original patch > and risking it not being noticed. > > It's not always obvious what is applicable and what is not, especially > when the old schema is in the state it is. Unless bindings are very precise, usually it's not visible what is applicable or not, so there is just no benefit in multi-step approach in split from old bindings. The same as with conversion of bindings, the assumption is that original file was not correct, so we review the final file. > >>> unless I first do the split and then explicitly remove >>> things that were presumably *never* applicable and just happened to be >>> added because all bindings where combined in one large mess of a schema? > > So you suggest we keep this regulator for all PHY variants even though > it was probably only needed for UFS on some older SoCs? No. I commented only that reason is not a good one. The proper reason could be: there is or there is no such pin in the device or the history tells that adding it for all variants was a mistake. Best regards, Krzysztof
On Tue, Jul 05, 2022 at 08:13:55PM +0200, Krzysztof Kozlowski wrote: > On 05/07/2022 14:43, Johan Hovold wrote: > > So you suggest we keep this regulator for all PHY variants even though > > it was probably only needed for UFS on some older SoCs? > > No. I commented only that reason is not a good one. The proper reason > could be: there is or there is no such pin in the device or the history > tells that adding it for all variants was a mistake. Ok, I'll include it since it was there as an optional regulator from the start and hence implicitly made part of the binding for later PHYs even though it likely only applies to the older MSM8996/98. If anyone has access to the documentation they can drop it later. Johan
diff --git a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml index 557cccc8f4dd..ff1577f68a00 100644 --- a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml @@ -66,10 +66,6 @@ properties: description: Phandle to 1.8V regulator supply to PHY refclk pll block. - vddp-ref-clk-supply: - description: - Phandle to a regulator supply to any specific refclk pll block. - patternProperties: "^phy@[0-9a-f]+$": type: object
Only UFS PHY nodes in mainline have a vddp-ref-clk supply. Drop it from the PCIe PHY binding. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml | 4 ---- 1 file changed, 4 deletions(-)