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@xxxxxxxxxxxxx> 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