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

[Guenter Roeck]

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