| Message ID | 20190516085018.2207-1-masneyb@onstation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator | expand |
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.
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
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.
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
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
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
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
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
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
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.
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 {
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(+)