Re: [PATCH v2 1/3] dt-bindings: hwmon: fan: Add fan binding to schema

[Guenter Roeck]

On 10/11/22 09:12, Naresh Solanki wrote:
> Hi Guenter,
>
> fan-common is intended for fan properties. i.e., those derived from
> fan datasheets.
> For min-rpm, some fans have minimum non zero rpm like 1900rpm below which
> the fan cannot run.

I would argue the properties are for fan controllers, not for fans.
Fans don't have or depend on specific pwm frequencies. Fan controllers
do. Fans don't have a configurable pwm polarity. Fan controllers do,
to match the hardware on a board. Fans don't have or rely on
a target rpm. That is a system property, configured into the
fan controller. And so on.

If this is for fans, we'll need another set of properties for
fan controllers. The driver for max6639, being a fan controller,
would need to implement those properties.

Also, as implemented in the MAX6639, max-rpm is the fan's
rpm range, not the actual rpm. Your implementation is just
confusing, including the example in the bindings. Valid values
should be what the chip accepts, not some random value.

Really, I don't understand where you are going with this.

Guenter

> But not sure what the best approach is but for chip specific setting
> it should be in
> chip specific DT schema. Suggestion?
>
> Regards,
> Naresh Solanki
>
>
>
> 9elements GmbH, Kortumstraße 19-21, 44787 Bochum, Germany
> Email:  naresh.solanki@xxxxxxxxxxxxx
> Mobile:  +91 9538631477
>
> Sitz der Gesellschaft: Bochum
> Handelsregister: Amtsgericht Bochum, HRB 17519
> Geschäftsführung: Sebastian Deutsch, Eray Basar
>
> Datenschutzhinweise nach Art. 13 DSGVO
>
> On Tue, 11 Oct 2022 at 20:30, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > On 10/11/22 03:47, Naresh Solanki wrote:
> > > Add common fan properties bindings to a schema.
> > >
> > > Bindings for fan controllers can reference the common schema for the
> > > fan
> > >
> > > child nodes:
> > >
> > >     patternProperties:
> > >       "^fan@[0-2]":
> > >         type: object
> > >         allOf:
> > >           - $ref: fan-common.yaml#
> > >
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > ---
> > >    .../devicetree/bindings/hwmon/fan-common.yaml | 80 +++++++++++++++++++
> > >    1 file changed, 80 insertions(+)
> > >    create mode 100644 Documentation/devicetree/bindings/hwmon/fan-common.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/fan-common.yaml b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> > > new file mode 100644
> > > index 000000000000..abc8375da646
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/fan-common.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwmon/fan-common.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Common fan properties
> > > +
> > > +maintainers:
> > > +  - Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
> > > +
> > > +properties:
> > > +  max-rpm:
> > > +    description:
> > > +      Max RPM supported by fan
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  pulse-per-revolution:
> > > +    description:
> > > +      The number of pulse from fan sensor per revolution.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  target-rpm:
> > > +    description:
> > > +      Target RPM the fan should be configured during driver probe.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  pwm-frequency:
> > > +    description:
> > > +      PWM frequency for fan.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  pwm-polarity-inverse:
> > > +    description:
> > > +      PWM polarity for fan.
> > > +    type: boolean
> > > +
> > > +  label:
> > > +    description:
> > > +      Optional fan label
> > > +    $ref: /schemas/types.yaml#/definitions/string
> > > +
> >
> > Same question as before:
> >
> > How are additional common bindings, such as min-rpm or fan-divider
> > (also sometimes called fan-prescale) supposed to be handled ?
> > As additions to this schema, or individually in each driver needing/
> > using them ?
> >
> > Thanks,
> > Guenter