From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E3300EC8758 for ; Thu, 7 Sep 2023 19:44:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=kt19/m08KBLXSMPM2j21RI2tN2xqHYsdgTVSeaekYcA=; b=mjMb04XR1zkKAK EWWXXF/4QzALtVwp3Swo0wmTarJED2NcZH0MukkxTJ2zU+sWo6CGJV3hxafGGksK8bD7aY6WK5v0S nWG8H5DgpwRUUL+Q2+9Tks/zL/kEapX9xN5Vox+3x/WjFBZR9wGvaLhs0f045KKkmM577568Z9ITO VnDy92xeIEiHR+EGNpcxJqUg66cjlOqr7YpolFmAKyKrpnIE0NvvzREuly627FPTGJy2cPTSAtbqI EM8kvIY+uOznS4k0SQs0WVK+fwCpJYUCvXtCzPyDN8KKZOY60w5pTGq82lVttb09wxtKfU2Tcj0Sl B2sfTyj++uwPmx74SJGw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qeKv3-00Ccmh-2y; Thu, 07 Sep 2023 19:44:01 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qeKv0-00Ccls-2l for linux-arm-kernel@lists.infradead.org; Thu, 07 Sep 2023 19:44:00 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B447361365; Thu, 7 Sep 2023 19:43:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1A5F7C433C7; Thu, 7 Sep 2023 19:43:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1694115834; bh=XNJ6eaiy6oOw6b1KlAbPBrj8EtJk96f0FRUMDyQiiPw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LBkzCyN6TBJDyL3szlqcxnUz72Sedly9MEc8nJDUoDQRryvSruVBTgamW/W2No+3a PP+prYSndor/ivzTvcAhKwJjvhGbQbfB+i/ROmxf5MjZZ6oZDEt9kg6NKR3k+zd89f aPkYv6BJfJbgqJMlDCKSt/MSP2b83tJBOSL4vwEpHuVaVq8YO9o5oR991tFG+rf8Q5 YUD/zqAq8N9lkT0GyBdHvuAbeDH6IeBZA1GEhg5JRk9r+Xagqzv0bLjaMKnptFVG+T qsx5821KeoE6/HPF+iR0oRIIuUj1FsMP2rUCKGh2xlT6BVfw5J8Qz/oHF7V5Q9Gm9e E1HoIz9ceZQFQ== Received: (nullmailer pid 2204967 invoked by uid 1000); Thu, 07 Sep 2023 19:43:51 -0000 Date: Thu, 7 Sep 2023 14:43:51 -0500 From: Rob Herring To: Billy Tsai Cc: "jdelvare@suse.com" , "linux@roeck-us.net" , "krzysztof.kozlowski+dt@linaro.org" , "joel@jms.id.au" , "andrew@aj.id.au" , "corbet@lwn.net" , "thierry.reding@gmail.com" , "u.kleine-koenig@pengutronix.de" , "p.zabel@pengutronix.de" , "naresh.solanki@9elements.com" , "linux-hwmon@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-aspeed@lists.ozlabs.org" , "linux-kernel@vger.kernel.org" , "linux-doc@vger.kernel.org" , "linux-pwm@vger.kernel.org" , BMC-SW , "patrick@stwcx.xyz" < patrick@stwcx.xyz> Subject: Re: [PATCH v8 1/3] dt-bindings: hwmon: fan: Add fan binding to schema Message-ID: <20230907194351.GA2033402-robh@kernel.org> References: <20230830123202.3408318-1-billy_tsai@aspeedtech.com> <20230830123202.3408318-2-billy_tsai@aspeedtech.com> <20230905170010.GA3505375-robh@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230907_124359_006178_EA21C75F X-CRM114-Status: GOOD ( 46.36 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > >> > >> 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