da7280: haptic driver submission

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

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

[...]

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

+	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) {

Quoted 14 lines

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

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

drivers/input/misc/da7280.c line 391

Quoted 6 lines

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

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

drivers/input/misc/da7280.c line 399

Quoted 8 lines

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

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

Quoted 19 lines

[...]

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

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

drivers/input/misc/da7280.c line 1725

Quoted 6 lines

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

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

drivers/input/misc/da7280.c line 1730

Quoted 5 lines

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

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

drivers/input/misc/da7280.c line 1737

Quoted 7 lines

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

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

Quoted 15 lines

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

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

drivers/input/misc/da7280.c line 391

Quoted 6 lines

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

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

Quoted 4 lines

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

+		 * See datasheet 'PWM mode' section.
+		 */
+		if (!haptics->acc_en) {
+			period_mag_multi += state.period;

drivers/input/misc/da7280.c line 399

Quoted 4 lines

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

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

Quoted 19 lines

[...]

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

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

drivers/input/misc/da7280.c line 1725

Quoted 6 lines

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

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

drivers/input/misc/da7280.c line 1730

Quoted 5 lines

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

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

drivers/input/misc/da7280.c line 1737

Quoted 7 lines

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

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