Re: [PATCH v5 5/9] dt-bindings: fsi: Document the FSI controller common properties

[Eddie James]

On 5/15/24 09:35, Krzysztof Kozlowski wrote:
> On 15/05/2024 16:28, Eddie James wrote:
> > On 5/15/24 09:18, Krzysztof Kozlowski wrote:
> > > On 14/05/2024 21:54, Eddie James wrote:
> > > > Since there are multiple FSI controllers documented, the common
> > > > properties should be documented separately and then referenced
> > > > from the specific controller documentation.
> > > >
> > > > Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
> > > > ---
> > > > Changes since v4:
> > > >    - Add interrupt controller properties
> > > >    - Add clock-frequency property to FSI controller and CFAM
> > > >    - Add detail to chip-id property description
> > > >
> > > >    .../bindings/fsi/fsi-controller.yaml          | 66 +++++++++++++++++++
> > > >    1 file changed, 66 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/fsi/fsi-controller.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/fsi/fsi-controller.yaml b/Documentation/devicetree/bindings/fsi/fsi-controller.yaml
> > > > new file mode 100644
> > > > index 0000000000000..8620e4da6de77
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/fsi/fsi-controller.yaml
> > > > @@ -0,0 +1,66 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/fsi/fsi-controller.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: FSI Controller Common Properties
> > > > +
> > > > +maintainers:
> > > > +  - Eddie James <eajames@xxxxxxxxxxxxx>
> > > > +
> > > > +description:
> > > > +  FSI (FRU (Field Replaceable Unit) Service Interface) is a two wire bus. The
> > > > +  FSI bus is connected to a CFAM (Common FRU Access Macro) which contains
> > > > +  various engines such as I2C controllers, SPI controllers, etc.
> > > > +
> > > > +properties:
> > > > +  "#address-cells":
> > > > +    const: 2
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > > > +
> > > > +  '#interrupt-cells':
> > > > +    const: 1
> > > > +
> > > > +  clock-frequency:
> > > > +    minimum: 1
> > > > +    maximum: 200000000
> > > This is a deprecated property in general. Why did it appear? It does not
> > > exist in current bindings and nothing in commit msg suggests changes in
> > > the bindings themselves.
> >
> > OK, is there some document that describes what properties are
> > deprecated? Because it's used all over the place in the bindings. Anyway
> dtschema: dtschema/schemas/clock/clock.yaml
>
> buses anyway should use bus-frequency but it is also legacy one.
>
> > I need this property, I can rename it if you like. I can also update the
> Why do you need it? Why clocks cannot be chosen by drivers and initial
> state selected by assigned-clock-rates?


Well, I could use assigned-clock-rates, though it seems like I'd then 
have to implement the clock provider framework for both the FSI 
controller driver and the CFAM driver, which is a lot of extra work. FSI 
controller isn't really a clock provider, it's a bus like i2c (which 
uses clock-frequency), so it doesn't quite fit in my opinion...


Thanks for your detailed review Krzysztof,

Eddie


> > commit message to indicate that I'm adding it.
>
>
> Best regards,
> Krzysztof