Re: [PATCH V2 3/9] dt-bindings: Tegra186 tachometer device tree bindings

From: Guenter Roeck
Date: Tue Apr 10 2018 - 09:43:55 EST


On 04/10/2018 06:30 AM, Rob Herring wrote:
On Mon, Apr 9, 2018 at 9:37 AM, Mikko Perttunen <cyndis@xxxxxxxx> wrote:


On 04/09/2018 04:21 PM, Rob Herring wrote:

On Mon, Apr 9, 2018 at 12:38 AM, Mikko Perttunen <cyndis@xxxxxxxx> wrote:

Rob,


Please don't top post to lists.

this binding is for a specific IP block (for measuring/aggregating input
pulses) on the Tegra186 SoC, so I don't think it fits into any generic
binding.


What is it hooked up to to measure? You only mention "fan" five times
in the doc.


In practice, fans.


You have #pwm-cells too, so this block has PWM output as well? If not,
then where's the PWM for the fan control because there is no point in
having fan tach without some control mechanism.


It doesn't provide a PWM output. The (Linux) PWM framework provides
functionality in both directions - control and capture. But if the device
tree #pwm-cells/pwms properties are only for control, we may need to
introduce a new #capture-pwm-cells/capture-pwms or similar.

Yes, perhaps. But there is no point in having
#capture-pwm-cells/capture-pwms if you aren't describing the
connection between the fan and the fan controller.

The idea is that the generic fan node can then specify two pwms, one for
control and one for capture, to enable e.g. closed-loop control (I'm not
personally familiar with the usecase for this but I could imagine something
like that). The control PWM can be something completely different, maybe not
a PWM in the first place (e.g. some fixed voltage).

Yes. As you can have different types of fans (3-wire, 4-wire, etc.)
they would have different compatibles and differing properties
associated with them.

There's only so many ways to control fans and types of fans, so yes,
the interface of control and feedback lines between a fan and its
controller should absolutely be generic.


I'm not quite getting what you mean by this. Clearly we need a custom
compatibility string for the tachometer as it's a different hardware block
with different programming than others.

Yes, of course. It's the interface between fan controllers and fans
that I'm concerned about.

Or are you complaining about the
nvidia,pulse-per-rev/capture-window-len properties?

Well, those sound like properties of a fan (at least the first one),
so they belong in a fan node.

The aspeed fan controller is probably the closest thing we have to a
fan binding. Look at that if you haven't already.


FWIW, this is a fan speed (tachometer) counter which is modeled as pwm input.
This, in my opinion, and as stated before, is conceptually wrong. The pwm
subsystem should not (need to) know anything about fans, much less about
specifics such as the number of pulses per revolution.

Guenter