On 02/10/2024 11:20, Michael Wu wrote: >> On Tue, Oct 01, 2024 at 04:29:33PM +0800, Michael Wu wrote: >>> Since there are no registers controlling the hardware parameters >>> IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION, their values can only be >>> declared in the device tree. >>> >>> snps,bus-capacitance-pf indicates the bus capacitance in picofarads (pF). >>> It affects the high and low pulse width of SCL line in high speed mode. >>> The legal values for this property are 100 and 400 only, and default >>> value is 100. This property corresponds to IC_CAP_LOADING. >>> >>> snps,clk-freq-optimized indicates whether the hardware input clock >>> frequency is reduced by reducing the internal latency. This property >>> corresponds to IC_CLK_FREQ_OPTIMIZATION. >>> >>> The driver can calculate hs_hcnt and hs_lcnt appropriate for the hardware >>> based on these two properties. >>> >>> Signed-off-by: Michael Wu <michael.wu@xxxxxxxxx> >>> --- >>> .../bindings/i2c/snps,designware-i2c.yaml | 24 >> +++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git >> a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml >> b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml >>> index 60035a787e5c..c373f3acd34b 100644 >>> --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml >>> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml > ... >>> + This property indicates the bus capacitance in picofarads (pF). >>> + This value is used to compute the tHIGH and tLOW periods for high >> speed >>> + mode. >>> + default: 100 >> >> I asked for some constraints here. min/maximum. I think you never >> replied to this. >> > > In I2C DesignWare Databook v2.03a the mandatory option is provided to > select whether the bus capacitance is 400pF or 100pF. It presents the > description like that: > > Description: > For high speed mode, the bus loading (pF) affects the high and low > pulse width of SCL. > Values: 100, 400 > Default Value: 100 > Parameter Name: IC_CAP_LOADING > > There is no further information describing this option except to the > declaration of legal values above, let alone minimum and maximum limits. So only two values are valid? Then not min/max but enum. > As a user I don't think I have the right to define a value range for the > vendor. > > From the information provided in the data sheet, I prefer to list the > legal values like the following: > > enum: [100, 400] > default: 100 > > instead of declaring its range. What do you think? > > In patches v2 I used if (dev->bus_capacitance_pf == 400) {... } else {...} > and other statements in the driver code to indicate that the capacitance > can only be 400pf or not. Maybe this is a metaphor. I'm sorry that I > wasn't more explicit about the constraints. > >>> + >>> + snps,clk-freq-optimized: >>> + description: > >>> + This property indicates whether the hardware input clock frequency >> is >>> + reduced by reducing the internal latency. This value is used to >> compute >>> + the tHIGH and tLOW periods for high speed mode. >>> + type: boolean >>> + >>> unevaluatedProperties: false >>> >>> required: >>> @@ -146,4 +161,13 @@ examples: >>> interrupts = <8>; >>> clocks = <&ahb_clk>; >>> }; >>> + - | >>> + i2c@c5000000 { >>> + compatible = "snps,designware-i2c"; >> >> Extend EXISTING example. Not add new example. > > Should I insert these two properties into one or all existing examples? Into any example, where it looks reasonable. Best regards, Krzysztof