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@cirrus.com>
---
 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;
+};
+
 /**
  * struct ff_effect - defines force feedback effect
  * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
- *	FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
+ *	FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, FF_PRESTORED, or FF_CUSTOM)
  * @id: an unique id assigned to an effect
  * @direction: direction of the effect
  * @trigger: trigger conditions (struct ff_trigger)
  * @replay: scheduling of the effect (struct ff_replay)
  * @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
- *	ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
- *	defining effect parameters
+ *	ff_periodic_effect, ff_condition_effect, ff_rumble_effect, ff_prestored_effect)
+ *	further defining effect parameters
  *
  * This structure is sent through ioctl from the application to the driver.
  * To create a new effect application should set its @id to -1; the kernel
@@ -464,6 +474,7 @@ struct ff_effect {
 		struct ff_periodic_effect periodic;
 		struct ff_condition_effect condition[2]; /* One for each axis */
 		struct ff_rumble_effect rumble;
+		struct ff_prestored_effect prestored;
 	} u;
 };
 
@@ -479,20 +490,21 @@ struct ff_effect {
 #define FF_DAMPER	0x55
 #define FF_INERTIA	0x56
 #define FF_RAMP		0x57
+#define FF_PRESTORED	0x58
 
 #define FF_EFFECT_MIN	FF_RUMBLE
-#define FF_EFFECT_MAX	FF_RAMP
+#define FF_EFFECT_MAX	FF_PRESTORED
 
 /*
  * Force feedback periodic effect types
  */
 
-#define FF_SQUARE	0x58
-#define FF_TRIANGLE	0x59
-#define FF_SINE		0x5a
-#define FF_SAW_UP	0x5b
-#define FF_SAW_DOWN	0x5c
-#define FF_CUSTOM	0x5d
+#define FF_SQUARE	0x59
+#define FF_TRIANGLE	0x5a
+#define FF_SINE		0x5b
+#define FF_SAW_UP	0x5c
+#define FF_SAW_DOWN	0x5d
+#define FF_CUSTOM	0x5e
 
 #define FF_WAVEFORM_MIN	FF_SQUARE
 #define FF_WAVEFORM_MAX	FF_CUSTOM
-- 
2.25.1

Hi Dmitry,

> On Jun 12, 2023, at 2:43 PM, James Ogletree <James.Ogletree@cirrus.com> 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@cirrus.com>
> ---
> 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;
> +};
> +
> /**
>  * struct ff_effect - defines force feedback effect
>  * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
> - * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
> + * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, FF_PRESTORED, or FF_CUSTOM)
>  * @id: an unique id assigned to an effect
>  * @direction: direction of the effect
>  * @trigger: trigger conditions (struct ff_trigger)
>  * @replay: scheduling of the effect (struct ff_replay)
>  * @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
> - * ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
> - * defining effect parameters
> + * ff_periodic_effect, ff_condition_effect, ff_rumble_effect, ff_prestored_effect)
> + * further defining effect parameters
>  *
>  * This structure is sent through ioctl from the application to the driver.
>  * To create a new effect application should set its @id to -1; the kernel
> @@ -464,6 +474,7 @@ struct ff_effect {
> struct ff_periodic_effect periodic;
> struct ff_condition_effect condition[2]; /* One for each axis */
> struct ff_rumble_effect rumble;
> + struct ff_prestored_effect prestored;
> } u;
> };
> 
> @@ -479,20 +490,21 @@ struct ff_effect {
> #define FF_DAMPER 0x55
> #define FF_INERTIA 0x56
> #define FF_RAMP 0x57
> +#define FF_PRESTORED 0x58
> 
> #define FF_EFFECT_MIN FF_RUMBLE
> -#define FF_EFFECT_MAX FF_RAMP
> +#define FF_EFFECT_MAX FF_PRESTORED
> 
> /*
>  * Force feedback periodic effect types
>  */
> 
> -#define FF_SQUARE 0x58
> -#define FF_TRIANGLE 0x59
> -#define FF_SINE 0x5a
> -#define FF_SAW_UP 0x5b
> -#define FF_SAW_DOWN 0x5c
> -#define FF_CUSTOM 0x5d
> +#define FF_SQUARE 0x59
> +#define FF_TRIANGLE 0x5a
> +#define FF_SINE 0x5b
> +#define FF_SAW_UP 0x5c
> +#define FF_SAW_DOWN 0x5d
> +#define FF_CUSTOM 0x5e
> 
> #define FF_WAVEFORM_MIN FF_SQUARE
> #define FF_WAVEFORM_MAX FF_CUSTOM
> -- 
> 2.25.1
> 

Is there something more you’d like to see or discuss before considering applying this?

Best,

James

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@cirrus.com>
> ---
>  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.

> +
>  /**
>   * struct ff_effect - defines force feedback effect
>   * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
> - *	FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
> + *	FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, FF_PRESTORED, or FF_CUSTOM)
>   * @id: an unique id assigned to an effect
>   * @direction: direction of the effect
>   * @trigger: trigger conditions (struct ff_trigger)
>   * @replay: scheduling of the effect (struct ff_replay)
>   * @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
> - *	ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
> - *	defining effect parameters
> + *	ff_periodic_effect, ff_condition_effect, ff_rumble_effect, ff_prestored_effect)
> + *	further defining effect parameters
>   *
>   * This structure is sent through ioctl from the application to the driver.
>   * To create a new effect application should set its @id to -1; the kernel
> @@ -464,6 +474,7 @@ struct ff_effect {
>  		struct ff_periodic_effect periodic;
>  		struct ff_condition_effect condition[2]; /* One for each axis */
>  		struct ff_rumble_effect rumble;
> +		struct ff_prestored_effect prestored;
>  	} u;
>  };
>  
> @@ -479,20 +490,21 @@ struct ff_effect {
>  #define FF_DAMPER	0x55
>  #define FF_INERTIA	0x56
>  #define FF_RAMP		0x57
> +#define FF_PRESTORED	0x58
>  
>  #define FF_EFFECT_MIN	FF_RUMBLE
> -#define FF_EFFECT_MAX	FF_RAMP
> +#define FF_EFFECT_MAX	FF_PRESTORED
>  
>  /*
>   * Force feedback periodic effect types
>   */
>  
> -#define FF_SQUARE	0x58
> -#define FF_TRIANGLE	0x59
> -#define FF_SINE		0x5a
> -#define FF_SAW_UP	0x5b
> -#define FF_SAW_DOWN	0x5c
> -#define FF_CUSTOM	0x5d
> +#define FF_SQUARE	0x59
> +#define FF_TRIANGLE	0x5a
> +#define FF_SINE		0x5b
> +#define FF_SAW_UP	0x5c
> +#define FF_SAW_DOWN	0x5d
> +#define FF_CUSTOM	0x5e
>  
>  #define FF_WAVEFORM_MIN	FF_SQUARE
>  #define FF_WAVEFORM_MAX	FF_CUSTOM
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy

> On Jun 12, 2023, at 8:25 PM, Jeff LaBundy <jeff@labundy.com> 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@cirrus.com>
>> ---
>> 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.

Thanks,
James

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@labundy.com> 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@cirrus.com>
> >> ---
> >> 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@labundy.com>

> 
> Thanks,
> James
> 
> 
> 

Kind regards,
Jeff LaBundy