A while ago Brian Masney sent some patches for a clk-vibrator which was
then succeeded by the idea of a clk-pwm driver that "converts" a clock
into a PWM and to use the existing pwm-vibra driver.
Since clk-pwm has landed last year we can finally add haptics support
upstream.
We just need to add support for an enable GPIO to the pwm-vibra driver
since that also needs to be high for the haptics to work on this device.
Signed-off-by: Luca Weiss <[email protected]>
---
Luca Weiss (4):
dt-bindings: input: pwm-vibrator: Add enable-gpio
Input: pwm-vibra - add newline to dev_err prints
Input: pwm-vibra - add support for enable GPIO
ARM: dts: qcom: msm8974-hammerhead: Add vibrator
.../devicetree/bindings/input/pwm-vibrator.yaml | 2 ++
.../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35 +++++++++++++++++++++
drivers/input/misc/pwm-vibra.c | 36 ++++++++++++++++------
3 files changed, 63 insertions(+), 10 deletions(-)
---
base-commit: dec7f67a13c3270f9a38eba227a4fc15993f01b3
change-id: 20230427-hammerhead-vibra-06bd1bf771a3
Best regards,
--
Luca Weiss <[email protected]>
The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
which we can use with the clk-pwm driver, then we can use that pwm with
pwm-vibrator to get haptics functionality.
This patch is based on Brian Masney's previous patch with clk-vibrator.
Signed-off-by: Luca Weiss <[email protected]>
---
.../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35 ++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
index ab35f2d644c0..fea8a6be9021 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -41,6 +41,25 @@ key-volume-down {
};
};
+ clk_pwm: pwm {
+ compatible = "clk-pwm";
+ clocks = <&mmcc CAMSS_GP1_CLK>;
+
+ pinctrl-0 = <&vibrator_pin>;
+ pinctrl-names = "default";
+
+ #pwm-cells = <2>;
+ };
+
+ vibrator {
+ compatible = "pwm-vibrator";
+ pwms = <&clk_pwm 0 100000>;
+ pwm-names = "enable";
+
+ vcc-supply = <&pm8941_l19>;
+ enable-gpios = <&tlmm 60 GPIO_ACTIVE_HIGH>;
+ };
+
vreg_wlan: wlan-regulator {
compatible = "regulator-fixed";
@@ -637,6 +656,22 @@ shutdown-pins {
function = "gpio";
};
};
+
+ vibrator_pin: vibrator-state {
+ core-pins {
+ pins = "gpio27";
+ function = "gp1_clk";
+ drive-strength = <6>;
+ bias-disable;
+ };
+
+ enable-pins {
+ pins = "gpio60";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ };
+ };
};
&usb {
--
2.40.0
Some pwm vibrators have a dedicated enable GPIO that needs to be set
high so that the vibrator works. Document that.
Signed-off-by: Luca Weiss <[email protected]>
---
Documentation/devicetree/bindings/input/pwm-vibrator.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
index d32716c604fe..6398534b43c3 100644
--- a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
+++ b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
@@ -32,6 +32,8 @@ properties:
minItems: 1
maxItems: 2
+ enable-gpios: true
+
vcc-supply: true
direction-duty-cycle-ns:
--
2.40.0
Some pwm vibrators have a dedicated enable GPIO that needs to be set
high so that the vibrator works. Add support for that optionally.
Signed-off-by: Luca Weiss <[email protected]>
---
drivers/input/misc/pwm-vibra.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
index c08971c97ad6..2ba035299db8 100644
--- a/drivers/input/misc/pwm-vibra.c
+++ b/drivers/input/misc/pwm-vibra.c
@@ -11,6 +11,7 @@
* Copyright (C) 2010, Lars-Peter Clausen <[email protected]>
*/
+#include <linux/gpio/consumer.h>
#include <linux/input.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -23,6 +24,7 @@
struct pwm_vibrator {
struct input_dev *input;
+ struct gpio_desc *enable_gpio;
struct pwm_device *pwm;
struct pwm_device *pwm_dir;
struct regulator *vcc;
@@ -48,6 +50,8 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
vibrator->vcc_on = true;
}
+ gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
+
pwm_get_state(vibrator->pwm, &state);
pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
state.enabled = true;
@@ -80,6 +84,8 @@ static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
pwm_disable(vibrator->pwm_dir);
pwm_disable(vibrator->pwm);
+ gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
+
if (vibrator->vcc_on) {
regulator_disable(vibrator->vcc);
vibrator->vcc_on = false;
@@ -142,6 +148,16 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
return err;
}
+ vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+ GPIOD_OUT_LOW);
+ err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
+ if (err) {
+ if (err != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
+ err);
+ return err;
+ }
+
vibrator->pwm = devm_pwm_get(&pdev->dev, "enable");
err = PTR_ERR_OR_ZERO(vibrator->pwm);
if (err) {
--
2.40.0
Make sure all printed messages end with a newline.
Signed-off-by: Luca Weiss <[email protected]>
---
drivers/input/misc/pwm-vibra.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
index d0e58a7cdfa3..c08971c97ad6 100644
--- a/drivers/input/misc/pwm-vibra.c
+++ b/drivers/input/misc/pwm-vibra.c
@@ -42,7 +42,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
if (!vibrator->vcc_on) {
err = regulator_enable(vibrator->vcc);
if (err) {
- dev_err(pdev, "failed to enable regulator: %d", err);
+ dev_err(pdev, "failed to enable regulator: %d\n", err);
return err;
}
vibrator->vcc_on = true;
@@ -54,7 +54,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
err = pwm_apply_state(vibrator->pwm, &state);
if (err) {
- dev_err(pdev, "failed to apply pwm state: %d", err);
+ dev_err(pdev, "failed to apply pwm state: %d\n", err);
return err;
}
@@ -65,7 +65,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
err = pwm_apply_state(vibrator->pwm_dir, &state);
if (err) {
- dev_err(pdev, "failed to apply dir-pwm state: %d", err);
+ dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
pwm_disable(vibrator->pwm);
return err;
}
@@ -137,7 +137,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
err = PTR_ERR_OR_ZERO(vibrator->vcc);
if (err) {
if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request regulator: %d",
+ dev_err(&pdev->dev, "Failed to request regulator: %d\n",
err);
return err;
}
@@ -146,7 +146,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
err = PTR_ERR_OR_ZERO(vibrator->pwm);
if (err) {
if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request main pwm: %d",
+ dev_err(&pdev->dev, "Failed to request main pwm: %d\n",
err);
return err;
}
@@ -158,7 +158,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
state.enabled = false;
err = pwm_apply_state(vibrator->pwm, &state);
if (err) {
- dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
+ dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
err);
return err;
}
@@ -172,7 +172,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
state.enabled = false;
err = pwm_apply_state(vibrator->pwm_dir, &state);
if (err) {
- dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
+ dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
err);
return err;
}
@@ -189,7 +189,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
break;
default:
- dev_err(&pdev->dev, "Failed to request direction pwm: %d", err);
+ dev_err(&pdev->dev, "Failed to request direction pwm: %d\n", err);
fallthrough;
case -EPROBE_DEFER:
@@ -207,13 +207,13 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
err = input_ff_create_memless(vibrator->input, NULL,
pwm_vibrator_play_effect);
if (err) {
- dev_err(&pdev->dev, "Couldn't create FF dev: %d", err);
+ dev_err(&pdev->dev, "Couldn't create FF dev: %d\n", err);
return err;
}
err = input_register_device(vibrator->input);
if (err) {
- dev_err(&pdev->dev, "Couldn't register input dev: %d", err);
+ dev_err(&pdev->dev, "Couldn't register input dev: %d\n", err);
return err;
}
--
2.40.0
On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Add support for that optionally.
>
> Signed-off-by: Luca Weiss <[email protected]>
Hi Luca,
Thank you for picking up this work!
> + vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> + GPIOD_OUT_LOW);
> + err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
> + if (err) {
> + if (err != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
> + err);
> + return err;
> + }
> +
Take a look at dev_err_probe() to remove the -EPROBE_DEFER check.
With that fixed:
Reviewed-by: Brian Masney <[email protected]>
On Thu, Apr 27, 2023 at 10:34:27PM +0200, Luca Weiss wrote:
> Make sure all printed messages end with a newline.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Brian Masney <[email protected]>
On Thu, Apr 27, 2023 at 10:34:26PM +0200, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Document that.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Brian Masney <[email protected]>
On Thu, Apr 27, 2023 at 10:34:29PM +0200, Luca Weiss wrote:
> The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
> which we can use with the clk-pwm driver, then we can use that pwm with
> pwm-vibrator to get haptics functionality.
>
> This patch is based on Brian Masney's previous patch with clk-vibrator.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Brian Masney <[email protected]>
On 27/04/2023 21:34, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Add support for that optionally.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> drivers/input/misc/pwm-vibra.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index c08971c97ad6..2ba035299db8 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -11,6 +11,7 @@
> * Copyright (C) 2010, Lars-Peter Clausen <[email protected]>
> */
>
> +#include <linux/gpio/consumer.h>
> #include <linux/input.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -23,6 +24,7 @@
>
> struct pwm_vibrator {
> struct input_dev *input;
> + struct gpio_desc *enable_gpio;
> struct pwm_device *pwm;
> struct pwm_device *pwm_dir;
> struct regulator *vcc;
> @@ -48,6 +50,8 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> vibrator->vcc_on = true;
> }
>
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
> +
> pwm_get_state(vibrator->pwm, &state);
> pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
> state.enabled = true;
> @@ -80,6 +84,8 @@ static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
> pwm_disable(vibrator->pwm_dir);
> pwm_disable(vibrator->pwm);
>
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
> +
> if (vibrator->vcc_on) {
> regulator_disable(vibrator->vcc);
> vibrator->vcc_on = false;
> @@ -142,6 +148,16 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> return err;
> }
>
> + vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> + GPIOD_OUT_LOW);
> + err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
> + if (err) {
> + if (err != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
> + err);
> + return err;
> + }
> +
> vibrator->pwm = devm_pwm_get(&pdev->dev, "enable");
> err = PTR_ERR_OR_ZERO(vibrator->pwm);
> if (err) {
>
On 27/04/2023 21:34, Luca Weiss wrote:
> The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
> which we can use with the clk-pwm driver, then we can use that pwm with
> pwm-vibrator to get haptics functionality.
>
> This patch is based on Brian Masney's previous patch with clk-vibrator.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> .../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35 ++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> index ab35f2d644c0..fea8a6be9021 100644
> --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> @@ -41,6 +41,25 @@ key-volume-down {
> };
> };
>
> + clk_pwm: pwm {
> + compatible = "clk-pwm";
> + clocks = <&mmcc CAMSS_GP1_CLK>;
> +
> + pinctrl-0 = <&vibrator_pin>;
> + pinctrl-names = "default";
> +
> + #pwm-cells = <2>;
> + };
> +
> + vibrator {
> + compatible = "pwm-vibrator";
> + pwms = <&clk_pwm 0 100000>;
> + pwm-names = "enable";
> +
> + vcc-supply = <&pm8941_l19>;
> + enable-gpios = <&tlmm 60 GPIO_ACTIVE_HIGH>;
> + };
> +
> vreg_wlan: wlan-regulator {
> compatible = "regulator-fixed";
>
> @@ -637,6 +656,22 @@ shutdown-pins {
> function = "gpio";
> };
> };
> +
> + vibrator_pin: vibrator-state {
> + core-pins {
> + pins = "gpio27";
> + function = "gp1_clk";
> + drive-strength = <6>;
> + bias-disable;
> + };
> +
> + enable-pins {
> + pins = "gpio60";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + };
> + };
> };
>
> &usb {
>
On 27/04/2023 21:34, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Document that.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> Documentation/devicetree/bindings/input/pwm-vibrator.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
> index d32716c604fe..6398534b43c3 100644
> --- a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
> +++ b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
> @@ -32,6 +32,8 @@ properties:
> minItems: 1
> maxItems: 2
>
> + enable-gpios: true
> +
> vcc-supply: true
>
> direction-duty-cycle-ns:
>
On 27/04/2023 21:34, Luca Weiss wrote:
> Make sure all printed messages end with a newline.
>
> Signed-off-by: Luca Weiss <[email protected]>
Reviewed-by: Caleb Connolly <[email protected]>
> ---
> drivers/input/misc/pwm-vibra.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index d0e58a7cdfa3..c08971c97ad6 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -42,7 +42,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> if (!vibrator->vcc_on) {
> err = regulator_enable(vibrator->vcc);
> if (err) {
> - dev_err(pdev, "failed to enable regulator: %d", err);
> + dev_err(pdev, "failed to enable regulator: %d\n", err);
> return err;
> }
> vibrator->vcc_on = true;
> @@ -54,7 +54,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>
> err = pwm_apply_state(vibrator->pwm, &state);
> if (err) {
> - dev_err(pdev, "failed to apply pwm state: %d", err);
> + dev_err(pdev, "failed to apply pwm state: %d\n", err);
> return err;
> }
>
> @@ -65,7 +65,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>
> err = pwm_apply_state(vibrator->pwm_dir, &state);
> if (err) {
> - dev_err(pdev, "failed to apply dir-pwm state: %d", err);
> + dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
> pwm_disable(vibrator->pwm);
> return err;
> }
> @@ -137,7 +137,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> err = PTR_ERR_OR_ZERO(vibrator->vcc);
> if (err) {
> if (err != -EPROBE_DEFER)
> - dev_err(&pdev->dev, "Failed to request regulator: %d",
> + dev_err(&pdev->dev, "Failed to request regulator: %d\n",
> err);
> return err;
> }
> @@ -146,7 +146,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> err = PTR_ERR_OR_ZERO(vibrator->pwm);
> if (err) {
> if (err != -EPROBE_DEFER)
> - dev_err(&pdev->dev, "Failed to request main pwm: %d",
> + dev_err(&pdev->dev, "Failed to request main pwm: %d\n",
> err);
> return err;
> }
> @@ -158,7 +158,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> state.enabled = false;
> err = pwm_apply_state(vibrator->pwm, &state);
> if (err) {
> - dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
> + dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> err);
> return err;
> }
> @@ -172,7 +172,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> state.enabled = false;
> err = pwm_apply_state(vibrator->pwm_dir, &state);
> if (err) {
> - dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
> + dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> err);
> return err;
> }
> @@ -189,7 +189,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> break;
>
> default:
> - dev_err(&pdev->dev, "Failed to request direction pwm: %d", err);
> + dev_err(&pdev->dev, "Failed to request direction pwm: %d\n", err);
> fallthrough;
>
> case -EPROBE_DEFER:
> @@ -207,13 +207,13 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> err = input_ff_create_memless(vibrator->input, NULL,
> pwm_vibrator_play_effect);
> if (err) {
> - dev_err(&pdev->dev, "Couldn't create FF dev: %d", err);
> + dev_err(&pdev->dev, "Couldn't create FF dev: %d\n", err);
> return err;
> }
>
> err = input_register_device(vibrator->input);
> if (err) {
> - dev_err(&pdev->dev, "Couldn't register input dev: %d", err);
> + dev_err(&pdev->dev, "Couldn't register input dev: %d\n", err);
> return err;
> }
>
>
On Freitag, 28. April 2023 01:29:27 CEST Brian Masney wrote:
> On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
> > Some pwm vibrators have a dedicated enable GPIO that needs to be set
> > high so that the vibrator works. Add support for that optionally.
> >
> > Signed-off-by: Luca Weiss <[email protected]>
>
> Hi Luca,
>
> Thank you for picking up this work!
>
> > + vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev,
"enable",
> > +
GPIOD_OUT_LOW);
> > + err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
> > + if (err) {
> > + if (err != -EPROBE_DEFER)
> > + dev_err(&pdev->dev, "Failed to request enable
gpio: %d\n",
> > + err);
> > + return err;
> > + }
> > +
>
> Take a look at dev_err_probe() to remove the -EPROBE_DEFER check.
The input subsystem doesn't like dev_err_probe for some reason, you should
quickly find examples of that being rejected on the mailing list (or see
"git grep dev_err_probe drivers/input/")
>
> With that fixed:
>
> Reviewed-by: Brian Masney <[email protected]>
Thanks for the reviews!
On Fri, Apr 28, 2023 at 06:06:20PM +0200, Luca Weiss wrote:
> On Freitag, 28. April 2023 01:29:27 CEST Brian Masney wrote:
> > Take a look at dev_err_probe() to remove the -EPROBE_DEFER check.
>
> The input subsystem doesn't like dev_err_probe for some reason, you should
> quickly find examples of that being rejected on the mailing list (or see
> "git grep dev_err_probe drivers/input/")
OK, that's fine then. Feel free to include my Reviewed-by.
Brian
Hi,
On Thu, Apr 27, 2023 at 10:34:26PM +0200, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Document that.
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> Documentation/devicetree/bindings/input/pwm-vibrator.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
> index d32716c604fe..6398534b43c3 100644
> --- a/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
> +++ b/Documentation/devicetree/bindings/input/pwm-vibrator.yaml
> @@ -32,6 +32,8 @@ properties:
> minItems: 1
> maxItems: 2
>
> + enable-gpios: true
> +
> vcc-supply: true
>
> direction-duty-cycle-ns:
>
> --
> 2.40.0
>
Hi,
On Thu, Apr 27, 2023 at 10:34:27PM +0200, Luca Weiss wrote:
> Make sure all printed messages end with a newline.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/input/misc/pwm-vibra.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index d0e58a7cdfa3..c08971c97ad6 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -42,7 +42,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> if (!vibrator->vcc_on) {
> err = regulator_enable(vibrator->vcc);
> if (err) {
> - dev_err(pdev, "failed to enable regulator: %d", err);
> + dev_err(pdev, "failed to enable regulator: %d\n", err);
> return err;
> }
> vibrator->vcc_on = true;
> @@ -54,7 +54,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>
> err = pwm_apply_state(vibrator->pwm, &state);
> if (err) {
> - dev_err(pdev, "failed to apply pwm state: %d", err);
> + dev_err(pdev, "failed to apply pwm state: %d\n", err);
> return err;
> }
>
> @@ -65,7 +65,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>
> err = pwm_apply_state(vibrator->pwm_dir, &state);
> if (err) {
> - dev_err(pdev, "failed to apply dir-pwm state: %d", err);
> + dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
> pwm_disable(vibrator->pwm);
> return err;
> }
> @@ -137,7 +137,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> err = PTR_ERR_OR_ZERO(vibrator->vcc);
> if (err) {
> if (err != -EPROBE_DEFER)
> - dev_err(&pdev->dev, "Failed to request regulator: %d",
> + dev_err(&pdev->dev, "Failed to request regulator: %d\n",
> err);
> return err;
> }
> @@ -146,7 +146,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> err = PTR_ERR_OR_ZERO(vibrator->pwm);
> if (err) {
> if (err != -EPROBE_DEFER)
> - dev_err(&pdev->dev, "Failed to request main pwm: %d",
> + dev_err(&pdev->dev, "Failed to request main pwm: %d\n",
> err);
> return err;
> }
> @@ -158,7 +158,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> state.enabled = false;
> err = pwm_apply_state(vibrator->pwm, &state);
> if (err) {
> - dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
> + dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> err);
> return err;
> }
> @@ -172,7 +172,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> state.enabled = false;
> err = pwm_apply_state(vibrator->pwm_dir, &state);
> if (err) {
> - dev_err(&pdev->dev, "failed to apply initial PWM state: %d",
> + dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
> err);
> return err;
> }
> @@ -189,7 +189,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> break;
>
> default:
> - dev_err(&pdev->dev, "Failed to request direction pwm: %d", err);
> + dev_err(&pdev->dev, "Failed to request direction pwm: %d\n", err);
> fallthrough;
>
> case -EPROBE_DEFER:
> @@ -207,13 +207,13 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> err = input_ff_create_memless(vibrator->input, NULL,
> pwm_vibrator_play_effect);
> if (err) {
> - dev_err(&pdev->dev, "Couldn't create FF dev: %d", err);
> + dev_err(&pdev->dev, "Couldn't create FF dev: %d\n", err);
> return err;
> }
>
> err = input_register_device(vibrator->input);
> if (err) {
> - dev_err(&pdev->dev, "Couldn't register input dev: %d", err);
> + dev_err(&pdev->dev, "Couldn't register input dev: %d\n", err);
> return err;
> }
>
>
> --
> 2.40.0
>
Hi,
On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Add support for that optionally.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
Reviewed-by: Sebastian Reichel <[email protected]>
-- Sebastian
> drivers/input/misc/pwm-vibra.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index c08971c97ad6..2ba035299db8 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -11,6 +11,7 @@
> * Copyright (C) 2010, Lars-Peter Clausen <[email protected]>
> */
>
> +#include <linux/gpio/consumer.h>
> #include <linux/input.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -23,6 +24,7 @@
>
> struct pwm_vibrator {
> struct input_dev *input;
> + struct gpio_desc *enable_gpio;
> struct pwm_device *pwm;
> struct pwm_device *pwm_dir;
> struct regulator *vcc;
> @@ -48,6 +50,8 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> vibrator->vcc_on = true;
> }
>
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
> +
> pwm_get_state(vibrator->pwm, &state);
> pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
> state.enabled = true;
> @@ -80,6 +84,8 @@ static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
> pwm_disable(vibrator->pwm_dir);
> pwm_disable(vibrator->pwm);
>
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
> +
> if (vibrator->vcc_on) {
> regulator_disable(vibrator->vcc);
> vibrator->vcc_on = false;
> @@ -142,6 +148,16 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
> return err;
> }
>
> + vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> + GPIOD_OUT_LOW);
> + err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
> + if (err) {
> + if (err != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
> + err);
> + return err;
> + }
> +
> vibrator->pwm = devm_pwm_get(&pdev->dev, "enable");
> err = PTR_ERR_OR_ZERO(vibrator->pwm);
> if (err) {
>
> --
> 2.40.0
>
On 27/04/2023 22:34, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Document that.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> high so that the vibrator works. Add support for that optionally.
So this is not simply a power supply in your case controlled by a GPIO?
We truly can have both GPIO and a separate regulator?
Thanks.
--
Dmitry
On 27.04.2023 22:34, Luca Weiss wrote:
> The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
> which we can use with the clk-pwm driver, then we can use that pwm with
> pwm-vibrator to get haptics functionality.
>
> This patch is based on Brian Masney's previous patch with clk-vibrator.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
> .../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35 ++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> index ab35f2d644c0..fea8a6be9021 100644
> --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> @@ -41,6 +41,25 @@ key-volume-down {
> };
> };
>
> + clk_pwm: pwm {
> + compatible = "clk-pwm";
> + clocks = <&mmcc CAMSS_GP1_CLK>;
Are you sure it's <&mmcc CAMSS_GP1_CLK> and not <&gcc GCC_GP1_CLK>?
Konrad
> +
> + pinctrl-0 = <&vibrator_pin>;
> + pinctrl-names = "default";
> +
> + #pwm-cells = <2>;
> + };
> +
> + vibrator {
> + compatible = "pwm-vibrator";
> + pwms = <&clk_pwm 0 100000>;
> + pwm-names = "enable";
> +
> + vcc-supply = <&pm8941_l19>;
> + enable-gpios = <&tlmm 60 GPIO_ACTIVE_HIGH>;
> + };
> +
> vreg_wlan: wlan-regulator {
> compatible = "regulator-fixed";
>
> @@ -637,6 +656,22 @@ shutdown-pins {
> function = "gpio";
> };
> };
> +
> + vibrator_pin: vibrator-state {
> + core-pins {
> + pins = "gpio27";
> + function = "gp1_clk";
> + drive-strength = <6>;
> + bias-disable;
> + };
> +
> + enable-pins {
> + pins = "gpio60";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + };
> + };
> };
>
> &usb {
>
On 28.04.2023 18:06, Luca Weiss wrote:
> On Freitag, 28. April 2023 01:29:27 CEST Brian Masney wrote:
>> On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
>>> Some pwm vibrators have a dedicated enable GPIO that needs to be set
>>> high so that the vibrator works. Add support for that optionally.
>>>
>>> Signed-off-by: Luca Weiss <[email protected]>
>>
>> Hi Luca,
>>
>> Thank you for picking up this work!
>>
>>> + vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev,
> "enable",
>>> +
> GPIOD_OUT_LOW);
>>> + err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
>>> + if (err) {
>>> + if (err != -EPROBE_DEFER)
>>> + dev_err(&pdev->dev, "Failed to request enable
> gpio: %d\n",
>>> + err);
>>> + return err;
>>> + }
>>> +
>>
Looks like your email client messes with the replies.. perhaps it tries
to round them to n characters forcefully?
Konrad
>> Take a look at dev_err_probe() to remove the -EPROBE_DEFER check.
>
> The input subsystem doesn't like dev_err_probe for some reason, you should
> quickly find examples of that being rejected on the mailing list (or see
> "git grep dev_err_probe drivers/input/")
>
>>
>> With that fixed:
>>
>> Reviewed-by: Brian Masney <[email protected]>
>
> Thanks for the reviews!
>
>
On Dienstag, 2. Mai 2023 12:39:10 CEST Konrad Dybcio wrote:
> On 28.04.2023 18:06, Luca Weiss wrote:
> > On Freitag, 28. April 2023 01:29:27 CEST Brian Masney wrote:
> >> On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
> >>> Some pwm vibrators have a dedicated enable GPIO that needs to be set
> >>> high so that the vibrator works. Add support for that optionally.
> >>>
> >>> Signed-off-by: Luca Weiss <[email protected]>
> >>
> >> Hi Luca,
> >>
> >> Thank you for picking up this work!
> >>
> >>> + vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev,
> >
> > "enable",
> >
> >>> +
> >
> > GPIOD_OUT_LOW);
> >
> >>> + err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
> >>> + if (err) {
> >>> + if (err != -EPROBE_DEFER)
> >>> + dev_err(&pdev->dev, "Failed to request enable
> >
> > gpio: %d\n",
> >
> >>> + err);
> >>> + return err;
> >>> + }
> >>> +
>
> Looks like your email client messes with the replies.. perhaps it tries
> to round them to n characters forcefully?
Quite possible, I'm using KMail with Options -> Wordwrap turned on, otherwise
I have to manually wrap everything but with this on there doesn't seem to be a
way to get over that limit, even when posting links etc - or when quoting
existing text.
Regards
Luca
>
> Konrad
>
> >> Take a look at dev_err_probe() to remove the -EPROBE_DEFER check.
> >
> > The input subsystem doesn't like dev_err_probe for some reason, you should
> > quickly find examples of that being rejected on the mailing list (or see
> > "git grep dev_err_probe drivers/input/")
> >
> >> With that fixed:
> >>
> >> Reviewed-by: Brian Masney <[email protected]>
> >
> > Thanks for the reviews!
On Dienstag, 2. Mai 2023 12:40:40 CEST Konrad Dybcio wrote:
> On 27.04.2023 22:34, Luca Weiss wrote:
> > The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
> > which we can use with the clk-pwm driver, then we can use that pwm with
> > pwm-vibrator to get haptics functionality.
> >
> > This patch is based on Brian Masney's previous patch with clk-vibrator.
> >
> > Signed-off-by: Luca Weiss <[email protected]>
> > ---
> >
> > .../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35
> > ++++++++++++++++++++++ 1 file changed, 35 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> > b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts index
> > ab35f2d644c0..fea8a6be9021 100644
> > --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> > +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> > @@ -41,6 +41,25 @@ key-volume-down {
> >
> > };
> >
> > };
> >
> > + clk_pwm: pwm {
> > + compatible = "clk-pwm";
> > + clocks = <&mmcc CAMSS_GP1_CLK>;
>
> Are you sure it's <&mmcc CAMSS_GP1_CLK> and not <&gcc GCC_GP1_CLK>?
Quite sure.
The driver uses:
cam_gp1_clk = clk_get(&pdev->dev, "cam_gp1_clk");
and this comes from the clock-8974.c driver
CLK_LOOKUP("cam_gp1_clk", camss_gp1_clk.c, "vibrator"),
Regards
Luca
>
> Konrad
>
> > +
> > + pinctrl-0 = <&vibrator_pin>;
> > + pinctrl-names = "default";
> > +
> > + #pwm-cells = <2>;
> > + };
> > +
> > + vibrator {
> > + compatible = "pwm-vibrator";
> > + pwms = <&clk_pwm 0 100000>;
> > + pwm-names = "enable";
> > +
> > + vcc-supply = <&pm8941_l19>;
> > + enable-gpios = <&tlmm 60 GPIO_ACTIVE_HIGH>;
> > + };
> > +
> >
> > vreg_wlan: wlan-regulator {
> >
> > compatible = "regulator-fixed";
> >
> > @@ -637,6 +656,22 @@ shutdown-pins {
> >
> > function = "gpio";
> >
> > };
> >
> > };
> >
> > +
> > + vibrator_pin: vibrator-state {
> > + core-pins {
> > + pins = "gpio27";
> > + function = "gp1_clk";
> > + drive-strength = <6>;
> > + bias-disable;
> > + };
> > +
> > + enable-pins {
> > + pins = "gpio60";
> > + function = "gpio";
> > + drive-strength = <2>;
> > + bias-disable;
> > + };
> > + };
> >
> > };
> >
> > &usb {
On 2.05.2023 17:28, Luca Weiss wrote:
> On Dienstag, 2. Mai 2023 12:40:40 CEST Konrad Dybcio wrote:
>> On 27.04.2023 22:34, Luca Weiss wrote:
>>> The Nexus 5 has a vibrator connected to the clock output of GP1_CLK
>>> which we can use with the clk-pwm driver, then we can use that pwm with
>>> pwm-vibrator to get haptics functionality.
>>>
>>> This patch is based on Brian Masney's previous patch with clk-vibrator.
>>>
>>> Signed-off-by: Luca Weiss <[email protected]>
>>> ---
>>>
>>> .../dts/qcom-msm8974-lge-nexus5-hammerhead.dts | 35
>>> ++++++++++++++++++++++ 1 file changed, 35 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
>>> b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts index
>>> ab35f2d644c0..fea8a6be9021 100644
>>> --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
>>> +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
>>> @@ -41,6 +41,25 @@ key-volume-down {
>>>
>>> };
>>>
>>> };
>>>
>>> + clk_pwm: pwm {
>>> + compatible = "clk-pwm";
>>> + clocks = <&mmcc CAMSS_GP1_CLK>;
>>
>> Are you sure it's <&mmcc CAMSS_GP1_CLK> and not <&gcc GCC_GP1_CLK>?
>
> Quite sure.
>
> The driver uses:
>
> cam_gp1_clk = clk_get(&pdev->dev, "cam_gp1_clk");
>
> and this comes from the clock-8974.c driver
>
> CLK_LOOKUP("cam_gp1_clk", camss_gp1_clk.c, "vibrator"),
>
> Regards
> Luca
ugh that hurts my brain but fine, maybe the camss clock had a
pad closer to the vibrator pcb traces..
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
>
>>
>> Konrad
>>
>>> +
>>> + pinctrl-0 = <&vibrator_pin>;
>>> + pinctrl-names = "default";
>>> +
>>> + #pwm-cells = <2>;
>>> + };
>>> +
>>> + vibrator {
>>> + compatible = "pwm-vibrator";
>>> + pwms = <&clk_pwm 0 100000>;
>>> + pwm-names = "enable";
>>> +
>>> + vcc-supply = <&pm8941_l19>;
>>> + enable-gpios = <&tlmm 60 GPIO_ACTIVE_HIGH>;
>>> + };
>>> +
>>>
>>> vreg_wlan: wlan-regulator {
>>>
>>> compatible = "regulator-fixed";
>>>
>>> @@ -637,6 +656,22 @@ shutdown-pins {
>>>
>>> function = "gpio";
>>>
>>> };
>>>
>>> };
>>>
>>> +
>>> + vibrator_pin: vibrator-state {
>>> + core-pins {
>>> + pins = "gpio27";
>>> + function = "gp1_clk";
>>> + drive-strength = <6>;
>>> + bias-disable;
>>> + };
>>> +
>>> + enable-pins {
>>> + pins = "gpio60";
>>> + function = "gpio";
>>> + drive-strength = <2>;
>>> + bias-disable;
>>> + };
>>> + };
>>>
>>> };
>>>
>>> &usb {
>
>
>
>
On Dienstag, 2. Mai 2023 02:47:29 CEST Dmitry Torokhov wrote:
> On Thu, Apr 27, 2023 at 10:34:28PM +0200, Luca Weiss wrote:
> > Some pwm vibrators have a dedicated enable GPIO that needs to be set
> > high so that the vibrator works. Add support for that optionally.
>
> So this is not simply a power supply in your case controlled by a GPIO?
> We truly can have both GPIO and a separate regulator?
Yes it appears to be the EN pin on the ISA1000A, see
https://electronics.stackexchange.com/q/380475
On apq8026-lg-lenok there is a similar setup for the vibration motor although
there I don't know whether it's actually a fixed-regulator or not, but since
the two devices were built in a similar time (without checking further) I
could assume they both contain the same IC.
Regards
Luca
>
> Thanks.
On Thu, Apr 27, 2023 at 10:34:25PM +0200, Luca Weiss wrote:
> A while ago Brian Masney sent some patches for a clk-vibrator which was
> then succeeded by the idea of a clk-pwm driver that "converts" a clock
> into a PWM and to use the existing pwm-vibra driver.
>
> Since clk-pwm has landed last year we can finally add haptics support
> upstream.
>
> We just need to add support for an enable GPIO to the pwm-vibra driver
> since that also needs to be high for the haptics to work on this device.
>
> Signed-off-by: Luca Weiss <[email protected]>
> ---
> Luca Weiss (4):
> dt-bindings: input: pwm-vibrator: Add enable-gpio
> Input: pwm-vibra - add newline to dev_err prints
> Input: pwm-vibra - add support for enable GPIO
> ARM: dts: qcom: msm8974-hammerhead: Add vibrator
Applied patches 1-3, thank you.
--
Dmitry
On Thu, 27 Apr 2023 22:34:25 +0200, Luca Weiss wrote:
> A while ago Brian Masney sent some patches for a clk-vibrator which was
> then succeeded by the idea of a clk-pwm driver that "converts" a clock
> into a PWM and to use the existing pwm-vibra driver.
>
> Since clk-pwm has landed last year we can finally add haptics support
> upstream.
>
> [...]
Applied, thanks!
[4/4] ARM: dts: qcom: msm8974-hammerhead: Add vibrator
commit: e0a6590d8ceb7d6c4e35b5b5eb368d9fb800487f
Best regards,
--
Bjorn Andersson <[email protected]>