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

[Roy Im]

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