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

[Roy Im <roy.im.opensource@diasemi.com>]

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.
> 
> Can it be that the PWM is already on at probe time and it might be usefull to keep it running as is?

Yes, that's right. I think so too..
Although it may not be that the PWM is already on at probe time but it may be useful to ensure that it is off for something unexpected.
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