On Thursday, Feb 27, 2020 at 1:13 AM, Uwe Kleine-König wrote:
> On Fri, Feb 21, 2020 at 04:27:05PM +0900, Roy Im wrote:
> > Adds support for the Dialog DA7280 LRA/ERM Haptic Driver with multiple
> > mode and integrated waveform memory and wideband support.
> > It communicates via an I2C bus to the device.
> >
> > Signed-off-by: Roy Im <roy.im.opensource@xxxxxxxxxxx>
> >
> > ---
> > v9:
> > - Removed the header file and put the definitions into the c file.
> > - Updated the pwm code and error logs with %pE
> > +
> > +struct da7280_haptic {
> > + struct regmap *regmap;
> > + struct input_dev *input_dev;
> > + struct device *dev;
> > + struct i2c_client *client;
> > + struct pwm_device *pwm_dev;
> > +
> > + bool legacy;
> > + int pwm_id;
>
> pwm_id is unused?
Okay, I will remove this.
[...]
> > +static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool
> > +enabled) {
> > + struct pwm_state state;
> > + u64 period_mag_multi;
> > + int error;
> > +
> > + if (!haptics->gain) {
> > + dev_err(haptics->dev,
> > + "Please set the gain first for the pwm mode\n");
> > + return -EINVAL;
> > + }
> > +
> > + pwm_get_state(haptics->pwm_dev, &state);
> > + state.enabled = enabled;
> > + period_mag_multi = enabled ? state.period * haptics->gain : 0;
>
> if .enabled is false .duty_cycle has no effect on the output. So there is no good reason for setting the duty_cyle to
> something different for a disabled PWM.
Okay, I will add a condition for this.
> > + state.duty_cycle = (haptics->acc_en) ?
> > + (unsigned int)(period_mag_multi >> 15) :
> > + (unsigned int)((period_mag_multi >> 15)
> > + + state.period) / 2;
>
> This can be written in a more readable way:
>
> /* would be great to have a symbolic name for 15 */
> period_mag_multi >>= 15;
>
> /* comment with a rational here */
> if (!haptics->acc_en) {
> period_mag_multi += state.period;
> period_mag_multi /= 2;
> }
>
> state.duty_cycle = period_mag_multi;
Okay, thanks. I have tried to update that as below.
Could I get your comment if you still see anything on this?
/* Maximum gain is 0x7fff for PWM mode */
#define MAX_MAGNITUDE_SHIFT 15
[...]
period_mag_multi >>= MAX_MAGNITUDE_SHIFT;
/* The interpretation of duty cycle depends on the acc_en,
* it should be from 50% to 100% for acc_en = 0.
* See datasheet 'PWM mode' section for more details.
*/
if (!haptics->acc_en) {
period_mag_multi += state.period;
period_mag_multi /= 2;
}
> > [...]
> > + pwm_init_state(haptics->pwm_dev, &state);
> > + state.enabled = false;
> > + error = pwm_apply_state(haptics->pwm_dev, &state);
> > + if (error) {
> > + dev_err(dev,
> > + "failed to apply initial PWM state: %pE\n",
> > + ERR_PTR(error));
> > + return error;
> > + }
> > +
> > + /* Check PWM Period, it must be in 10k ~ 250kHz */
> > + period2freq = 1000000 / pwm_get_period(haptics->pwm_dev);
>
> You already know the return value of pwm_get_period(), it's state.period.
Right, I will replace that to state.period.
> > + if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
> > + period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
> > [...]
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
Thanks a lot for your comments.
Best regards,
Roy
