[PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Brian Masney]

This patch adds device device tree bindings for the vibrator found on
the LG Nexus 5 (hammerhead) phone.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
This is a resend of the following patch that has missed the last two
merge windows:

https://lore.kernel.org/lkml/20190206013329.18195-4-masneyb@onstation.org/

 .../qcom-msm8974-lge-nexus5-hammerhead.dts    | 31 +++++++++++++++++++
 1 file changed, 31 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 b3b04736a159..1fd9f429f34a 100644
--- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
+++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
@@ -5,6 +5,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+#include <dt-bindings/clock/qcom,mmcc-msm8974.h>
 
 / {
 	model = "LGE MSM 8974 HAMMERHEAD";
@@ -306,6 +307,36 @@
 				input-enable;
 			};
 		};
+
+		vibrator_pin: vibrator {
+			pwm {
+				pins = "gpio27";
+				function = "gp1_clk";
+
+				drive-strength = <6>;
+				bias-disable;
+			};
+
+			enable {
+				pins = "gpio60";
+				function = "gpio";
+			};
+		};
+	};
+
+	vibrator@fd8c3450 {
+		compatible = "qcom,msm8974-vibrator";
+		reg = <0xfd8c3450 0x400>;
+
+		vcc-supply = <&pm8941_l19>;
+
+		clocks = <&mmcc CAMSS_GP1_CLK>;
+		clock-names = "pwm";
+
+		enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&vibrator_pin>;
 	};
 
 	sdhci@f9824900 {
-- 
2.17.2

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Stephen Boyd]

Quoting Brian Masney (2019-05-16 01:50:18)
> @@ -306,6 +307,36 @@
>                                 input-enable;
>                         };
>                 };
> +
> +               vibrator_pin: vibrator {
> +                       pwm {
> +                               pins = "gpio27";
> +                               function = "gp1_clk";
> +
> +                               drive-strength = <6>;
> +                               bias-disable;
> +                       };
> +
> +                       enable {
> +                               pins = "gpio60";
> +                               function = "gpio";
> +                       };
> +               };
> +       };
> +
> +       vibrator@fd8c3450 {
> +               compatible = "qcom,msm8974-vibrator";
> +               reg = <0xfd8c3450 0x400>;

This is inside the multimedia clk controller. The resource reservation
mechanism should be complaining loudly here. Is the driver writing
directly into clk controller registers to adjust a duty cycle of the
camera's general purpose clk?

Can you add support for duty cycle to the qcom clk driver's RCGs and
then write a generic clk duty cycle vibrator driver that adjusts the
duty cycle of the clk? That would be better than reaching into the clk
controller registers to do this.

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Brian Masney]

Hi Stephen,

On Mon, May 20, 2019 at 07:21:49AM -0700, Stephen Boyd wrote:
> Quoting Brian Masney (2019-05-16 01:50:18)
> > @@ -306,6 +307,36 @@
> >                                 input-enable;
> >                         };
> >                 };
> > +
> > +               vibrator_pin: vibrator {
> > +                       pwm {
> > +                               pins = "gpio27";
> > +                               function = "gp1_clk";
> > +
> > +                               drive-strength = <6>;
> > +                               bias-disable;
> > +                       };
> > +
> > +                       enable {
> > +                               pins = "gpio60";
> > +                               function = "gpio";
> > +                       };
> > +               };
> > +       };
> > +
> > +       vibrator@fd8c3450 {
> > +               compatible = "qcom,msm8974-vibrator";
> > +               reg = <0xfd8c3450 0x400>;
> 
> This is inside the multimedia clk controller. The resource reservation
> mechanism should be complaining loudly here. Is the driver writing
> directly into clk controller registers to adjust a duty cycle of the
> camera's general purpose clk?
> 
> Can you add support for duty cycle to the qcom clk driver's RCGs and
> then write a generic clk duty cycle vibrator driver that adjusts the
> duty cycle of the clk? That would be better than reaching into the clk
> controller registers to do this.

I don't see any complaints in dmesg about this, however I'll work on a
new clk duty cycle vibrator driver.

Brian

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Stephen Boyd]

