From: Guenter Roeck <linux@roeck-us.net>
To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"robh@kernel.org" <robh@kernel.org>,
"krzk+dt@kernel.org" <krzk+dt@kernel.org>,
"conor+dt@kernel.org" <conor+dt@kernel.org>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle
Date: Sun, 12 May 2024 09:58:15 -0700 [thread overview]
Message-ID: <df40a387-37db-4a4d-b43f-ae22905789b5@roeck-us.net> (raw)
In-Reply-To: <35361786-ef5f-4d81-83e8-e347f47c83ed@alliedtelesis.co.nz>
On 5/10/24 08:51, Chris Packham wrote:
>
> On 10/05/24 15:36, Guenter Roeck wrote:
>> Chris,
>>
>> On Thu, May 09, 2024 at 06:19:12PM +0000, Chris Packham wrote:
>>> Hi Krzysztof,
>>>
>>> On 9/05/24 19:06, Krzysztof Kozlowski wrote:
>>>> On 08/05/2024 23:55, Chris Packham wrote:
>>>>> Add documentation for the pwm-initial-duty-cycle and
>>>>> pwm-initial-frequency properties. These allow the starting state of the
>>>>> PWM outputs to be set to cater for hardware designs where undesirable
>>>>> amounts of noise is created by the default hardware state.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>> Changes in v2:
>>>>> - Document 0 as a valid value (leaves hardware as-is)
>>>>>
>>>>> .../devicetree/bindings/hwmon/adt7475.yaml | 27 ++++++++++++++++++-
>>>>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>> index 051c976ab711..97deda082b4a 100644
>>>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>>> @@ -51,6 +51,30 @@ properties:
>>>>> enum: [0, 1]
>>>>> default: 1
>>>>>
>>>>> + adi,pwm-initial-duty-cycle:
>>>>> + description: |
>>>>> + Configures the initial duty cycle for the PWM outputs. The hardware
>>>>> + default is 100% but this may cause unwanted fan noise at startup. Set
>>>>> + this to a value from 0 (0% duty cycle) to 255 (100% duty cycle).
>>>>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>>>>> + minItems: 3
>>>>> + maxItems: 3
>>>>> + items:
>>>>> + minimum: 0
>>>>> + maximum: 255
>>>>> + default: 255
>>>>> +
>>>>> + adi,pwm-initial-frequency:
>>>> Frequency usually has some units, so use appropriate unit suffix and
>>>> drop $ref. Maybe that's just target-rpm property?
>>>>
>>>> But isn't this duplicating previous property? This is fan controller,
>>>> not PWM provider (in any case you miss proper $refs to pwm.yaml or
>>>> fan-common.yaml), so the only thing you initially want to configure is
>>>> the fan rotation, not specific PWM waveform. If you you want to
>>>> configure specific PWM waveform, then it's a PWM provider... but it is
>>>> not... Confused.
>>> There's two things going on here. There's a PWM duty cycle which is
>>> configurable from 0% to 100%. It might be nice if this was expressed as
>>> a percentage instead of 0-255 but I went with the latter because that's
>>> how the sysfs ABI for the duty cycle works.
>>>
>>> The frequency (which I'll call adi,pwm-initial-frequency-hz in v3)
>>> affects how that duty cycle is presented to the fans. So you could still
>>> have a duty cycle of 50% at any frequency. What frequency is best
>>> depends on the kind of fans being used. In my particular case the lower
>>> frequencies end up with the fans oscillating annoyingly so I use the
>>> highest setting.
>>>
>> My udnerstanding is that we are supposed to use standard pwm provider
>> properties. The property description is provider specicic, so I think
>> we can pretty much just make it up.
>>
>> Essentially you'd first define a pwm provider which defines all the
>> pwm parameters needed, such as pwm freqency, default duty cycle,
>> and flags such as PWM_POLARITY_INVERTED. You'd then add something like
>>
>> pwms = <&pwm index frequency duty_cycle ... flags>;
>>
>> to the node for each fan, and be done.
>>
>> That doesn't mean that we would actually have to register the chip
>> as pwm provider with the pwm subsystem; all we would have to do is to
>> interpret the property values.
>
> We've already got the pwm-active-state as a separate property so that
> might be tricky to deal with, I guess it could be deprecated in favour
> of something else. Looking at pwm.yaml and fan-common.yaml I can't quite
> see how that'd help here. Were you thinking maybe something like
>
> pwm: hwmon@2e {
> compatible = "adi,adt7476";
> reg = <0x2e>;
> #pwm-cells = <4>;
> fan-0 {
> pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
> pwm-names = "PWM1";
> tach-ch = <0>;
> };
> fan-1 {
> // controlled by pwm 0
> tach-ch = <1>
> };
> fan-0 {
> pwms = <&pwm 2 255 22500 PWM_POLARITY_INVERTED>;
> pwm-names = "PWM3";
> tach-ch <2>;
> };
> fan-1 {
> // controlled by pwm 2
> tach-ch = <3>
I think that would have to be
...
fan-0 {
pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
tach-ch = <1 2>;
};
fan-1 {
tach-ch = <3>
};
...
Context: pwm-names is optional and does not add value here unless I am missing
something. Also, if I understand the bindings correctly, all tachometer channels
controlled by a single pwm are supposed to be listed in a single node. With the
above, you'd then have fan1, fan2, and fan3 plus pwm1 and pwm3 (pwm2 would be
disabled/unused).
Code-wise, I think you'd then call
struct of_phandle_args args;
...
err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", 0, &args)
with np pointing to the fan node. This should return the parameters in 'args'.
However, unless you have a use case, I'd suggest not to implement support for
"multiple fans controlled by single pwm" since that would require extra
code and you would not actually be able to test it. A mandatory 1:1 mapping
is fine with me. Support for 1:n mapping can be implemented if / when there
is a use case. The same is true for registering the driver with the pwm
subsystem - that would only be necessary if anyone ever uses one of the
pwm channels for non-fan use.
That makes me wonder if we actually need tach-ch in the first place or if
something like
fan-0 {
pwms = <&pwm 0 255 22500 PWM_POLARITY_INVERTED>;
};
fan-1 {
pwms = <&pwm 1 255 22500 0>;
};
...
would do for this chip.
Thanks,
Guenter
next prev parent reply other threads:[~2024-05-12 16:58 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 21:55 [PATCH v2 0/2] hwmon: (adt7475) duty cycle configuration Chris Packham
2024-05-08 21:55 ` [PATCH v2 1/2] dt-bindings: hwmon: Document adt7475 PWM initial duty cycle Chris Packham
2024-05-09 7:06 ` Krzysztof Kozlowski
2024-05-09 13:25 ` Guenter Roeck
2024-05-09 18:19 ` Chris Packham
2024-05-10 3:36 ` Guenter Roeck
2024-05-10 15:51 ` Chris Packham
2024-05-12 16:58 ` Guenter Roeck [this message]
2024-05-17 1:09 ` Chris Packham
2024-05-17 17:00 ` Conor Dooley
2024-05-17 17:02 ` Conor Dooley
2024-05-17 17:26 ` Guenter Roeck
2024-05-17 17:39 ` Conor Dooley
2024-05-08 21:55 ` [PATCH v2 2/2] hwmon: (adt7475) Add support for configuring initial PWM " Chris Packham
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=df40a387-37db-4a4d-b43f-ae22905789b5@roeck-us.net \
--to=linux@roeck-us.net \
--cc=Chris.Packham@alliedtelesis.co.nz \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=krzk+dt@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).