On 14/03/2022 04:29, Pavan Kondeti wrote: > Hi Krzysztof, > > On Thu, Mar 03, 2022 at 04:59:22PM +0100, Krzysztof Kozlowski wrote: >> On 03/03/2022 07:13, Sandeep Maheswaram wrote: >>> Add device tree bindings for SNPS phy tuning parameters. >>> >>> Signed-off-by: Sandeep Maheswaram <quic_c_sanm@xxxxxxxxxxx> >>> --- >>> .../bindings/phy/qcom,usb-snps-femto-v2.yaml | 125 +++++++++++++++++++++ >>> 1 file changed, 125 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml >>> index 0dfe691..227c097 100644 >>> --- a/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml >>> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-snps-femto-v2.yaml >>> @@ -50,6 +50,131 @@ properties: >>> vdda33-supply: >>> description: phandle to the regulator 3.3V supply node. >>> >>> + qcom,hs-disconnect: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + This adjusts the voltage level for the threshold used to >>> + detect a disconnect event at the host. Possible values are. >> >> ':', instead of full stop. >> >>> + 7 -> +21.56% >>> + 6 -> +17.43% >>> + 5 -> +13.32% >>> + 4 -> +9.73% >>> + 3 -> +6.3 >>> + 2 -> +3.17% >>> + 1 -> 0, Design default% >> >> Use "default:" instead. Here and in other places. >> >>> + 0 -> -2.72% >> >> In current form this should be an enum... but actually current form is >> wrong. You should not store register values in DT. What if next version >> of hardware has a different meaning of these values? >> >> Instead, you should store here meaningful values, not register values. >> > > Thanks for the feedback. > > The values in % really makes the tuning easy. People look at the eye diagram > and decided whether to increase/decrease the margin. The absolute values > may not be that useful. All we need is an "adjustment" here. The databook > it self does not give any absolute values. > > I agree to the "enum" suggestion which we have been following for the > qusb2 driver already. > > The values have not changed in the last 5 years for this hardware block, so > defining enums for the % values would be really helpful. I did not say you cannot store here percentages. Quite opposite - store here the percentages. Just do not store register value. No. Please read my comment again - meaningful values are needed. Best regards, Krzysztof
