| Message ID | 1521607244-29734-4-git-send-email-rrajk@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote: > Supply Device tree binding documentation for the NVIDIA > Tegra186 SoC's Tachometer Controller > > Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com> > --- > > V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer" > Renamed dt property values of clock-names and reset-names to "tachometer" > from "tach" Read my prior comments on v1. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 27, 2018 at 09:52:49AM -0500, Rob Herring wrote: > On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote: > > Supply Device tree binding documentation for the NVIDIA > > Tegra186 SoC's Tachometer Controller > > > > Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com> > > --- > > > > V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer" > > Renamed dt property values of clock-names and reset-names to "tachometer" > > from "tach" > > Read my prior comments on v1. Also, I'm trying to make sense of who you Cc'ed on this. There's a ton of folks I know that I'm pretty sure don't care about this series. Start with get_maintainers.pl and add people you know need to see this series. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob, 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. Thanks, Mikko On 03/27/2018 05:52 PM, Rob Herring wrote: > On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote: >> Supply Device tree binding documentation for the NVIDIA >> Tegra186 SoC's Tachometer Controller >> >> Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com> >> --- >> >> V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer" >> Renamed dt property values of clock-names and reset-names to "tachometer" >> from "tach" > > Read my prior comments on v1. > > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 9, 2018 at 12:38 AM, Mikko Perttunen <cyndis@kapsi.fi> 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. 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. 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. Rob > > Thanks, > Mikko > > > On 03/27/2018 05:52 PM, Rob Herring wrote: >> >> On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote: >>> >>> Supply Device tree binding documentation for the NVIDIA >>> Tegra186 SoC's Tachometer Controller >>> >>> Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com> >>> --- >>> >>> V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer" >>> Renamed dt property values of clock-names and reset-names to >>> "tachometer" >>> from "tach" >> >> >> Read my prior comments on v1. >> >> Rob >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/09/2018 04:21 PM, Rob Herring wrote: > On Mon, Apr 9, 2018 at 12:38 AM, Mikko Perttunen <cyndis@kapsi.fi> 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. 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). > > 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. Or are you complaining about the nvidia,pulse-per-rev/capture-window-len properties? Thanks, Mikko > > Rob > >> >> Thanks, >> Mikko >> >> >> On 03/27/2018 05:52 PM, Rob Herring wrote: >>> >>> On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote: >>>> >>>> Supply Device tree binding documentation for the NVIDIA >>>> Tegra186 SoC's Tachometer Controller >>>> >>>> Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com> >>>> --- >>>> >>>> V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer" >>>> Renamed dt property values of clock-names and reset-names to >>>> "tachometer" >>>> from "tach" >>> >>> >>> Read my prior comments on v1. >>> >>> Rob >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 9, 2018 at 9:37 AM, Mikko Perttunen <cyndis@kapsi.fi> wrote: > > > On 04/09/2018 04:21 PM, Rob Herring wrote: >> >> On Mon, Apr 9, 2018 at 12:38 AM, Mikko Perttunen <cyndis@kapsi.fi> 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. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/2018 06:30 AM, Rob Herring wrote: > On Mon, Apr 9, 2018 at 9:37 AM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >> >> >> On 04/09/2018 04:21 PM, Rob Herring wrote: >>> >>> On Mon, Apr 9, 2018 at 12:38 AM, Mikko Perttunen <cyndis@kapsi.fi> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pwm/pwm-tegra-tachometer.txt b/Documentation/devicetree/bindings/pwm/pwm-tegra-tachometer.txt new file mode 100644 index 0000000..4a7ead4 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-tegra-tachometer.txt @@ -0,0 +1,31 @@ +Bindings for a PWM based Tachometer driver + +Required properties: +- compatible: Must be "nvidia,tegra186-pwm-tachometer" +- reg: physical base addresses of the controller and length of + memory mapped region. +- #pwm-cells: should be 2. See pwm.txt in this directory for a + description of the cells format. +- clocks: phandle list of tachometer clocks +- clock-names: should be "tachometer". See clock-bindings.txt in documentations +- resets: phandle to the reset controller for the Tachometer IP +- reset-names: should be "tachometer". See reset.txt in documentations +- nvidia,pulse-per-rev: Integer, pulses per revolution of a Fan. This value + obtained from Fan specification document. +- nvidia,capture-window-len: Integer, window of the Fan Tach monitor, it indicates + that how many period of the input fan tach signal will the FAN TACH logic + monitor. Valid values are 1, 2, 4 and 8 only. + +Example: + tegra_tachometer: tachometer@39c0000 { + compatible = "nvidia,tegra186-pwm-tachometer"; + reg = <0x0 0x039c0000 0x0 0x10>; + #pwm-cells = <2>; + clocks = <&tegra_car TEGRA186_CLK_TACH>; + clock-names = "tachometer"; + resets = <&tegra_car TEGRA186_RESET_TACH>; + reset-names = "tachometer"; + nvidia,pulse-per-rev = <2>; + nvidia,capture-window-len = <2>; + status = "disabled"; + };
Supply Device tree binding documentation for the NVIDIA Tegra186 SoC's Tachometer Controller Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com> --- V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer" Renamed dt property values of clock-names and reset-names to "tachometer" from "tach" .../bindings/pwm/pwm-tegra-tachometer.txt | 31 ++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-tegra-tachometer.txt