Hello, On Mon, Jun 15, 2020 at 08:40:23PM +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 <[email protected]>
Just some picky comments below.
> + 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.)
> + 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,
> + "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?)
> + 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.
> + 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 =.)
> + },
> + .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");
> --
> end-of-patch for RESEND PATCH V13
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
