Re: [PATCH v3 1/2] dt-bindings: i2c: snps,designware-i2c: declare bus capacitance and clk freq optimized

[Michael Wu]

> 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.
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?

Thanks & Regards
Michael