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

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:

quoted

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 <redacted>

---
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.

[...]

quoted

+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.

quoted

+	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;
	}

quoted

[...]
+		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.

quoted

+		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