Re: [RESEND] Input: support pre-stored effects

[Jeff LaBundy]

Hi James,

On Thu, Jun 15, 2023 at 06:12:20PM +0000, James Ogletree wrote:
> 
> 
> > On Jun 12, 2023, at 8:25 PM, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
> > 
> > Hi James,
> > 
> > On Mon, Jun 12, 2023 at 07:43:57PM +0000, James Ogletree wrote:
> >> At present, the best way to define effects that
> >> pre-exist in device memory is by utilizing
> >> the custom_data field, which it was not intended
> >> for, and requires arbitrary interpretation by
> >> the driver to make meaningful.
> >> 
> >> Provide option for defining pre-stored effects in
> >> device memory.
> >> 
> >> Signed-off-by: James Ogletree <james.ogletree@xxxxxxxxxx>
> >> ---
> >> include/uapi/linux/input.h | 32 ++++++++++++++++++++++----------
> >> 1 file changed, 22 insertions(+), 10 deletions(-)
> >> 
> >> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> >> index 2557eb7b0561..689e5fa10647 100644
> >> --- a/include/uapi/linux/input.h
> >> +++ b/include/uapi/linux/input.h
> >> @@ -428,17 +428,27 @@ struct ff_rumble_effect {
> >> __u16 weak_magnitude;
> >> };
> >> 
> >> +/**
> >> + * struct ff_prestored_effect - defines parameters of a pre-stored force-feedback effect
> >> + * @index: index of effect
> >> + * @bank: memory bank of effect
> >> + */
> >> +struct ff_prestored_effect {
> >> + __u16 index;
> >> + __u16 bank;
> >> +};
> > 
> > This seems like a good start; I do wonder if it's useful to review recent
> > customer vibrator HAL implementations and decide whether you want to pack
> > any additional members here such as magnitude, etc. as is done for periodic
> > effects?
> > 
> > Back in L25 days, some customers would assign click or tap effects to one
> > or more entries in the wavetable and then use a separate digital volume
> > control (at that time exposed through sysfs) to create a few discrete
> > amplitude levels. Perhaps it would be handy to bundle this information as
> > part of the same call?
> > 
> > It's just a suggestion; I'll defer to your much more recent expertise.
> > 
> 
> My thinking is that ff_prestored_effect ought to be for effects being used
> “off-the-shelf”, and in such cases it would seem appropriate to defer to
> firmware for the effect design. I think this fits nicely as-is with the other
> structures as it serves a clear and distinct use-case. Otherwise one might
> just add these two members to ff_periodic_effect (or every kind of effect).
> 
> I think the current predominant method for setting "magnitude" for these
> pre-stored effects is by using the FF_GAIN event code as a separate write
> call, so I think adding a magnitude member would go unused, if I understand
> you correctly.

All great points. In that case:

Reviewed-by: Jeff LaBundy <jeff@xxxxxxxxxxx>

> 
> Thanks,
> James
> 
> 
> 

Kind regards,
Jeff LaBundy