Quoting Brian Masney (2019-05-22 01:23:48)
> On Mon, May 20, 2019 at 07:21:49AM -0700, Stephen Boyd wrote:
> > 
> > This is inside the multimedia clk controller. The resource reservation
> > mechanism should be complaining loudly here. Is the driver writing
> > directly into clk controller registers to adjust a duty cycle of the
> > camera's general purpose clk?
> > 
> > Can you add support for duty cycle to the qcom clk driver's RCGs and
> > then write a generic clk duty cycle vibrator driver that adjusts the
> > duty cycle of the clk? That would be better than reaching into the clk
> > controller registers to do this.
> 
> I don't see any complaints in dmesg about this, however I'll work on a
> new clk duty cycle vibrator driver.
> 

Ok. Probably no warning because the vibrator driver just creates the io
mapping without trying to reserve it with the io requesting APIs.

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Linus Walleij]

On Mon, May 20, 2019 at 4:21 PM Stephen Boyd <sboyd@kernel.org> wrote:

> > +       vibrator@fd8c3450 {
> > +               compatible = "qcom,msm8974-vibrator";
> > +               reg = <0xfd8c3450 0x400>;
>
> This is inside the multimedia clk controller. The resource reservation
> mechanism should be complaining loudly here. Is the driver writing
> directly into clk controller registers to adjust a duty cycle of the
> camera's general purpose clk?
>
> Can you add support for duty cycle to the qcom clk driver's RCGs and
> then write a generic clk duty cycle vibrator driver that adjusts the
> duty cycle of the clk? That would be better than reaching into the clk
> controller registers to do this.

There is something ontological about this.

A clock with variable duty cycle, isn't that by definition a PWM?
I don't suppose it is normal for qcom clocks to be able to control
their duty cycle, but rather default to 50/50 as we could expect?

I would rather say that maybe the qcom drivers/clk/qcom/* file
should be exporting a PWM from the linux side of things
rather than a clock for this thingie, and adding #pwm-cells
in the DT node for the clock controller, making it possible
to obtain PWMs right out of it, if it is a single device node for
the whole thing.

Analogous to how we have GPIOs that are ortogonally interrupt
providers I don't see any big problem in a clock controller
being clock and PWM provider at the same time.

There is code in drivers/clk/clk-pwm to use a pwm as a clock
but that is kind of the reverse use case, if we implement PWMs
directly in a clock controller driver then these can be turned into
clocks using clk-pwm.c should it be needed, right?

Part of me start to question whether clk and pwm should even
be separate subsystems :/ they seem to solve an overlapping
problem space.

Yours,
Linus Walleij

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Brian Masney]

On Wed, May 29, 2019 at 11:13:15AM +0200, Linus Walleij wrote:
> On Mon, May 20, 2019 at 4:21 PM Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > > +       vibrator@fd8c3450 {
> > > +               compatible = "qcom,msm8974-vibrator";
> > > +               reg = <0xfd8c3450 0x400>;
> >
> > This is inside the multimedia clk controller. The resource reservation
> > mechanism should be complaining loudly here. Is the driver writing
> > directly into clk controller registers to adjust a duty cycle of the
> > camera's general purpose clk?
> >
> > Can you add support for duty cycle to the qcom clk driver's RCGs and
> > then write a generic clk duty cycle vibrator driver that adjusts the
> > duty cycle of the clk? That would be better than reaching into the clk
> > controller registers to do this.
> 
> There is something ontological about this.
> 
> A clock with variable duty cycle, isn't that by definition a PWM?
> I don't suppose it is normal for qcom clocks to be able to control
> their duty cycle, but rather default to 50/50 as we could expect?
> 
> I would rather say that maybe the qcom drivers/clk/qcom/* file
> should be exporting a PWM from the linux side of things
> rather than a clock for this thingie, and adding #pwm-cells
> in the DT node for the clock controller, making it possible
> to obtain PWMs right out of it, if it is a single device node for
> the whole thing.
> 
> Analogous to how we have GPIOs that are ortogonally interrupt
> providers I don't see any big problem in a clock controller
> being clock and PWM provider at the same time.
> 
> There is code in drivers/clk/clk-pwm to use a pwm as a clock
> but that is kind of the reverse use case, if we implement PWMs
> directly in a clock controller driver then these can be turned into
> clocks using clk-pwm.c should it be needed, right?
> 
> Part of me start to question whether clk and pwm should even
> be separate subsystems :/ they seem to solve an overlapping
> problem space.

My first revision of this vibrator driver used the Linux PWM framework
due to the variable duty cycle:

https://lore.kernel.org/lkml/20180926235112.25710-1-masneyb@onstation.org/

I used the pwm-vibra driver on the input side.

Brian

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Linus Walleij]

On Wed, May 29, 2019 at 12:12 PM Brian Masney <masneyb@onstation.org> wrote:

> My first revision of this vibrator driver used the Linux PWM framework
> due to the variable duty cycle:

So what I perceive if I get the thread right is that actually a lot of
qcom clocks (all with the M/N/D counter set-up) have variable duty
cycle. Very few consumers use that feature.

It would be a bit much to ask that they all be implemented as PWMs
and then cast into clocks for the 50/50 dutycycle case, I get that.

What about simply doing both?

Export the same clocks from the clk and pwm frameworks and be
happy. Of course with some mutex inside the driver so that it can't
be used from both ends at the same time.

Further Thierry comments
https://lore.kernel.org/lkml/20181012114749.GC31561@ulmo/

> The device itself doesn't seem to be a
> generic PWM in the way that the PWM framework
> expects it.

I don't see why.  I just look at this function from the original
patch series:

+static int msm_vibra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct msm_vibra_pwm *msm_pwm = to_msm_vibra_pwm(chip);
+ int d_reg_val;
+
+ d_reg_val = 127 - (((duty_ns / 1000) * 126) / (period_ns / 1000));
+
+ msm_vibra_pwm_write(msm_pwm, REG_CFG_RCGR,
+    (2 << 12) | /* dual edge mode */
+    (0 << 8) |  /* cxo */
+    (7 << 0));
+ msm_vibra_pwm_write(msm_pwm, REG_M, 1);
+ msm_vibra_pwm_write(msm_pwm, REG_N, 128);
+ msm_vibra_pwm_write(msm_pwm, REG_D, d_reg_val);
+ msm_vibra_pwm_write(msm_pwm, REG_CMD_RCGR, 1);
+ msm_vibra_pwm_write(msm_pwm, REG_CBCR, 1);
+
+ return 0;
+}

