da7280: haptic driver submission

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 <[email protected]>

drivers/input/misc/da7280.c new file +6 -0

+	return error;
+}
+[...]
+
+		/* Sync up PWM state and ensure it is off. */
+		pwm_init_state(haptics->pwm_dev, &state);

drivers/input/misc/da7280.c new file +9 -0

+	},
+	.probe	  = da7280_probe,
+	.id_table = da7280_i2c_id,
+};
+module_i2c_driver(da7280_driver);
+
+MODULE_DESCRIPTION("DA7280 haptics driver");
+MODULE_AUTHOR("Roy Im <[email protected]>");
+MODULE_LICENSE("GPL");

Signature · 1 line

end-of-patch for RESEND PATCH V13

Hello,

Quoted 5 lines · Roy Im

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 <[email protected]>

Just some picky comments below.

drivers/input/misc/da7280.c line 404

Quoted 5 lines

drivers/input/misc/da7280.c new file +5 -0

+	error = pwm_apply_state(haptics->pwm_dev, &state);
+	if (error)
+		dev_err(haptics->dev,
+			"failed to apply pwm state: %pE\n",
+			ERR_PTR(error));

You can save some horizontal space here by dropping the \n after the first comma. (There are a few more occurences.)

Quoted 10 lines

drivers/input/misc/da7280.c new file +10 -0

+	return error;
+}
+[...]
+
+		/* Sync up PWM state and ensure it is off. */
+		pwm_init_state(haptics->pwm_dev, &state);
+		state.enabled = false;
+		error = pwm_apply_state(haptics->pwm_dev, &state);
+		if (error) {
+			dev_err(dev,

drivers/input/misc/da7280.c line 1744

Quoted 6 lines

drivers/input/misc/da7280.c new file +6 -0

+				"failed to apply initial PWM state: %pE\n",
+				ERR_PTR(error));
+			return error;
+		}
+
+		/* Check PWM Period, it must be in 10k ~ 250kHz */

The unit of a PWM period is in (nano)seconds, to it cannot be in the range [10kHz, 250kHz]. (Is this too picky?)

drivers/input/misc/da7280.c line 1749

Quoted 6 lines

drivers/input/misc/da7280.c new file +5 -0

+		period2freq = 1000000 / state.period;
+		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
+		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
+			dev_err(dev, "Not supported PWM frequency(%d)\n",
+				period2freq);

Doesn't the compiler warn here about period2freq being unsigned and %d being for signed int values?

For my (non-native) English ear s/Not supported/Unsupported/ sounds better. Also my eyes would be pleases by a space before the opening brace.

drivers/input/misc/da7280.c line 1889

Quoted 12 lines

drivers/input/misc/da7280.c new file +11 -0

+			return -EINVAL;
+		}
+	}
+
+
+[...]
+static struct i2c_driver da7280_driver = {
+	.driver	= {
+		.name		= "da7280",
+		.of_match_table	= of_match_ptr(da7280_of_match),
+		.pm		= &da7280_pm_ops,

I don't like alignment of =, but I'm aware this is a subjective thing.
(The reason for me is that adding a new member that is longer than the already existing requires touching all lines. So I stick to a single space before a =.)

Quoted 11 lines

drivers/input/misc/da7280.c new file +9 -1

+	},
+	.probe	  = da7280_probe,
+	.id_table = da7280_i2c_id,
+};
+module_i2c_driver(da7280_driver);
+
+MODULE_DESCRIPTION("DA7280 haptics driver"); MODULE_AUTHOR("Roy Im
+<[email protected]>");
+MODULE_LICENSE("GPL");
--

Quoted 1 line

end-of-patch for RESEND PATCH V13

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Hello Uwe,

Quoted 12 lines · Uwe Kleine-König

Hello,

Quoted 5 lines · Roy Im

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 <[email protected]>

Just some picky comments below.

Quoted 8 lines

drivers/input/misc/da7280.c line 404

Quoted 5 lines

drivers/input/misc/da7280.c new file +5 -0

+	error = pwm_apply_state(haptics->pwm_dev, &state);
+	if (error)
+		dev_err(haptics->dev,
+			"failed to apply pwm state: %pE\n",
+			ERR_PTR(error));

You can save some horizontal space here by dropping the \n after the first comma. (There are a few more occurences.)

Ok, is this what you want?

	error = pwm_apply_state(haptics->pwm_dev, &state);
	if (error)
		dev_err(haptics->dev, "failed to apply pwm state: %pE\n",
			ERR_PTR(error));

Quoted 10 lines

Quoted 10 lines

drivers/input/misc/da7280.c new file +10 -0

+	return error;
+}
+[...]
+
+		/* Sync up PWM state and ensure it is off. */
+		pwm_init_state(haptics->pwm_dev, &state);
+		state.enabled = false;
+		error = pwm_apply_state(haptics->pwm_dev, &state);
+		if (error) {
+			dev_err(dev,

Quoted 8 lines

drivers/input/misc/da7280.c line 1744

Quoted 6 lines

drivers/input/misc/da7280.c new file +6 -0

+				"failed to apply initial PWM state: %pE\n",
+				ERR_PTR(error));
+			return error;
+		}
+
+		/* Check PWM Period, it must be in 10k ~ 250kHz */

