Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 11 Oct 2022 14:37:44 -0500
From:   Rob Herring <robh@kernel.org>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Naresh Solanki <naresh.solanki@9elements.com>,
        devicetree@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>
Subject: Re: [PATCH v2 1/3] dt-bindings: hwmon: fan: Add fan binding to schema
Message-ID: <20221011193744.GA4051000-robh@kernel.org>
References: <20221011104739.53262-1-Naresh.Solanki@9elements.com>
 <20221011104739.53262-2-Naresh.Solanki@9elements.com>
 <d2c83c4f-653c-438e-a91a-d17b846dec5b@roeck-us.net>
 <CABqG17hBJBU2rZSq4qy6Fn=pDH9ezwL=Fjh5vhj_G0cge-07mQ@mail.gmail.com>
 <8d2e905b-8669-b7ec-c7c0-1b0d78fded92@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8d2e905b-8669-b7ec-c7c0-1b0d78fded92@roeck-us.net>
Precedence: bulk

On Tue, Oct 11, 2022 at 09:46:08AM -0700, Guenter Roeck wrote:
> 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.

Presumably fan controllers can produce a range of frequencies. If they 
need a specific frequency, then why are they programmable? Something 
outside the fan controller must have the constraint.

> Fans don't have a configurable pwm polarity. Fan controllers do,
> to match the hardware on a board.

We don't model an inverter, so it's got to go somewhere.

> Fans don't have or rely on
> a target rpm. That is a system property, configured into the
> fan controller. And so on.

Yes, but it is per fan. per fan properties/settings should go in the fan 
node IMO.

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

A fan would have some design maximum RPM depending on its mechanical 
design and lifetime requirements. A controller would have some maximum 
in terms of electrical pulse frequency or register bit sizes (depending 
how RPM or pulse counts are exposed to s/w. That should all be implied 
from the controller part and not in DT. It's the mechanical limits of 
the fan we should be defining here.

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

Certainly it needs more thought for different cases.

Rob