How is this NOT a a generic PWM in the way that the PWM
framework expects it? It configures the period and duty cycle on
a square wave, that is what a generic PWM is in my book.

Yours,
Linus Walleij

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Brian Masney]

Hi Stephen and Thierry,

On Fri, May 31, 2019 at 12:51:38PM +0200, Linus Walleij wrote:
> On Wed, May 29, 2019 at 12:12 PM Brian Masney <masneyb@onstation.org> wrote:
> 
> > My first revision of this vibrator driver used the Linux PWM framework
> > due to the variable duty cycle:
> 
> So what I perceive if I get the thread right is that actually a lot of
> qcom clocks (all with the M/N/D counter set-up) have variable duty
> cycle. Very few consumers use that feature.
> 
> It would be a bit much to ask that they all be implemented as PWMs
> and then cast into clocks for the 50/50 dutycycle case, I get that.
> 
> What about simply doing both?
> 
> Export the same clocks from the clk and pwm frameworks and be
> happy. Of course with some mutex inside the driver so that it can't
> be used from both ends at the same time.

Do you have any feedback about this? As far as I understand, there are
two options on the table right now:

1) Add support for the duty cycle to the qcom clk driver and write a
   general purpose clk-vibra driver for the input subsystem.

2) Do what Linus suggests above. We can use v1 of this series from last
   September (see below for link) that adds this to the pwm subsystem.
   The locking would need to be added so that it won't conflict with the
   clk subsystem. This can be tied into the input subsystem with the
   existing pwm-vibra driver.

Either case, the msm-vibrator driver that I added to the input subsystem
will be dropped.

Thanks,

Brian

