RE: [PATCH V10 3/3] Input: new da7280 haptic driver

[Roy Im]

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