Messages in this thread | | | Date | Tue, 28 Jul 2020 23:36:38 -0700 | From | Dmitry Torokhov <> | Subject | Re: [PATCH v18 3/3] Input: new da7280 haptic driver |
| |
Hi Roy,
On Wed, Jul 29, 2020 at 11:59:40AM +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.
A few questions/suggestions...
> > Reviewed-by: Jes Sorensen <Jes.Sorensen@gmail.com>. > > Signed-off-by: Roy Im <roy.im.opensource@diasemi.com> > > --- > v18: > - Corrected comments in Kconfig > - Updated to preferred style for multi line comments in c file. > v17: > - fixed an issue. > v16: > - Corrected some code and updated description in Kconfig. > v15: > - Removed some defines and updated some comments. > v14: > - Updated pwm related code, alignments and comments. > v13: > - Updated some conditions in pwm function and alignments. > v12: No changes. > v11: > - Updated the pwm related code, comments and typo. > v10: > - Updated the pwm related function and added some comments. > v9: > - Removed the header file and put the definitions into the c file. > - Updated the pwm code and error logs with %pE
I believe the %pE is to format an escaped buffer, you probably want to %pe (lowercase) to print errors. I am also not quite sure if we want to use it in cases when we have non-pointer error, or we should stick with %d as most of the kernel does.
... > + > +/* DA7280_ACTUATOR3 (Address 0x0e) */ > +#define DA7280_IMAX_MASK (31 << 0)
We have GENMASK(h,l) macro in include/linux/bits.h that could be used here and in other mask definitions.
> + > + bool legacy; > + struct delayed_work work_duration; > + struct work_struct work_playback; > + struct work_struct work_setgain;
How do we ensure that all these works do not clash with each other? As far as I can see we could have the "duration" work executing simultaneously with playback...
> +static int da7280_haptics_playback(struct input_dev *dev, > + int effect_id, int val) > +{ > + struct da7280_haptic *haptics = input_get_drvdata(dev); > + > + if (!haptics->op_mode) { > + dev_warn(haptics->dev, > + "Any effects are not uploaded yet\n");
"No effects have been uploaded"?
> + return -EPERM;
I'd say EINVAL.
> +static DEVICE_ATTR_RW(ps_seq_id); > +static DEVICE_ATTR_RW(ps_seq_loop); > +static DEVICE_ATTR_RW(gpi_seq_id0); > +static DEVICE_ATTR_RW(gpi_seq_id1); > +static DEVICE_ATTR_RW(gpi_seq_id2); > +static DEVICE_ATTR_WO(patterns);
Should this be a binary attribute instead of having string parsing in the kernel?
Thanks.
-- Dmitry
| |