diff mbox series

[4/7] dt-bindings: Input: introduce new clock vibrator bindings

Message ID 20191205002503.13088-5-masneyb@onstation.org (mailing list archive)
State New, archived
Headers show
Series qcom: add clk-vibrator driver | expand

Commit Message

Brian Masney Dec. 5, 2019, 12:25 a.m. UTC
Add support for clock-based vibrator devices where the speed can be
controlled by changing the duty cycle.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 .../bindings/input/clk-vibrator.yaml          | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/clk-vibrator.yaml

Comments

Rob Herring Dec. 5, 2019, 1:56 p.m. UTC | #1
On Wed, Dec 4, 2019 at 6:25 PM Brian Masney <masneyb@onstation.org> wrote:
>
> Add support for clock-based vibrator devices where the speed can be
> controlled by changing the duty cycle.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  .../bindings/input/clk-vibrator.yaml          | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/clk-vibrator.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/clk-vibrator.yaml b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> new file mode 100644
> index 000000000000..2103a5694fad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/input/clk-vibrator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clock vibrator
> +
> +maintainers:
> +  - Brian Masney <masneyb@onstation.org>
> +
> +description: |
> +  Support for clock-based vibrator devices where the speed can be controlled
> +  by changing the duty cycle.
> +
> +properties:
> +  compatible:
> +    const: clk-vibrator
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description: output clock that controls the speed
> +    items:
> +      - const: core

No point in making up a name when there's only one clock, so drop.

> +
> +  clock-frequency: true

Given the frequency is variable, what does this mean in this case?

> +  enable-gpios:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Regulator that provides power
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - clock-frequency

Add:

additionalProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    vibrator {
> +        compatible = "clk-vibrator";
> +
> +        vcc-supply = <&pm8941_l19>;
> +
> +        clocks = <&mmcc CAMSS_GP1_CLK>;
> +        clock-names = "core";
> +        clock-frequency = <24000>;
> +
> +        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> +
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&vibrator_pin>;
> +    };
> --
> 2.21.0
>
Brian Masney Dec. 9, 2019, 12:54 a.m. UTC | #2
On Thu, Dec 05, 2019 at 07:56:10AM -0600, Rob Herring wrote:
> On Wed, Dec 4, 2019 at 6:25 PM Brian Masney <masneyb@onstation.org> wrote:
> >
> > Add support for clock-based vibrator devices where the speed can be
> > controlled by changing the duty cycle.
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > ---
> >  .../bindings/input/clk-vibrator.yaml          | 60 +++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/clk-vibrator.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/clk-vibrator.yaml b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > new file mode 100644
> > index 000000000000..2103a5694fad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/bindings/input/clk-vibrator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Clock vibrator
> > +
> > +maintainers:
> > +  - Brian Masney <masneyb@onstation.org>
> > +
> > +description: |
> > +  Support for clock-based vibrator devices where the speed can be controlled
> > +  by changing the duty cycle.
> > +
> > +properties:
> > +  compatible:
> > +    const: clk-vibrator
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description: output clock that controls the speed
> > +    items:
> > +      - const: core
> 
> No point in making up a name when there's only one clock, so drop.

OK, will do.

> 
> > +
> > +  clock-frequency: true
> 
> Given the frequency is variable, what does this mean in this case?

The clock frequency is fixed. The duty cycle is what's variable.

Brian



