From: Rob Herring <robh@kernel.org>
To: Billy Tsai <billy_tsai@aspeedtech.com>
Cc: "jdelvare@suse.com" <jdelvare@suse.com>,
"linux@roeck-us.net" <linux@roeck-us.net>,
"krzysztof.kozlowski+dt@linaro.org"
<krzysztof.kozlowski+dt@linaro.org>,
"joel@jms.id.au" <joel@jms.id.au>,
"andrew@aj.id.au" <andrew@aj.id.au>,
"corbet@lwn.net" <corbet@lwn.net>,
"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"naresh.solanki@9elements.com" <naresh.solanki@9elements.com>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
BMC-SW <BMC-SW@aspeedtech.com>,
"patrick@stwcx.xyz" < patrick@stwcx.xyz>
Subject: Re: [PATCH v8 1/3] dt-bindings: hwmon: fan: Add fan binding to schema
Date: Thu, 7 Sep 2023 14:43:51 -0500 [thread overview]
Message-ID: <20230907194351.GA2033402-robh@kernel.org> (raw)
In-Reply-To: <SG2PR06MB336567E43537C7F4947E342F8BEEA@SG2PR06MB3365.apcprd06.prod.outlook.com>
On Thu, Sep 07, 2023 at 07:17:55AM +0000, Billy Tsai wrote:
> On Wed, Aug 30, 2023 at 08:32:00PM +0800, Billy Tsai wrote:
> >> From: Naresh Solanki <naresh.solanki@9elements.com>
> >>
> >> Add common fan properties bindings to a schema.
> >>
> >> Bindings for fan controllers can reference the common schema for the
> >> fan
> >> +properties:
> >> + max-rpm:
> >> + description:
> >> + Max RPM supported by fan.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
>
> > Physics will limit this to something much less than 2^32. Add some
> > constraints. 10000?
>
>
> >> +
> >> + min-rpm:
> >> + description:
> >> + Min RPM supported by fan.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
>
> > ditto
>
> >> +
> >> + pulses-per-revolution:
> >> + description:
> >> + The number of pulse from fan sensor per revolution.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
>
> >Needs constraints. I assume this is never more than 4 (or 2 even)?
>
> Do you think we should add the contraint in the common binding?
> In my option, the limit of the max/min rpm should be declared by
> the binding if necessary, because the usage of each fan monitor is
> based on the connection of the tach pin.
Yes, I think we should have default limits.
Unless we go as far as a schema for every specific fan model, then there
is actually no way we can have specific limits unless the fan
controllers have some limits.
The most I see in tree for pulses-per-revolution is 2. There's no value
in more. So set the max to 4 and then if anyone needs more they can bump
the value.
Or maybe there's some electrical/mechanical design reason fans are 1 or
2 pulses and we'll never see anything else? This document[1] seems to
indicate that is indeed the case. (First hit googling "fan tach signal
pulses")
>
>
> >> + div:
>
> > Too generic of a name.
>
> >> + description:
> >> + Fan clock divisor
>
> > But what is a fan clock?
>
> This is the divisor for the tachometer sampling clock, which determines the sensitivity of the tach pin.
> So, if the name of the property changes to 'tach-div,' is it acceptable to you?
That sounds like a property of the controller, not the fan, so it
belongs in the controller binding. Is this really a common thing?
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> +
> >> + target-rpm:
> >> + description:
> >> + Target RPM the fan should be configured during driver probe.
>
> > What driver? By the time the OS driver runs, a bunch of other boot
> > software has already run on modern systems. So this value would likely
> > be used much earlier. The point is that when exactly is outside the
> > scope of DT. This is "what RPM do I use in case of no other information
> > (e.g. temperature)".
>
> So, the description should be changed to 'The default desired fan speed in RPM,'
> and we shouldn't mention the timing of the property's operation in the DT, is that correct?
Correct.
>
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> +
> >> + mode:
>
> > Too generic.
>
> >> + description:
> >> + Select the operational mode of the fan.
>
> > What are modes? Spin and don't spin?
>
> The mode is used to indicate the driving mode of the fan (DC, PWM and so on).
> So, if the name of the property changes to 'fan-driving-mode,' is it acceptable to you?
I tend to think that should be implied from the parent node and/or other
properties. PWM if "pwms" property is present. DC if the supply is
variable. We could also use compatible strings in the fan nodes if
there's a need.
That reminds me, both of these modes probably need a table of
voltage/duty-cycle to RPMs. I imagine it's not always a linear response.
Naresh also privately sent me (don't do that) an updated common binding
which we discussed the need for this. I expect him to comment further
with details.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
> >> +
> >> + pwms:
> >> + description:
> >> + PWM provider.
>
> > maxItems: 1
>
> > I don't think there are fans with more than 1 PWM input?
>
> Ok, I will add the constraint for the pwm input.
>
> >> +
> >> + tach-ch:
> >> + description:
> >> + The tach channel used for the fan.
> >> + $ref: /schemas/types.yaml#/definitions/uint32
>
> > The existing ASpeed version of this property allows more than 1 entry. I
> > don't understand how a fan would have 2 tach signals, but if so, the
> > generic property should allow for that.
>
> Ok, I will modify it to the uint32-array
Perhaps uint8-array to align with existing versions of the property.
>
> > Perhaps 'reg' should be defined in here with some text saying 'reg'
> > corresponds to the fan controller specific id which may be the PWM+TACH
> > channel, PWM channel (deprecated), or TACH channel. I think there are
> > examples of all 3 of these cases.
>
> I don't think it's necessary for the 'reg' because the case you mentioned is
> already covered by the property 'tach-ch' and the 'pwms'.
Yes, but when we have N child nodes of the same thing, we usually have
"reg" and its value corresponds to how the parent identifies each child.
We already have a mixture using PWM or tach channel. Yes, this can all
just be in the fan controllers binding, but putting it here would just
document the options.
Rob
[1] http://www.comairrotron.com/methods-monitoring-fan-performance
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-09-07 19:44 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-30 12:31 [PATCH v8 0/3] Support pwm/tach driver for aspeed ast26xx Billy Tsai
2023-08-30 12:32 ` [PATCH v8 1/3] dt-bindings: hwmon: fan: Add fan binding to schema Billy Tsai
2023-09-05 17:00 ` Rob Herring
2023-09-07 7:17 ` Billy Tsai
2023-09-07 19:43 ` Rob Herring [this message]
2023-09-08 8:49 ` Billy Tsai
2023-09-12 15:11 ` Rob Herring
2023-08-30 12:32 ` [PATCH v8 2/3] dt-bindings: hwmon: Support Aspeed g6 PWM TACH Control Billy Tsai
2023-09-05 17:07 ` Rob Herring
2023-09-07 7:24 ` Billy Tsai
2023-08-30 12:32 ` [PATCH v8 3/3] hwmon: (aspeed-g6-pwm-tacho): Support for ASPEED g6 PWM/Fan tach Billy Tsai
2023-09-01 4:54 ` Potin Lai
2023-09-01 5:10 ` Billy Tsai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230907194351.GA2033402-robh@kernel.org \
--to=robh@kernel.org \
--cc=BMC-SW@aspeedtech.com \
--cc=andrew@aj.id.au \
--cc=billy_tsai@aspeedtech.com \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=joel@jms.id.au \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-aspeed@lists.ozlabs.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=naresh.solanki@9elements.com \
--cc=p.zabel@pengutronix.de \
--cc=patrick@stwcx.xyz \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).