> 
> Further Thierry comments
> https://lore.kernel.org/lkml/20181012114749.GC31561@ulmo/
> 
> > The device itself doesn't seem to be a
> > generic PWM in the way that the PWM framework
> > expects it.
> 
> I don't see why.  I just look at this function from the original
> patch series:
> 
> +static int msm_vibra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct msm_vibra_pwm *msm_pwm = to_msm_vibra_pwm(chip);
> + int d_reg_val;
> +
> + d_reg_val = 127 - (((duty_ns / 1000) * 126) / (period_ns / 1000));
> +
> + msm_vibra_pwm_write(msm_pwm, REG_CFG_RCGR,
> +    (2 << 12) | /* dual edge mode */
> +    (0 << 8) |  /* cxo */
> +    (7 << 0));
> + msm_vibra_pwm_write(msm_pwm, REG_M, 1);
> + msm_vibra_pwm_write(msm_pwm, REG_N, 128);
> + msm_vibra_pwm_write(msm_pwm, REG_D, d_reg_val);
> + msm_vibra_pwm_write(msm_pwm, REG_CMD_RCGR, 1);
> + msm_vibra_pwm_write(msm_pwm, REG_CBCR, 1);
> +
> + return 0;
> +}
> 
> How is this NOT a a generic PWM in the way that the PWM
> framework expects it? It configures the period and duty cycle on
> a square wave, that is what a generic PWM is in my book.
> 
> Yours,
> Linus Walleij

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Linus Walleij]

On Sun, Jun 23, 2019 at 12:53 PM Brian Masney <masneyb@onstation.org> wrote:

> 2) Do what Linus suggests above. We can use v1 of this series from last
>    September (see below for link) that adds this to the pwm subsystem.
>    The locking would need to be added so that it won't conflict with the
>    clk subsystem. This can be tied into the input subsystem with the
>    existing pwm-vibra driver.

What I imagined was that the clk driver would double as a pwm driver.
Just register both interfaces.

There are already plenty of combines clk+reset drivers for example.

Otherwise I'm all for this approach (but that's just me).

Yours,
Linus Walleij

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Brian Masney]

On Tue, Jun 25, 2019 at 12:29:29AM +0200, Linus Walleij wrote:
> On Sun, Jun 23, 2019 at 12:53 PM Brian Masney <masneyb@onstation.org> wrote:
> 
> > 2) Do what Linus suggests above. We can use v1 of this series from last
> >    September (see below for link) that adds this to the pwm subsystem.
> >    The locking would need to be added so that it won't conflict with the
> >    clk subsystem. This can be tied into the input subsystem with the
> >    existing pwm-vibra driver.
> 
> What I imagined was that the clk driver would double as a pwm driver.
> Just register both interfaces.
> 
> There are already plenty of combines clk+reset drivers for example.
> 
> Otherwise I'm all for this approach (but that's just me).

I agree that this makes sense. I especially like that it'll allow us
to use the existing pwm-vibra driver in the input subsystem with this
approach.

Brian

Re: [PATCH RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator

[Stephen Boyd]

Quoting Brian Masney (2019-06-24 17:54:34)
> On Tue, Jun 25, 2019 at 12:29:29AM +0200, Linus Walleij wrote:
> > On Sun, Jun 23, 2019 at 12:53 PM Brian Masney <masneyb@onstation.org> wrote:
> > 
> > > 2) Do what Linus suggests above. We can use v1 of this series from last
> > >    September (see below for link) that adds this to the pwm subsystem.
> > >    The locking would need to be added so that it won't conflict with the
> > >    clk subsystem. This can be tied into the input subsystem with the
> > >    existing pwm-vibra driver.
> > 
> > What I imagined was that the clk driver would double as a pwm driver.
> > Just register both interfaces.
> > 
> > There are already plenty of combines clk+reset drivers for example.
> > 
> > Otherwise I'm all for this approach (but that's just me).
> 
> I agree that this makes sense. I especially like that it'll allow us
> to use the existing pwm-vibra driver in the input subsystem with this
> approach.
> 

This whole discussion is ignoring that clk_set_duty_cycle() exists.
Maybe you can look back on the history of why the duty cycle API was
added to the clk framework to make a strong argument for the replacement
of this API with your proposal of registering to two different
frameworks instead?

Here's the first few parts of the commit text of 9fba738a53dd ("clk: add
duty cycle support"):

    Add the possibility to apply and query the clock signal duty cycle ratio.

    This is useful when the duty cycle of the clock signal depends on some
    other parameters controlled by the clock framework.

    For example, the duty cycle of a divider may depends on the raw divider
    setting (ratio = N / div) , which is controlled by the CCF. In such case,
    going through the pwm framework to control the duty cycle ratio of this
    clock would be a burden.

In the case of qcom clks, I seem to recall the frequency of the clk
depends on the duty cycle settings. The duty cycle is constrained by the
M/N values which determine the frequency of the clk after it's
pre-divided. We did some sort of bit trick to set the duty cycle to the
N value inverted or something so that we always got 50% duty cycle.