> 
> > +  enable-gpios:
> > +    maxItems: 1
> > +
> > +  vcc-supply:
> > +    description: Regulator that provides power
> > +
> > +required:
> > +  - compatible
> > +  - clocks
> > +  - clock-names
> > +  - clock-frequency
> 
> Add:
> 
> additionalProperties: false
> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    vibrator {
> > +        compatible = "clk-vibrator";
> > +
> > +        vcc-supply = <&pm8941_l19>;
> > +
> > +        clocks = <&mmcc CAMSS_GP1_CLK>;
> > +        clock-names = "core";
> > +        clock-frequency = <24000>;
> > +
> > +        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> > +
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&vibrator_pin>;
> > +    };
> > --
> > 2.21.0
> >
Rob Herring Dec. 9, 2019, 4:16 p.m. UTC | #3
On Sun, Dec 8, 2019 at 6:54 PM Brian Masney <masneyb@onstation.org> wrote:
>
> On Thu, Dec 05, 2019 at 07:56:10AM -0600, Rob Herring wrote:
> > On Wed, Dec 4, 2019 at 6:25 PM Brian Masney <masneyb@onstation.org> wrote:
> > >
> > > Add support for clock-based vibrator devices where the speed can be
> > > controlled by changing the duty cycle.
> > >
> > > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > > ---
> > >  .../bindings/input/clk-vibrator.yaml          | 60 +++++++++++++++++++
> > >  1 file changed, 60 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/clk-vibrator.yaml b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > > new file mode 100644
> > > index 000000000000..2103a5694fad
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > > @@ -0,0 +1,60 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/bindings/input/clk-vibrator.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Clock vibrator
> > > +
> > > +maintainers:
> > > +  - Brian Masney <masneyb@onstation.org>
> > > +
> > > +description: |
> > > +  Support for clock-based vibrator devices where the speed can be controlled
> > > +  by changing the duty cycle.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: clk-vibrator
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    description: output clock that controls the speed
> > > +    items:
> > > +      - const: core
> >
> > No point in making up a name when there's only one clock, so drop.
>
> OK, will do.
>
> >
> > > +
> > > +  clock-frequency: true
> >
> > Given the frequency is variable, what does this mean in this case?
>
> The clock frequency is fixed. The duty cycle is what's variable.

That sounds like a PWM then...

Rob
Brian Masney Dec. 9, 2019, 4:55 p.m. UTC | #4
On Mon, Dec 09, 2019 at 10:16:26AM -0600, Rob Herring wrote:
> On Sun, Dec 8, 2019 at 6:54 PM Brian Masney <masneyb@onstation.org> wrote:
> >
> > On Thu, Dec 05, 2019 at 07:56:10AM -0600, Rob Herring wrote:
> > > On Wed, Dec 4, 2019 at 6:25 PM Brian Masney <masneyb@onstation.org> wrote:
> > > >
> > > > Add support for clock-based vibrator devices where the speed can be
> > > > controlled by changing the duty cycle.
> > > >
> > > > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > > > ---
> > > >  .../bindings/input/clk-vibrator.yaml          | 60 +++++++++++++++++++
> > > >  1 file changed, 60 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/clk-vibrator.yaml b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > > > new file mode 100644
> > > > index 000000000000..2103a5694fad
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> > > > @@ -0,0 +1,60 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/bindings/input/clk-vibrator.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Clock vibrator
> > > > +
> > > > +maintainers:
> > > > +  - Brian Masney <masneyb@onstation.org>
> > > > +
> > > > +description: |
> > > > +  Support for clock-based vibrator devices where the speed can be controlled
> > > > +  by changing the duty cycle.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: clk-vibrator
> > > > +
> > > > +  clocks:
> > > > +    maxItems: 1
> > > > +
> > > > +  clock-names:
> > > > +    description: output clock that controls the speed
> > > > +    items:
> > > > +      - const: core
> > >
> > > No point in making up a name when there's only one clock, so drop.
> >
> > OK, will do.
> >
> > >
> > > > +
> > > > +  clock-frequency: true
> > >
> > > Given the frequency is variable, what does this mean in this case?
> >
> > The clock frequency is fixed. The duty cycle is what's variable.
> 
> That sounds like a PWM then...

Yes... See this message from Stephen with some more background
information about why this is in the clk subsystem:

https://lore.kernel.org/lkml/20190627234929.B78E520815@mail.kernel.org/

Brian
Stephen Boyd Jan. 5, 2020, 8:35 a.m. UTC | #5
Quoting Brian Masney (2019-12-04 16:25:00)
> diff --git a/Documentation/devicetree/bindings/input/clk-vibrator.yaml b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> new file mode 100644
> index 000000000000..2103a5694fad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/input/clk-vibrator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Clock vibrator
> +
> +maintainers:
> +  - Brian Masney <masneyb@onstation.org>
> +
> +description: |
> +  Support for clock-based vibrator devices where the speed can be controlled
> +  by changing the duty cycle.
> +
> +properties:
> +  compatible:
> +    const: clk-vibrator
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    description: output clock that controls the speed
> +    items:
> +      - const: core
> +
> +  clock-frequency: true

Can you use assigned-clock-rates for this instead? Then the driver can
call clk_get_rate() if it wants to know the rate that was actually set
on the clk.

> +
> +  enable-gpios:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Regulator that provides power
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - clock-frequency
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    vibrator {
> +        compatible = "clk-vibrator";
> +
> +        vcc-supply = <&pm8941_l19>;
> +
> +        clocks = <&mmcc CAMSS_GP1_CLK>;
> +        clock-names = "core";
> +        clock-frequency = <24000>;
> +
> +        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> +
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&vibrator_pin>;

I'm still trying to wrap my head around this. I think we can have a pwm
provider in a clk controller node (so imagine &mmcc has #pwm-cells) and
then this 'clk-vibrator' binding wouldn't exist? Instead we would have
some sort of binding for a device that expects a pwm and whatever else
is required, like the enable gpio and power supply. Is there an actual
hardware block that is this way? Does it have a real product id and is
made by some company? Right now this looks a little too generic to not
just be a catch-all for something that buzzes.
Brian Masney Jan. 7, 2020, 12:03 p.m. UTC | #6
On Sun, Jan 05, 2020 at 12:35:33AM -0800, Stephen Boyd wrote:
> Quoting Brian Masney (2019-12-04 16:25:00)
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    vibrator {
> > +        compatible = "clk-vibrator";
> > +
> > +        vcc-supply = <&pm8941_l19>;
> > +
> > +        clocks = <&mmcc CAMSS_GP1_CLK>;
> > +        clock-names = "core";
> > +        clock-frequency = <24000>;
> > +
> > +        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> > +
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&vibrator_pin>;
> 
> I'm still trying to wrap my head around this. I think we can have a pwm
> provider in a clk controller node (so imagine &mmcc has #pwm-cells) and
> then this 'clk-vibrator' binding wouldn't exist? Instead we would have
> some sort of binding for a device that expects a pwm and whatever else
> is required, like the enable gpio and power supply. Is there an actual
> hardware block that is this way? Does it have a real product id and is
> made by some company? Right now this looks a little too generic to not
> just be a catch-all for something that buzzes.

So have some of the Qualcomm clocks like this one register with both the
clk and the pwm frameworks? I feel that approach would better represent
the hardware in device tree.

If we did that, then the pwm-vibra driver in the input subsystem could
be used.

Brian
Stephen Boyd Jan. 7, 2020, 5:52 p.m. UTC | #7
Quoting Brian Masney (2020-01-07 04:03:17)
> On Sun, Jan 05, 2020 at 12:35:33AM -0800, Stephen Boyd wrote:
> > Quoting Brian Masney (2019-12-04 16:25:00)
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +
> > > +    vibrator {
> > > +        compatible = "clk-vibrator";
> > > +
> > > +        vcc-supply = <&pm8941_l19>;
> > > +
> > > +        clocks = <&mmcc CAMSS_GP1_CLK>;
> > > +        clock-names = "core";
> > > +        clock-frequency = <24000>;
> > > +
> > > +        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> > > +
> > > +        pinctrl-names = "default";
> > > +        pinctrl-0 = <&vibrator_pin>;
> > 
> > I'm still trying to wrap my head around this. I think we can have a pwm
> > provider in a clk controller node (so imagine &mmcc has #pwm-cells) and
> > then this 'clk-vibrator' binding wouldn't exist? Instead we would have
> > some sort of binding for a device that expects a pwm and whatever else
> > is required, like the enable gpio and power supply. Is there an actual
> > hardware block that is this way? Does it have a real product id and is
> > made by some company? Right now this looks a little too generic to not
> > just be a catch-all for something that buzzes.
> 
> So have some of the Qualcomm clocks like this one register with both the
> clk and the pwm frameworks? I feel that approach would better represent
> the hardware in device tree.

That is one option. Or another option would be to have another node that
"adapts" a clk signal to a pwm provider. Similar to how we adapt a gpio
to make a clk gate or mux. Something like:

	gcc: clock-controller@f00d {
		reg = <0xf00d 0xd00d>;
		#clock-cells = <1>;
	};


	pwm {
		compatible = "pwm-clk";
		#pwm-cells = <0>;
		clocks = <&gcc 45>;
		assigned-clocks = <&gcc 45>;
		assigned-clock-rates = <1400000>;
	};

And then the pwm-clk driver would adjust the duty cycle to generate a
pwm.

> 
> If we did that, then the pwm-vibra driver in the input subsystem could
> be used.
Brian Masney Jan. 7, 2020, 11:18 p.m. UTC | #8
On Tue, Jan 07, 2020 at 09:52:21AM -0800, Stephen Boyd wrote:
> Quoting Brian Masney (2020-01-07 04:03:17)
> > On Sun, Jan 05, 2020 at 12:35:33AM -0800, Stephen Boyd wrote:
> > > Quoting Brian Masney (2019-12-04 16:25:00)
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
> > > > +    #include <dt-bindings/gpio/gpio.h>
> > > > +
> > > > +    vibrator {
> > > > +        compatible = "clk-vibrator";
> > > > +
> > > > +        vcc-supply = <&pm8941_l19>;
> > > > +
> > > > +        clocks = <&mmcc CAMSS_GP1_CLK>;
> > > > +        clock-names = "core";
> > > > +        clock-frequency = <24000>;
> > > > +
> > > > +        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> > > > +
> > > > +        pinctrl-names = "default";
> > > > +        pinctrl-0 = <&vibrator_pin>;
> > > 
> > > I'm still trying to wrap my head around this. I think we can have a pwm
> > > provider in a clk controller node (so imagine &mmcc has #pwm-cells) and
> > > then this 'clk-vibrator' binding wouldn't exist? Instead we would have
> > > some sort of binding for a device that expects a pwm and whatever else
> > > is required, like the enable gpio and power supply. Is there an actual
> > > hardware block that is this way? Does it have a real product id and is
> > > made by some company? Right now this looks a little too generic to not
> > > just be a catch-all for something that buzzes.
> > 
> > So have some of the Qualcomm clocks like this one register with both the
> > clk and the pwm frameworks? I feel that approach would better represent
> > the hardware in device tree.
> 
> That is one option. Or another option would be to have another node that
> "adapts" a clk signal to a pwm provider. Similar to how we adapt a gpio
> to make a clk gate or mux. Something like:
> 
> 	gcc: clock-controller@f00d {
> 		reg = <0xf00d 0xd00d>;
> 		#clock-cells = <1>;
> 	};
> 
> 
> 	pwm {
> 		compatible = "pwm-clk";
> 		#pwm-cells = <0>;
> 		clocks = <&gcc 45>;
> 		assigned-clocks = <&gcc 45>;
> 		assigned-clock-rates = <1400000>;
> 	};
> 
> And then the pwm-clk driver would adjust the duty cycle to generate a
> pwm.

OK, that makes sense.

I'll pick this up after someone from Qualcomm posts a patch that
implements the clock duty cycle. I'm willing to do that work if someone
explains the relationship between the m, n, and d values on these clocks.

Brian
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/clk-vibrator.yaml b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
new file mode 100644
index 000000000000..2103a5694fad
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/clk-vibrator.yaml
@@ -0,0 +1,60 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/input/clk-vibrator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Clock vibrator
+
+maintainers:
+  - Brian Masney <masneyb@onstation.org>
+
+description: |
+  Support for clock-based vibrator devices where the speed can be controlled
+  by changing the duty cycle.
+
+properties:
+  compatible:
+    const: clk-vibrator
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    description: output clock that controls the speed
+    items:
+      - const: core
+
+  clock-frequency: true
+
+  enable-gpios:
+    maxItems: 1
+
+  vcc-supply:
+    description: Regulator that provides power
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - clock-frequency
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,mmcc-msm8974.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    vibrator {
+        compatible = "clk-vibrator";
+
+        vcc-supply = <&pm8941_l19>;
+
+        clocks = <&mmcc CAMSS_GP1_CLK>;
+        clock-names = "core";
+        clock-frequency = <24000>;
+
+        enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
+
+        pinctrl-names = "default";
+        pinctrl-0 = <&vibrator_pin>;
+    };