Friday, Mar 06, 2020 at 4:06 PM, Uwe Kleine-König wrote
> On Fri, Mar 06, 2020 at 01:23:08AM +0900, Roy Im wrote:
> > +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;
> > + if (enabled) {
> > + period_mag_multi = state.period * haptics->gain;
> > + period_mag_multi >>= MAX_MAGNITUDE_SHIFT;
> > +
> > + /* The interpretation of duty cycle depends on the acc_en,
> > + * it should be form 50% to 100% for acc_en = 0.
>
> At least s/form/from/, but maybe better: it should be between 50% and 100% ...
Okay, got it and sorry for the typo, I will update as you advise..
> > + * See datasheet 'PWM mode' section.
> > + */
> > + if (!haptics->acc_en) {
> > + period_mag_multi += state.period;
> > + period_mag_multi /= 2;
> > + }
> > +
> > + state.duty_cycle = (unsigned int)period_mag_multi;
>
> This cast is not needed. (Also it seems struct pwm_state::duty_cycle
> becomes u64 soon, after this happens the cast even
> hurts.)
Okay, I will remove the cast.
> > [...]
> > + struct device *dev = &client->dev;
> > + struct da7280_haptic *haptics;
> > + struct input_dev *input_dev;
> > + struct ff_device *ff;
> > + struct pwm_state state;
> > + unsigned int period2freq;
> > + int error;
> > +
> > + haptics = devm_kzalloc(dev, sizeof(*haptics), GFP_KERNEL);
> > + if (!haptics)
> > + return -ENOMEM;
> > + haptics->dev = dev;
> > +
> > + if (!client->irq) {
> > + dev_err(dev, "No IRQ configured\n");
> > + return -EINVAL;
> > + }
> > +
> > + da7280_parse_properties(dev, haptics);
> > +
> > + if (haptics->const_op_mode == DA7280_PWM_MODE) {
> > + haptics->pwm_dev = devm_pwm_get(dev, NULL);
> > + if (IS_ERR(haptics->pwm_dev)) {
> > + dev_err(dev, "failed to get PWM device\n");
>
> Please use %pE to show the actual error and don't print if it is EPROBE_DEFER.
Okay, I will change as following:
if (IS_ERR(haptics->pwm_dev)) {
error = PTR_ERR(haptics->pwm_dev);
if (error != -EPROBE_DEFER)
dev_err(dev, "unable to request PWM: %pE\n",
ERR_PTR(error));
return error;
}
Do you still see anything on this changes?
> > + return PTR_ERR(haptics->pwm_dev);
> > + }
> > +
> > + pwm_init_state(haptics->pwm_dev, &state);
> > + state.enabled = false;
>
> This usuage is strange (which might be because pwm_init_state() is
> strange). I assume the goal here is to disable the PWM with the right
> polarity set, right? Ah, and initialize .period as this isn't touched later on. Hmm, no better idea, I guess we have to leave it as is for now.
Yes, that's right. I think so.
> Can it be that the PWM is already on at probe time and it might be usefull to keep it running as is?
I don't think it can be that the PWM is already on at probe time but it would be useful to ensure that it is off.
Also I will add this comments on the pwm_init_state():
/* Sync up PWM state and ensure it is off. */
pwm_init_state(haptics->pwm_dev, &state);
Do you have any comments more on this?
> > + 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;
> > + }
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Uwe,
Thanks for your comments.
Kind regards,
Roy