The unit of a PWM period is in (nano)seconds, to it cannot be in the range [10kHz, 250kHz]. (Is this too picky?)

No, it isn't. The 'Period' should be changed to 'frequency', I will fix this as below.

		/* Check PWM frequency, valid range: 10k ~ 250kHz */

Quoted 11 lines

drivers/input/misc/da7280.c line 1749

Quoted 6 lines

drivers/input/misc/da7280.c new file +5 -0

+		period2freq = 1000000 / state.period;
+		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
+		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
+			dev_err(dev, "Not supported PWM frequency(%d)\n",
+				period2freq);

Doesn't the compiler warn here about period2freq being unsigned and %d being for signed int values?

For my (non-native) English ear s/Not supported/Unsupported/ sounds better. Also my eyes would be pleases by a space before the opening brace.

I didn't see any warning message, but I will change/improve them to proper one and the log as below.

		period2freq = 1000000 / state.period;
		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
			dev_err(dev, "Unsupported PWM frequency (%u)\n",
				period2freq);
			return -EINVAL;
		}

If you look ok, let me update this so.

Quoted 26 lines · Roy Im

Hello Uwe,

Quoted 18 lines · Uwe Kleine-König

Hello,

drivers/input/misc/da7280.c line 1749

Quoted 11 lines · Roy Im

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 <[email protected]>

drivers/input/misc/da7280.c new file +5 -0

+		period2freq = 1000000 / state.period;
+		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
+		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
+			dev_err(dev, "Not supported PWM frequency(%d)\n",
+				period2freq);

Doesn't the compiler warn here about period2freq being unsigned and %d being for signed int values?

Quoted 2 lines

For my (non-native) English ear s/Not supported/Unsupported/ sounds better. Also my eyes would be pleases by a space before the opening brace.

I didn't see any warning message, but I will change/improve them to proper one and the log as below.

Quoted 8 lines

		period2freq = 1000000 / state.period;
		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
			dev_err(dev, "Unsupported PWM frequency (%u)\n",
				period2freq);
			return -EINVAL;
		}

If you look ok, let me update this so.

looks fine.

Note that you don't need the division if you check for:

	if (state.period > 100000 || state.period < 4000) {
		...

(maybe the compiler is already clever enough to get rid of the division for you, but then the check is:

	if (state.period > 100000 || state.period < 3985) {

because of rounding errors.)

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Quoted 17 lines · Uwe Kleine-König

Quoted 8 lines · Roy Im

		period2freq = 1000000 / state.period;
		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
			dev_err(dev, "Unsupported PWM frequency (%u)\n",
				period2freq);
			return -EINVAL;
		}

If you look ok, let me update this so.

looks fine.

Note that you don't need the division if you check for:

	if (state.period > 100000 || state.period < 4000) {
		...

Quoted 5 lines

(maybe the compiler is already clever enough to get rid of the division for you, but then the check is:

	if (state.period > 100000 || state.period < 3985) {

because of rounding errors.)

OK, you are right, that will be better. So let me change that as below if you look fine.

	/* Check PWM period, PWM freq = 1000000 / state.period.
	 * The valid PWM freq range: 10k ~ 250kHz.
	 */
	if (state.period > 100000 || state.period < 3985) {

		dev_err(dev, "Unsupported PWM period (%u)\n",
			state.period);
		return -EINVAL;
	}

Quoted 18 lines · Uwe Kleine-König

Quoted 17 lines · Roy Im

Quoted 8 lines

		period2freq = 1000000 / state.period;
		if (period2freq < DA7280_MIN_PWM_FREQ_KHZ ||
		    period2freq > DA7280_MAX_PWM_FREQ_KHZ) {
			dev_err(dev, "Unsupported PWM frequency (%u)\n",
				period2freq);
			return -EINVAL;
		}

If you look ok, let me update this so.

looks fine.

Note that you don't need the division if you check for:

	if (state.period > 100000 || state.period < 4000) {
		...

Quoted 8 lines

Quoted 5 lines

(maybe the compiler is already clever enough to get rid of the division for you, but then the check is:

	if (state.period > 100000 || state.period < 3985) {

because of rounding errors.)

OK, you are right, that will be better. So let me change that as below if you look fine.

I look fine, the code however is wrong. :-)

Quoted 4 lines

	/* Check PWM period, PWM freq = 1000000 / state.period.
	 * The valid PWM freq range: 10k ~ 250kHz.
	 */
	if (state.period > 100000 || state.period < 3985) {

You want 4000 here ---------------------------------^^^^, don't you?

Quoted 5 lines

		dev_err(dev, "Unsupported PWM period (%u)\n",
			state.period);
		return -EINVAL;
	}

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |