| Message ID | 20191205002503.13088-5-masneyb@onstation.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | qcom: add clk-vibrator driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
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 >
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 > >
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
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
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.
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
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.
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 --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>; + };
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